Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: update V8 to 10.2 #42115

Closed
wants to merge 18 commits into from
Closed

deps: update V8 to 10.2 #42115

wants to merge 18 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Feb 24, 2022

ETA: Tue, Mar 1, 2022

@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 24, 2022
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Feb 24, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 24, 2022

@targos
Copy link
Member Author

targos commented Feb 24, 2022

New Windows failure, as usual 😞

15:01:51 C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\xutility(4198,16): error C2280: 'std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>> &std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>::operator =(const std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>> &)': attempting to reference a deleted function [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_init.vcxproj]
15:01:51 C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\memory(3270): message : see declaration of 'std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>::operator =' [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_init.vcxproj]
15:01:51 C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\memory(3270,17): message : 'std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>> &std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>::operator =(const std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>> &)': function was explicitly deleted [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_init.vcxproj]
15:01:51 C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\vector(1175): message : see reference to function template instantiation '_OutIt *std::_Copy_unchecked<_Iter,std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>*>(_InIt,_InIt,_OutIt)' being compiled [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_init.vcxproj]
15:01:51           with
15:01:51           [
15:01:51               _OutIt=std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>> *,
15:01:51               _Iter=std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>> *,
15:01:51               _InIt=std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>> *
15:01:51           ]
15:01:51 C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\vector(1189): message : see reference to function template instantiation 'void std::vector<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>,std::allocator<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>>>::_Assign_range<_Iter>(_Iter,_Iter,std::forward_iterator_tag)' being compiled [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_init.vcxproj]
15:01:51           with
15:01:51           [
15:01:51               _Iter=std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>> *
15:01:51           ]
15:01:51 C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\vector(1189): message : see reference to function template instantiation 'void std::vector<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>,std::allocator<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>>>::_Assign_range<_Iter>(_Iter,_Iter,std::forward_iterator_tag)' being compiled [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_init.vcxproj]
15:01:51           with
15:01:51           [
15:01:51               _Iter=std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>> *
15:01:51           ]
15:01:51 C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\vector(1200): message : see reference to function template instantiation 'void std::vector<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>,std::allocator<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>>>::assign<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>*,0>(_Iter,_Iter)' being compiled [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_init.vcxproj]
15:01:51           with
15:01:51           [
15:01:51               _Iter=std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>> *
15:01:51           ]
15:01:51 C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\vector(1200): message : see reference to function template instantiation 'void std::vector<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>,std::allocator<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>>>::assign<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>*,0>(_Iter,_Iter)' being compiled [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_init.vcxproj]
15:01:51           with
15:01:51           [
15:01:51               _Iter=std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>> *
15:01:51           ]
15:01:51 C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\vector(1197): message : while compiling class template member function 'void std::vector<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>,std::allocator<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>>>::_Copy_assign(const std::vector<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>,std::allocator<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>>> &,std::false_type)' [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_init.vcxproj]
15:01:51 C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\vector(1216): message : see reference to function template instantiation 'void std::vector<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>,std::allocator<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>>>::_Copy_assign(const std::vector<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>,std::allocator<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>>> &,std::false_type)' being compiled [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_init.vcxproj]
15:01:51 C:\workspace\node-compile-windows\node\deps\v8\include\cppgc\heap.h(126): message : see reference to class template instantiation 'std::vector<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>,std::allocator<std::unique_ptr<cppgc::CustomSpaceBase,std::default_delete<cppgc::CustomSpaceBase>>>>' being compiled [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_init.vcxproj]

@targos
Copy link
Member Author

targos commented Feb 25, 2022

I created a V8 issue: https://bugs.chromium.org/p/v8/issues/detail?id=12661

@targos
Copy link
Member Author

targos commented Mar 1, 2022

Anyone knows how we could work around the Windows error?

@bnoordhuis
Copy link
Member

Untested, but does this diff work?

