Bug in IMMEDIATE_CRASH macro, for MSVC, for is_official_build=true

[Paul Harris]

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:

https://chromium-review.googlesource.com/c/v8/v8/+/3899298/95

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

https://chromium-review.googlesource.com/c/v8/v8/+/3899298/95/src/compiler/turboshaft/assembler.h

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

[Paul Harris]

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_

[Jakob Kummerow]

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.

--

[Paul Harris]

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)

[Jakob Kummerow]

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.

[Daryl Haresign]

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 <jkum...@chromium.org> wrote:



--
--
v8-dev mailing list
v8-...@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+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAKSzg3RtpP7Sst-npC5rMdG07zWSh_8fFa51aCgDXQQywzNjUg%40mail.gmail.com.

[Paul Harris]

Hi all,

I thought I posted this message but it disappeared.

I've figured it out, but I'd like some help submitting the patch.
I'm not geared up to build all the tests, it'll take hours and hours on MSVC.

And these are small patches...

The problem was remembered-set.cc checked CPPGC_YOUNG_GENERATION too early, before any headers were included.

And the CPPGC_YOUNG_GENERATION definition is not in the compiler command line.

This is the command line:

[22/1084] ninja -t msvc -e environment.x64 -- "C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.35.32215\bin\Hostx64\x64\cl.exe" /c v8/src/heap/cppgc/remembered-set.cc /Foobj/cppgc_base/remembered-set.obj /nologo /showIncludes -DUSE_AURA=1 -DOFFICIAL_BUILD -D_CRT_NONSTDC_NO_WARNINGS -D_WINSOCK_DEPRECATED_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D__STD_C -D_CRT_RAND_S -D_CRT_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_DEPRECATE -D_ATL_NO_OPENGL -D_WINDOWS -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DPSAPI_VERSION=2 -DWIN32 -D_SECURE_ATL -DWINAPI_FAMILY=WINAPI_FAMILY_DESKTOP_APP -DWIN32_LEAN_AND_MEAN -DNOMINMAX -D_UNICODE -DUNICODE -DNTDDI_VERSION=NTDDI_WIN10_FE -D_WIN32_WINNT=0x0A00 -DWINVER=0x0A00 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DV8_TYPED_ARRAY_MAX_SIZE_IN_HEAP=64 -DOBJECT_PRINT -DV8_INTL_SUPPORT -DV8_ATOMIC_OBJECT_FIELD_WRITES -DV8_ENABLE_LAZY_SOURCE_POSITIONS -DV8_SHARED_RO_HEAP -DV8_WIN64_UNWINDING_INFO -DV8_ENABLE_REGEXP_INTERPRETER_THREADED_DISPATCH -DV8_SHORT_BUILTIN_CALLS -DV8_EXTERNAL_CODE_SPACE -DV8_ENABLE_MAGLEV -DV8_ENABLE_SYSTEM_INSTRUMENTATION -DV8_ENABLE_ETW_STACK_WALKING -DV8_ENABLE_WEBASSEMBLY -DV8_ALLOCATION_FOLDING -DV8_ALLOCATION_SITE_TRACKING -DV8_ADVANCED_BIGINT_ALGORITHMS -DV8_USE_ZLIB -DV8_GN_HEADER -DV8_TARGET_ARCH_X64 -DV8_HAVE_TARGET_OS -DV8_TARGET_OS_WIN -Iv8 -Igen -Iv8/include -Igen/include /wd4244 /Gy /FS /bigobj /utf-8 /Zc:sizedDealloc- /MD /wd4716 /wd4245 /wd4267 /wd4324 /wd4701 /wd4702 /wd4703 /wd4709 /wd4714 /wd4715 /wd4718 /wd4723 /wd4724 /wd4800 /wd4506 /wd4091 /wd4127 /wd4251 /wd4275 /wd4312 /wd4324 /wd4351 /wd4355 /wd4503 /wd4589 /wd4611 /wd4100 /wd4121 /wd4244 /wd4505 /wd4510 /wd4512 /wd4610 /wd4838 /wd4995 /wd4996 /wd4456 /wd4457 /wd4458 /wd4459 /wd4200 /wd4201 /wd4204 /wd4221 /wd4245 /wd4267 /wd4305 /wd4389 /wd4702 /wd4701 /wd4703 /wd4661 /wd4706 /wd4715 /O2 /Ob2 /Oy- /Zc:inline /Gw /std:c++20 /TP /GR- /std:c++17 /Fd"obj/cppgc_base_cc.pdb"

And this is the patch that finally allowed v8 to build.

Note this is for 11.0.226.19 but I had a quick look at more recent branches, and nothing related appears to have changed.

diff --git a/src/base/immediate-crash.h b/src/base/immediate-crash.h
index 770cb273f9..40199412ce 100644
--- a/src/base/immediate-crash.h
+++ b/src/base/immediate-crash.h
@@ -155,7 +155,14 @@

 #else
 
 // This is supporting build with MSVC where there is no __builtin_unreachable().
-#define IMMEDIATE_CRASH() WRAPPED_TRAP_SEQUENCE_()

+// Note that it cannot be: #define IMMEDIATE_CRASH WRAPPED_TRAP_SEQUENCE_()
+// because in src/compiler/turboshaft/machine-optimization-reducer.h ...
+// LABEL_BLOCK(no_change) expands to:
+// for (; false; do { __debugbreak(); ; } while (false))
+// and MSVC does not support do/while inside a for loop like that.
+//
+// So instead, this will expand to: for (; false; IMMEDIATE_CRASH())

+inline void IMMEDIATE_CRASH() { WRAPPED_TRAP_SEQUENCE_(); }
 
 #endif  // defined(__clang__) || defined(COMPILER_GCC)
 

diff --git a/src/heap/cppgc/remembered-set.cc b/src/heap/cppgc/remembered-set.cc
index 0a342aa344..0cbb1b54a9 100644
--- a/src/heap/cppgc/remembered-set.cc
+++ b/src/heap/cppgc/remembered-set.cc
@@ -2,8 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#if defined(CPPGC_YOUNG_GENERATION)
-
 #include "src/heap/cppgc/remembered-set.h"
 
 #include <algorithm>
@@ -17,6 +15,8 @@
 #include "src/heap/cppgc/heap-visitor.h"
 #include "src/heap/cppgc/marking-state.h"
 
+#if defined(CPPGC_YOUNG_GENERATION)
+
 namespace cppgc {
 namespace internal {
 
diff --git a/src/heap/cppgc/remembered-set.h b/src/heap/cppgc/remembered-set.h
index 763b1afe32..50ce59b36f 100644
--- a/src/heap/cppgc/remembered-set.h
+++ b/src/heap/cppgc/remembered-set.h
@@ -5,14 +5,14 @@
 #ifndef V8_HEAP_CPPGC_REMEMBERED_SET_H_
 #define V8_HEAP_CPPGC_REMEMBERED_SET_H_
 
-#if defined(CPPGC_YOUNG_GENERATION)
-
 #include <set>
 
 #include "src/base/macros.h"
 #include "src/heap/base/basic-slot-set.h"
 #include "src/heap/cppgc/marking-worklists.h"
 
+#if defined(CPPGC_YOUNG_GENERATION)
+
 namespace cppgc {
 
 class Visitor;

--------------------------------------------------

Can anyone help submit this upstream please?

Thanks very much,
Paul

[Jakob Kummerow]

Can you follow https://v8.dev/docs/contribute to submit the patch for review please?