deps: V8: add explicit constructor to CppHeapCreateParams

[targos]

Refs: #45402
Refs: #45118

[nodejs-github-bot]

Review requested:

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

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/48262/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu1804-intel-64,v8test=v8test/5026/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5026/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5026/

[targos]

The Windows problem still exists, so we can't revert the change without fixing it another way:

C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\xutility(4096,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 [D:\a\node\node\tools\v8_gypfiles\v8_init.vcxproj]

See above.

[targos]

@dharesign would you be able to help finding and/or fixing the root cause?

[dharesign]

OK, that's very strange indeed. Looking at the logs, the first error you get is:

C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\xutility(4096,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
...
D:\a\node\node\deps\v8\include\cppgc\heap.h(127): 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

That's this vector in HeapOptions. That's not CppHeapCreateParams. I don't even see CppHeapCreateParams in the error log at all.

[dharesign]

Based on the objects being compiled, I would say it's coming from v8_pch.cc (here: https://github.com/targos/node/blob/revert-cppgc/tools/msvs/pch/v8_pch.h). Certainly, that ends up including the above linked cppgc/heap.h file:

. ./src/api/api-inl.h
.. ./src/heap/heap-inl.h
... ./src/heap/concurrent-allocator-inl.h
.... ./src/heap/incremental-marking.h
..... ./src/heap/mark-compact.h
...... ./src/heap/concurrent-marking.h
....... ./src/heap/marking-visitor.h
........ ./src/heap/marking-worklist.h
......... ./src/heap/cppgc-js/cpp-marking-state.h
.......... ./src/heap/cppgc-js/cpp-heap.h
........... ./src/heap/cppgc/heap-base.h
............ ./include/cppgc/heap.h

I haven't used pre-compiled headers, so I don't know if this kind of breakage is par-for-the-course.

Maybe to test that theory out, you could modify v8_pch.h to be empty?

[gengjiawen]

@targos Maybe use this patch: https://chromium-review.googlesource.com/c/v8/v8/+/3533019/2/include/v8-cppgc.h#78

[dharesign]

@targos Maybe use this patch: https://chromium-review.googlesource.com/c/v8/v8/+/3533019/2/include/v8-cppgc.h#78

I think it would be best to have no patch :) I can try building Node locally on a Windows machine next week and see if I can figure out the issue.

[gengjiawen]

I think it would be best to have no patch :)

Yeap. but without any patch MSVC just not works on building v8. Upload patches to upstream v8 is always the right way to do (it stuck because MSVC not ready on cpp20).

Also heads up, build Node.js or V8 in cpp20 on MSVC is still not possible, MSVC still fixing all their bugs.

[dharesign]

Sorry, I guess I should have commented here rather than #45402. Copying my comments here.

---

I figured out why it's breaking: it's because the CppHeapCreateParams struct is dllexport, which seems to cause it to fully instantiate the vector<> class, including parts that don't work.

Can reproduce: https://godbolt.org/z/8Wah9KbP3

Note that the error points to line 9 as the place the class is being instantiated, even though it's line 16 that's really causing it.

---

Relevant: https://devblogs.microsoft.com/oldnewthing/20190927-00/?p=102932

---

So unique_ptr<T> is not is_copy_constructible, but vector<unique_ptr<T>> is. It seems that this is a deliberate design choice that enables vector to be instantiated with incomplete types.

The only way to then export such a non-copyable class is to have the copy constructors explicitly deleted.

Doing that breaks the C++20 aggregate initialization per https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1008r1.pdf

I guess the most reasonable way forward is probably to give CppHeapCreateParams real constructors and not rely on aggregate initialization.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/48779/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu1804-intel-64,v8test=v8test/5086/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5086/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5086/

[gengjiawen]

@targos Maybe use this patch: chromium-review.googlesource.com/c/v8/v8/+/3533019/2/include/v8-cppgc.h#78

@targos upstream is merged.

[targos]

I'm actually just going to close this. We will inherit the fix with a future V8 update. Thanks for upstreaming this @gengjiawen !