diff --git a/include/cppgc/heap.h b/include/cppgc/heap.h
index 136c4fb44d..e677bb2845 100644
--- a/include/cppgc/heap.h
+++ b/include/cppgc/heap.h
@@ -111,13 +111,6 @@ class V8_EXPORT Heap {
    * heap through `Heap::Create()`.
    */
   struct HeapOptions {
-    /**
-     * Creates reasonable defaults for instantiating a Heap.
-     *
-     * \returns the HeapOptions that can be passed to `Heap::Create()`.
-     */
-    static HeapOptions Default() { return {}; }
-
     /**
      * Custom spaces added to heap are required to have indices forming a
      * numbered sequence starting at 0, i.e., their `kSpaceIndex` must
@@ -161,7 +154,7 @@ class V8_EXPORT Heap {
    */
   static std::unique_ptr<Heap> Create(
       std::shared_ptr<Platform> platform,
-      HeapOptions options = HeapOptions::Default());
+      HeapOptions options = {});
 
   virtual ~Heap() = default;
 

I think the compiler is allowed to fail here because it's not required to apply copy elision in Default().

@targos
Copy link
Member Author

targos commented Mar 1, 2022

Let's try it!

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 1, 2022

@targos
Copy link
Member Author

targos commented Mar 1, 2022

@bnoordhuis GCC doesn't like it:

18:22:24 ../deps/v8/include/cppgc/heap.h:157:30: error: could not convert '<brace-enclosed initializer list>()' from '<brace-enclosed initializer list>' to 'cppgc::Heap::HeapOptions'
18:22:24        HeapOptions options = {});
18:22:24                               ^

@targos
Copy link
Member Author

targos commented Mar 1, 2022

Also, MSVC still gives the same error as before, I think.

@bnoordhuis
Copy link
Member

Maybe msvc is tripping over its own auto-generated copy constructors. Does explicitly deleting them help?

