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
On Tuesday, March 21, 2023 at 8:20:09 AM UTC+8 da...@haresign.com wrote:
> 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:
>
>
>
> 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 <harr...@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 <https://v8.dev/docs/contribute> 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:
>>>>> 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
>>>>>
>>>>> --
> --
> 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
>
> <https://groups.google.com/d/msgid/v8-dev/CAKSzg3RtpP7Sst-npC5rMdG07zWSh_8fFa51aCgDXQQywzNjUg%40mail.gmail.com?utm_medium=email&utm_source=footer>
> .
>
>
--
--
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/b6f8d90f-a0b1-45ca-a81e-ffa09333e6ccn%40googlegroups.com.
