Re: [v8-dev] Re: Bug in IMMEDIATE_CRASH macro, for MSVC, for is_official_build=true

Mon, 20 Mar 2023 17:20:12 -0700

You probably need it on the WeakCallbackItem struct in marking-worklists.h?

Sent from my iPhone

On Mar 20, 2023, at 15:16, Jakob Kummerow <jkumme...@chromium.org> wrote:


I would have suggested V8_EXPORT_PRIVATE, but that annotation is already on that class, so... I don't know.

Try the static library build. The shared library build on Windows has often been reported to be broken.


On Mon, Mar 20, 2023 at 4:22 PM Paul Harris <harris...@gmail.com> wrote:
Ah ok, thanks for that info.

That might also explain why I'm getting linker errors, eg 1 of about a dozen similar linker errors... any ideas how to patch this one?

cppgc_base.lib(marker.obj) : error LNK2019: unresolved external symbol "public: void __cdecl cppgc::internal::OldToNewRememberedSet::AddWeakCallback(struct cppgc::internal::MarkingWorklists::WeakCallbackItem)" (?AddWeakCallback@OldToNewRememberedSet@internal@cppgc@@QEAAXUWeakCallbackItem@MarkingWorklists@23@@Z) referenced in function "public: void __cdecl cppgc::internal::MarkerBase::ProcessWeakness(void)" (?ProcessWeakness@MarkerBase@internal@cppgc@@QEAAXXZ)


On Monday, March 20, 2023 at 9:36:38 PM UTC+8 Jakob Kummerow wrote:
Like other non-Clang compilers, MSVC is largely community-supported: we have a CI bot for it that we're keeping green, but it only tests one configuration, and other configurations (e.g. other MSVC versions, or other GN args) tend to break from time to time. Please feel free to submit a patch to fix MSVC-related issues.


On Fri, Mar 17, 2023 at 5:13 AM Paul Harris <harr...@gmail.com> wrote:
Sorry, that patch won't work as you can't put ({ }) code just anywhere in MSVC (gcc likes it).

I gave up trying to please msvc, and in the end moved forward with this patch... ie using an inline function
I know it'll probably mess with the crash location of chromium dumps, but it was time to cut losses...

--- a/src/base/immediate-crash.h
+++ b/src/base/immediate-crash.h
@@ -154,9 +154,9 @@

 #else

 // This is supporting build with MSVC where there is no __builtin_unreachable().
-#define IMMEDIATE_CRASH() WRAPPED_TRAP_SEQUENCE_()
+inline void IMMEDIATE_CRASH() { WRAPPED_TRAP_SEQUENCE_(); }

 #endif  // defined(__clang__) || defined(COMPILER_GCC)

 #endif  // V8_BASE_IMMEDIATE_CRASH_H_


On Friday, March 17, 2023 at 11:13:05 AM UTC+8 Paul Harris wrote:
Hello all,

I'm building 11.0.226.19 with VS 17.5.1
ie MSVC 
ie cl.exe 19.35.32215

tldr is:
```
--- immediate-crash.h.orig      2023-03-17 10:59:27.251900900 +0800
+++ immediate-crash.h   2023-03-17 10:59:37.427856200 +0800
@@ -144,19 +144,22 @@

 #if defined(__clang__) || V8_CC_GCC

 // __builtin_unreachable() hints to the compiler that this is noreturn and can
 // be packed in the function epilogue.
 #define IMMEDIATE_CRASH()     \
   ({                          \
     WRAPPED_TRAP_SEQUENCE_(); \
     __builtin_unreachable();  \
   })

 #else

 // This is supporting build with MSVC where there is no __builtin_unreachable().
-#define IMMEDIATE_CRASH() WRAPPED_TRAP_SEQUENCE_()
+#define IMMEDIATE_CRASH()     \
+  ({                          \
+    WRAPPED_TRAP_SEQUENCE_(); \
+  })

 #endif  // defined(__clang__) || defined(COMPILER_GCC)

 #endif  // V8_BASE_IMMEDIATE_CRASH_H_
```

I'm not sure why MSVC didn't get the ({ brackets })
nor do I know why noone else has seen this yet.

The problem started with this change:

In particular, the new macro LABEL_BLOCK in line 59 here:


When building with is_official_build=true, the following happens:

src/compiler/turboshaft/machine-optimization-reducer.h : 1669
  OpIndex ReducePhi(base::Vector<const OpIndex> inputs,
                    RegisterRepresentation rep) {
    LABEL_BLOCK(no_change) { return Next::ReducePhi(inputs, rep); }


LABEL_BLOCK line becomes:
for (; false; UNREACHABLE()) no_change: { return Next::ReducePhi(inputs, rep); 
}

focus on the for loop, that becomes:
for (; false; FATAL(message))
becomes:
for (; false; IMMEDIATE_CRASH())
becomes:
  for (; false; WRAPPED_TRAP_SEQUENCE_())
becomes:
  for (; false; TRAP_SEQUENCE_())
becomes:
for (; false; do {  TRAP_SEQUENCE1_(); TRAP_SEQUENCE2_(); } while (false))
becomes:
for (; false; do { __debugbreak(); ; } while (false))
Which fails on all compilers (including gcc) thanks to the do following the ;

This would have compiled if it had some extra brackets in there:
for (; false; ({ do { __debugbreak(); ; } while (false) }) )

What can we do about putting in this tiny patch?
And, is noone at google using MSVC ?

cheers,
Paul

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAKSzg3RtpP7Sst-npC5rMdG07zWSh_8fFa51aCgDXQQywzNjUg%40mail.gmail.com.

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/7FC53A5B-B12D-48A9-9BA4-035CB6C41B3F%40haresign.com.

Reply via email to