diff --git a/include/cppgc/heap.h b/include/cppgc/heap.h
index 136c4fb44d..23c1756abb 100644
--- a/include/cppgc/heap.h
+++ b/include/cppgc/heap.h
@@ -150,6 +150,9 @@ class V8_EXPORT Heap {
      * GC scheduler follows.
      */
     ResourceConstraints resource_constraints;
+
+    HeapOptions(const HeapOptions&) = delete;
+    void operator=(const HeapOptions&) = delete;
   };
 
   /**
diff --git a/include/v8-cppgc.h b/include/v8-cppgc.h
index af21054e2b..7d11c245c7 100644
--- a/include/v8-cppgc.h
+++ b/include/v8-cppgc.h
@@ -80,6 +80,9 @@ struct WrapperDescriptor final {
 struct V8_EXPORT CppHeapCreateParams {
   std::vector<std::unique_ptr<cppgc::CustomSpaceBase>> custom_spaces;
   WrapperDescriptor wrapper_descriptor;
+
+  CppHeapCreateParams(const CppHeapCreateParams&) = delete;
+  void operator=(const CppHeapCreateParams&) = delete;
 };
 
 /**

The gcc error can probably be fixed by writing HeapOptions options = HeapOptions().

@targos
Copy link
Member Author

targos commented Mar 2, 2022

That error is gone, thanks!

Now we have:

  Checking inspector protocol compatibility, and also Generating inspector protocol sources from protocol json
  Traceback (most recent call last):
    File "D:\a\node\node\deps\v8\third_party\inspector_protocol\code_generator.py", line 702, in <module>
      main()
    File "D:\a\node\node\deps\v8\third_party\inspector_protocol\code_generator.py", line 572, in main
      protocol = Protocol(config)
    File "D:\a\node\node\deps\v8\third_party\inspector_protocol\code_generator.py", line 360, in __init__
      self.generate_domains = self.read_protocol_file(config.protocol.path)
  AttributeError: 'X' object has no attribute 'path'

I going to investigate on my machine, because 32dd0d7#diff-33f548d8c76be9f5052a98ec2fff58be393ac5b920e0c997227bf17cbb2deaa0R169 was supposed to take care of it.

@targos
Copy link
Member Author

targos commented Mar 2, 2022

The generated argument is wrong:
image

@targos
Copy link
Member Author

targos commented Mar 2, 2022

Also:

In file included from /usr/include/c++/9/memory:80,
                 from ../deps/v8/include/cppgc/heap.h:10,
                 from ../deps/v8/src/heap/cppgc/heap.h:8,
                 from ../deps/v8/src/heap/cppgc/heap.cc:5:
/usr/include/c++/9/bits/unique_ptr.h: In instantiation of ‘typename std::_MakeUniq<_Tp>::__single_object std::make_unique(_Args&& ...) [with _Tp = cppgc::internal::Heap; _Args = {std::shared_ptr<cppgc::Platform>, cppgc::Heap::HeapOptions}; typename std::_MakeUniq<_Tp>::__single_object = std::unique_ptr<cppgc::internal::Heap, std::default_delete<cppgc::internal::Heap> >]’:
../deps/v8/src/heap/cppgc/heap.cc:42:61:   required from here
/usr/include/c++/9/bits/unique_ptr.h:857:30: error: use of deleted function ‘cppgc::Heap::HeapOptions::HeapOptions(const cppgc::Heap::HeapOptions&)’
  857 |     { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }
      |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../deps/v8/src/heap/cppgc/heap.h:8,
                 from ../deps/v8/src/heap/cppgc/heap.cc:5:
../deps/v8/include/cppgc/heap.h:154:5: note: declared here
  154 |     HeapOptions(const HeapOptions&) = delete;
      |     ^~~~~~~~~~~
../deps/v8/src/heap/cppgc/heap.cc:80:37: note:   initializing argument 2 of ‘cppgc::internal::Heap::Heap(std::shared_ptr<cppgc::Platform>, cppgc::Heap::HeapOptions)’
   80 |            cppgc::Heap::HeapOptions options)
      |            ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
make[2]: *** [tools/v8_gypfiles/v8_base_without_compiler.target.mk:1005: /home/runner/work/node/node/out/Release/obj.target/v8_base_without_compiler/deps/v8/src/heap/cppgc/heap.o] Error 1
make[2]: *** Waiting for unfinished jobs....

@targos
Copy link
Member Author

targos commented Mar 2, 2022

The generated argument is wrong: image

The problem is the path fixing logic from

arguments = [i if (i[:1] in "/-") else _FixPath(i, "/") for i in cmd[1:]]

@targos
Copy link
Member Author

targos commented Mar 2, 2022

Same error about the deleted function on windows:

C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.31.31103\include\memory(3417,59): error C2280: 'cppgc::Heap::HeapOptions::HeapOptions(const cppgc::Heap::HeapOptions &)' : tentative de référencemen
t d'une fonction supprimée [D:\Git\nodejs\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]
D:\Git\nodejs\node\deps\v8\include\cppgc\heap.h(154): message : voir la déclaration de 'cppgc::Heap::HeapOptions::HeapOptions' [D:\Git\nodejs\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]
D:\Git\nodejs\node\deps\v8\src\heap\cppgc\heap.cc(42): message : voir la référence à l'instanciation de la fonction modèle 'std::unique_ptr<cppgc::internal::Heap,std::default_delete<cppgc::internal::Heap>> std::make_uniqu
e<cppgc::internal::Heap,std::shared_ptr<cppgc::Platform>,cppgc::Heap::HeapOptions,0>(std::shared_ptr<cppgc::Platform> &&,cppgc::Heap::HeapOptions &&)' en cours de compilation [D:\Git\nodejs\node\tools\v8_gypfiles\v8_base_ 
without_compiler.vcxproj]
```

@jasnell
Copy link
Member

jasnell commented Mar 2, 2022

FYI: #42192 ... tl;dr 9.9 updates the value serialization format such that any version of Node.js using < 9.9 won't be able to deserialize anything serialized with >= 9.9.

@targos
Copy link
Member Author

targos commented Mar 8, 2022

@gengjiawen Do you have any idea about a magical workaround?

@gengjiawen
Copy link
Member

@gengjiawen Do you have any idea about a magical workaround?

I should have time on weekends. But I need a windows vm if this not fixed this at that time.

@gengjiawen
Copy link
Member

gengjiawen commented Mar 17, 2022

@targos Please try, I have verified on vs2022. I will upstream it later.

diff --git a/deps/v8/include/v8-cppgc.h b/deps/v8/include/v8-cppgc.h
index af21054e2b..43be774632 100644
--- a/deps/v8/include/v8-cppgc.h
+++ b/deps/v8/include/v8-cppgc.h
@@ -78,6 +78,9 @@ struct WrapperDescriptor final {
 };
 
 struct V8_EXPORT CppHeapCreateParams {
+  CppHeapCreateParams(const CppHeapCreateParams&) = delete;
+  CppHeapCreateParams& operator=(const CppHeapCreateParams&) = delete;
+
   std::vector<std::unique_ptr<cppgc::CustomSpaceBase>> custom_spaces;
   WrapperDescriptor wrapper_descriptor;
 }

@targos
Copy link
Member Author

targos commented Apr 6, 2022

I opened nodejs/gyp-next#143 for the GYP fix.

targos added a commit to nodejs/gyp-next that referenced this pull request Apr 6, 2022
They are probably not paths, and V8 uses an argument like that, so the
fix is needed in Node.js

Refs: nodejs/node#42115
juanarbol pushed a commit that referenced this pull request Apr 6, 2022
Since v8 10.1 v8::External::New DCHECKs that the data passed
into it cannot be a nullptr because that's not serializable
as external references. This updates the test to pass a
dummy data pointer to the call - which does not matter for the
test since we only care about whether the finalizer is
called.

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/3513234

PR-URL: #42532
Refs: #42115
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@targos targos marked this pull request as draft April 8, 2022 15:14
targos added a commit to targos/node that referenced this pull request Apr 8, 2022
Original commit message:

    [ic] use CSA_DCHECK in CodeStubAssembler::SharedValueBarrier

    Since the code is generated unconditionally, using a DCHECK to check
    that shared RO heap is enabled breaks builds with
    v8_enable_shared_ro_heap set to false, this patch turns that into a
    CSA_DCHECK so it only crashes when V8 actually attempts to store into
    a shared struct while the RO heap isn't shared at run time.

    Refs: nodejs#42115
    Bug: v8:12547
    Change-Id: I30d9a02b98a0b647097125c0a9d141e40d6348cc
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3561598
    Reviewed-by: Shu-yu Guo <[email protected]>
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#79756}

Refs: v8/v8@87ce4f5
targos added a commit to targos/node that referenced this pull request Apr 9, 2022
Original commit message:

    [ic] use CSA_DCHECK in CodeStubAssembler::SharedValueBarrier

    Since the code is generated unconditionally, using a DCHECK to check
    that shared RO heap is enabled breaks builds with
    v8_enable_shared_ro_heap set to false, this patch turns that into a
    CSA_DCHECK so it only crashes when V8 actually attempts to store into
    a shared struct while the RO heap isn't shared at run time.

    Refs: nodejs#42115
    Bug: v8:12547
    Change-Id: I30d9a02b98a0b647097125c0a9d141e40d6348cc
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3561598
    Reviewed-by: Shu-yu Guo <[email protected]>
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#79756}

Refs: v8/v8@87ce4f5
targos added a commit to targos/node that referenced this pull request Apr 12, 2022
Original commit message:

    [ic] use CSA_DCHECK in CodeStubAssembler::SharedValueBarrier

    Since the code is generated unconditionally, using a DCHECK to check
    that shared RO heap is enabled breaks builds with
    v8_enable_shared_ro_heap set to false, this patch turns that into a
    CSA_DCHECK so it only crashes when V8 actually attempts to store into
    a shared struct while the RO heap isn't shared at run time.

    Refs: nodejs#42115
    Bug: v8:12547
    Change-Id: I30d9a02b98a0b647097125c0a9d141e40d6348cc
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3561598
    Reviewed-by: Shu-yu Guo <[email protected]>
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#79756}

Refs: v8/v8@87ce4f5
targos added a commit that referenced this pull request Apr 12, 2022
Original commit message:

    [ic] use CSA_DCHECK in CodeStubAssembler::SharedValueBarrier

    Since the code is generated unconditionally, using a DCHECK to check
    that shared RO heap is enabled breaks builds with
    v8_enable_shared_ro_heap set to false, this patch turns that into a
    CSA_DCHECK so it only crashes when V8 actually attempts to store into
    a shared struct while the RO heap isn't shared at run time.

    Refs: #42115
    Bug: v8:12547
    Change-Id: I30d9a02b98a0b647097125c0a9d141e40d6348cc
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3561598
    Reviewed-by: Shu-yu Guo <[email protected]>
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#79756}

Refs: v8/v8@87ce4f5

PR-URL: #42657
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
vmoroz pushed a commit to vmoroz/node that referenced this pull request Apr 13, 2022
Original commit message:

    [ic] use CSA_DCHECK in CodeStubAssembler::SharedValueBarrier

    Since the code is generated unconditionally, using a DCHECK to check
    that shared RO heap is enabled breaks builds with
    v8_enable_shared_ro_heap set to false, this patch turns that into a
    CSA_DCHECK so it only crashes when V8 actually attempts to store into
    a shared struct while the RO heap isn't shared at run time.

    Refs: nodejs#42115
    Bug: v8:12547
    Change-Id: I30d9a02b98a0b647097125c0a9d141e40d6348cc
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3561598
    Reviewed-by: Shu-yu Guo <[email protected]>
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#79756}

Refs: v8/v8@87ce4f5

PR-URL: nodejs#42657
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos
Copy link
Member Author

targos commented Apr 14, 2022

#42740

@targos targos closed this Apr 14, 2022
@targos targos deleted the v8-99 branch April 21, 2022 10:21
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
Since v8 10.1 v8::External::New DCHECKs that the data passed
into it cannot be a nullptr because that's not serializable
as external references. This updates the test to pass a
dummy data pointer to the call - which does not matter for the
test since we only care about whether the finalizer is
called.

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/3513234

PR-URL: nodejs#42532
Refs: nodejs#42115
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
Original commit message:

    [ic] use CSA_DCHECK in CodeStubAssembler::SharedValueBarrier

    Since the code is generated unconditionally, using a DCHECK to check
    that shared RO heap is enabled breaks builds with
    v8_enable_shared_ro_heap set to false, this patch turns that into a
    CSA_DCHECK so it only crashes when V8 actually attempts to store into
    a shared struct while the RO heap isn't shared at run time.

    Refs: nodejs#42115
    Bug: v8:12547
    Change-Id: I30d9a02b98a0b647097125c0a9d141e40d6348cc
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3561598
    Reviewed-by: Shu-yu Guo <[email protected]>
    Reviewed-by: Leszek Swirski <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#79756}

Refs: v8/v8@87ce4f5

PR-URL: nodejs#42657
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@joyeecheung
Copy link
Member

joyeecheung commented Apr 25, 2022

This seems to break --v8-non-optimized-debug, I got it to compile again with this (taken from this CL which is in 10.3: https://chromium-review.googlesource.com/c/v8/v8/+/3585943)

diff --git a/deps/v8/src/codegen/register.h b/deps/v8/src/codegen/register.h
index e36e4d1e9a..ef5f6dfcf7 100644
--- a/deps/v8/src/codegen/register.h
+++ b/deps/v8/src/codegen/register.h
@@ -26,7 +26,8 @@ template <typename... RegTypes,
               std::conjunction_v<std::is_same<Register, RegTypes>...> ||
               std::conjunction_v<std::is_same<DoubleRegister, RegTypes>...>>>
 inline constexpr bool AreAliased(RegTypes... regs) {
-  int num_different_regs = RegListBase{regs...}.Count();
+  using FirstRegType = std::tuple_element_t<0, std::tuple<RegTypes...>>;
+  int num_different_regs = RegListBase<FirstRegType>{regs...}.Count();
   int num_given_regs = (... + (regs.is_valid() ? 1 : 0));
   return num_different_regs < num_given_regs;
 }

kangwoosukeq pushed a commit to prosyslab/v8 that referenced this pull request Apr 28, 2022
Since the code is generated unconditionally, using a DCHECK to check
that shared RO heap is enabled breaks builds with
v8_enable_shared_ro_heap set to false, this patch turns that into a
CSA_DCHECK so it only crashes when V8 actually attempts to store into
a shared struct while the RO heap isn't shared at run time.

Refs: nodejs/node#42115
Bug: v8:12547
Change-Id: I30d9a02b98a0b647097125c0a9d141e40d6348cc
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3561598
Reviewed-by: Shu-yu Guo <[email protected]>
Reviewed-by: Leszek Swirski <[email protected]>
Commit-Queue: Joyee Cheung <[email protected]>
Cr-Commit-Position: refs/heads/main@{#79756}
juanarbol pushed a commit that referenced this pull request May 31, 2022
Since v8 10.1 v8::External::New DCHECKs that the data passed
into it cannot be a nullptr because that's not serializable
as external references. This updates the test to pass a
dummy data pointer to the call - which does not matter for the
test since we only care about whether the finalizer is
called.

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/3513234

PR-URL: #42532
Refs: #42115
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
Since v8 10.1 v8::External::New DCHECKs that the data passed
into it cannot be a nullptr because that's not serializable
as external references. This updates the test to pass a
dummy data pointer to the call - which does not matter for the
test since we only care about whether the finalizer is
called.

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/3513234

PR-URL: #42532
Refs: #42115
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 11, 2022
Since v8 10.1 v8::External::New DCHECKs that the data passed
into it cannot be a nullptr because that's not serializable
as external references. This updates the test to pass a
dummy data pointer to the call - which does not matter for the
test since we only care about whether the finalizer is
called.

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/3513234

PR-URL: #42532
Refs: #42115
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
Since v8 10.1 v8::External::New DCHECKs that the data passed
into it cannot be a nullptr because that's not serializable
as external references. This updates the test to pass a
dummy data pointer to the call - which does not matter for the
test since we only care about whether the finalizer is
called.

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/3513234

PR-URL: #42532
Refs: #42115
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Since v8 10.1 v8::External::New DCHECKs that the data passed
into it cannot be a nullptr because that's not serializable
as external references. This updates the test to pass a
dummy data pointer to the call - which does not matter for the
test since we only care about whether the finalizer is
called.

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/3513234

PR-URL: nodejs/node#42532
Refs: nodejs/node#42115
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants