build: compile with C++20 support #45427

targos · 2022-11-11T12:30:34Z

nodejs-github-bot · 2022-11-11T12:30:38Z

Review requested:

@nodejs/gyp

targos · 2022-11-11T12:31:12Z

I see a lot of [-Wambiguous-reversed-operator] in ICU. For example:

[308/3813] CXX obj/deps/icu-small/source/i18n/icui18n.pluralranges.o
In file included from ../../deps/icu-small/source/i18n/pluralranges.cpp:18:
In file included from ../../deps/icu-small/source/i18n/numrange_impl.h:15:
In file included from ../../deps/icu-small/source/i18n/number_formatimpl.h:14:
../../deps/icu-small/source/i18n/number_utils.h:53:17: warning: ISO C++20 considers use of overloaded operator '==' (with operand types 'const icu_72::MeasureUnit' and 'icu_72::MeasureUnit') to be ambiguous despite there being a unique best viable function [-Wambiguous-reversed-operator]
    return unit == MeasureUnit();
           ~~~~ ^  ~~~~~~~~~~~~~
../../deps/icu-small/source/i18n/unicode/measunit.h:435:18: note: ambiguity is between a regular call to this operator and a call with the argument order reversed
    virtual bool operator==(const UObject& other) const;
                 ^
1 warning generated.

targos · 2022-11-11T12:31:18Z

I think V8 built fine. There's an error in env.cc:

FAILED: obj/src/libnode.env.o
c++ -MMD -MF obj/src/libnode.env.o.d -DV8_DEPRECATION_WARNINGS -DV8_IMMINENT_DEPRECATION_WARNINGS -D_GLIBCXX_USE_CXX11_ABI=1 -DNODE_OPENSSL_CONF_NAME=nodejs_conf -DNODE_OPENSSL_HAS_QUIC -D_DARWIN_USE_64_BIT_INODE=1 -DOPENSSL_NO_PINSHARED -DOPENSSL_THREADS '-DNODE_ARCH="arm64"' -DNODE_WANT_INTERNALS=1 -DV8_DEPRECATION_WARNINGS=1 '-DNODE_OPENSSL_SYSTEM_CERT_PATH=""' -DNODE_USE_NODE_CODE_CACHE=1 -DHAVE_INSPECTOR=1 -D__POSIX__ -DNODE_USE_V8_PLATFORM=1 -DNODE_HAVE_I18N_SUPPORT=1 '-DNODE_PLATFORM="darwin"' -DHAVE_OPENSSL=1 -DOPENSSL_API_COMPAT=0x10100000L -DBASE64_STATIC_DEFINE -DUCONFIG_NO_SERVICE=1 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION=1 -DU_HAVE_STD_STRING=1 -DUCONFIG_NO_BREAK_ITERATION=0 -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -DNGHTTP2_STATICLIB -DNDEBUG -DL_ENDIAN -DOPENSSL_BUILDING_OPENSSL -DECP_NISTZ256_ASM -DKECCAK1600_ASM -DOPENSSL_BN_ASM_MONT -DOPENSSL_CPUID_OBJ -DPOLY1305_ASM -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DVPAES_ASM -DOPENSSL_PIC -DNGTCP2_STATICLIB -DNGHTTP3_STATICLIB -I../../src -Igen -Igen/include -Igen/src -I../../deps/base64/base64/include -I../../deps/googletest/include -I../../deps/histogram/src -I../../deps/uvwasi/include -I../../deps/v8/include -I../../deps/icu-small/source/i18n -I../../deps/icu-small/source/common -I../../deps/zlib -I../../deps/llhttp/include -I../../deps/cares/include -I../../deps/uv/include -I../../deps/nghttp2/lib/includes -I../../deps/brotli/c/include -I../../deps/openssl/openssl/include -I../../deps/openssl/openssl/crypto/include -I../../deps/openssl/config/archs/darwin64-arm64-cc/asm_avx2/include -I../../deps/openssl/config/archs/darwin64-arm64-cc/asm_avx2 -I../../deps/ngtcp2 -I../../deps/ngtcp2/ngtcp2/lib/includes -I../../deps/ngtcp2/ngtcp2/crypto/includes -I../../deps/ngtcp2/ngtcp2/crypto -I../../deps/ngtcp2/nghttp3/lib/includes -O3 -gdwarf-2 -mmacosx-version-min=10.15 -arch arm64 -Wall -Wendif-labels -W -Wno-unused-parameter -Werror=undefined-inline -Wall -Wendif-labels -W -Wno-unused-parameter -Werror -std=gnu++20 -stdlib=libc++ -fno-rtti -fno-exceptions -fno-strict-aliasing  -c ../../src/env.cc -o obj/src/libnode.env.o
../../src/env.cc:1630:22: error: no matching constructor for initialization of 'node::DeserializeRequest'
  DeserializeRequest request{cb, {isolate(), holder}, index, info};
                     ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/env.h:504:3: note: candidate constructor not viable: requires single argument 'other', but 4 arguments were provided
  DeserializeRequest(DeserializeRequest&& other) = default;
  ^
../../src/env.h:497:8: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 4 were provided
struct DeserializeRequest {
       ^
1 error generated.
[3657/3813] CXX obj/gen/libnode.node_javascript.o

Any suggestions?

targos · 2022-11-11T15:51:30Z

I see a lot of [-Wambiguous-reversed-operator] in ICU.

If I understand https://unicode-org.atlassian.net/browse/ICU-22014 correctly, we need to build ICU with -Wno-ambiguous-reversed-operator.

gengjiawen · 2022-11-12T04:58:35Z

I think V8 built fine. There's an error in env.cc:

FAILED: obj/src/libnode.env.o
c++ -MMD -MF obj/src/libnode.env.o.d -DV8_DEPRECATION_WARNINGS -DV8_IMMINENT_DEPRECATION_WARNINGS -D_GLIBCXX_USE_CXX11_ABI=1 -DNODE_OPENSSL_CONF_NAME=nodejs_conf -DNODE_OPENSSL_HAS_QUIC -D_DARWIN_USE_64_BIT_INODE=1 -DOPENSSL_NO_PINSHARED -DOPENSSL_THREADS '-DNODE_ARCH="arm64"' -DNODE_WANT_INTERNALS=1 -DV8_DEPRECATION_WARNINGS=1 '-DNODE_OPENSSL_SYSTEM_CERT_PATH=""' -DNODE_USE_NODE_CODE_CACHE=1 -DHAVE_INSPECTOR=1 -D__POSIX__ -DNODE_USE_V8_PLATFORM=1 -DNODE_HAVE_I18N_SUPPORT=1 '-DNODE_PLATFORM="darwin"' -DHAVE_OPENSSL=1 -DOPENSSL_API_COMPAT=0x10100000L -DBASE64_STATIC_DEFINE -DUCONFIG_NO_SERVICE=1 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION=1 -DU_HAVE_STD_STRING=1 -DUCONFIG_NO_BREAK_ITERATION=0 -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -DNGHTTP2_STATICLIB -DNDEBUG -DL_ENDIAN -DOPENSSL_BUILDING_OPENSSL -DECP_NISTZ256_ASM -DKECCAK1600_ASM -DOPENSSL_BN_ASM_MONT -DOPENSSL_CPUID_OBJ -DPOLY1305_ASM -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DVPAES_ASM -DOPENSSL_PIC -DNGTCP2_STATICLIB -DNGHTTP3_STATICLIB -I../../src -Igen -Igen/include -Igen/src -I../../deps/base64/base64/include -I../../deps/googletest/include -I../../deps/histogram/src -I../../deps/uvwasi/include -I../../deps/v8/include -I../../deps/icu-small/source/i18n -I../../deps/icu-small/source/common -I../../deps/zlib -I../../deps/llhttp/include -I../../deps/cares/include -I../../deps/uv/include -I../../deps/nghttp2/lib/includes -I../../deps/brotli/c/include -I../../deps/openssl/openssl/include -I../../deps/openssl/openssl/crypto/include -I../../deps/openssl/config/archs/darwin64-arm64-cc/asm_avx2/include -I../../deps/openssl/config/archs/darwin64-arm64-cc/asm_avx2 -I../../deps/ngtcp2 -I../../deps/ngtcp2/ngtcp2/lib/includes -I../../deps/ngtcp2/ngtcp2/crypto/includes -I../../deps/ngtcp2/ngtcp2/crypto -I../../deps/ngtcp2/nghttp3/lib/includes -O3 -gdwarf-2 -mmacosx-version-min=10.15 -arch arm64 -Wall -Wendif-labels -W -Wno-unused-parameter -Werror=undefined-inline -Wall -Wendif-labels -W -Wno-unused-parameter -Werror -std=gnu++20 -stdlib=libc++ -fno-rtti -fno-exceptions -fno-strict-aliasing  -c ../../src/env.cc -o obj/src/libnode.env.o
../../src/env.cc:1630:22: error: no matching constructor for initialization of 'node::DeserializeRequest'
  DeserializeRequest request{cb, {isolate(), holder}, index, info};
                     ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/env.h:504:3: note: candidate constructor not viable: requires single argument 'other', but 4 arguments were provided
  DeserializeRequest(DeserializeRequest&& other) = default;
  ^
../../src/env.h:497:8: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 4 were provided
struct DeserializeRequest {
       ^
1 error generated.
[3657/3813] CXX obj/gen/libnode.node_javascript.o

Any suggestions?

I pushed a commit try to fix, not verified

gengjiawen · 2022-11-12T05:09:55Z

gen/node_snapshot.cc:620:16: error: no matching constructor for initialization of 'node::SnapshotData'
};SnapshotData snapshot_data {
               ^             ~
../../src/env.h:574:3: note: candidate constructor not viable: requires 1 argument, but 6 were provided
  SnapshotData(const SnapshotData&) = delete;

Looks similar problem to DeserializeRequest error.
cc @joyeecheung

joyeecheung · 2022-11-12T12:08:06Z

hmm, what if we use designated initializers? e.g. instead of DeserializeRequest request{cb, {isolate(), holder}, index, info}, we do DeserializeRequest request{.cb = cb, .holder = v8::Global<v8::Object>(isolate(), holder), .index = index, .info = info}?

nodejs-github-bot · 2022-11-12T12:50:40Z

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

targos · 2022-11-12T16:34:31Z

GCC 8 and 9 expect -std=gnu++2a

joyeecheung · 2022-11-12T16:44:11Z

I found the cause: https://www.open-std.org/Jtc1/sc22/wg21/docs/papers/2018/p1008r1.pdf, we need to remove all the constructors of the aggregate type, even the = default and = delete ones (which are the only ones we have). This compiles for me locally with c++20

diff --git a/src/env.h b/src/env.h
index 3d44e0acbd..8fbd48d58d 100644
--- a/src/env.h
+++ b/src/env.h
@@ -499,9 +499,6 @@ struct DeserializeRequest {
   v8::Global<v8::Object> holder;
   int index;
   InternalFieldInfoBase* info = nullptr;  // Owned by the request
-
-  // Move constructor
-  DeserializeRequest(DeserializeRequest&& other) = default;
 };

 struct EnvSerializeInfo {
@@ -565,13 +562,6 @@ struct SnapshotData {
   static bool FromBlob(SnapshotData* out, FILE* in);

   ~SnapshotData();
-
-  SnapshotData(const SnapshotData&) = delete;
-  SnapshotData& operator=(const SnapshotData&) = delete;
-  SnapshotData(SnapshotData&&) = delete;
-  SnapshotData& operator=(SnapshotData&&) = delete;
-
-  SnapshotData() = default;
 };

 void DefaultProcessExitHandlerInternal(Environment* env, ExitCode exit_code);

gengjiawen · 2022-11-13T09:17:45Z

Most platforms build passed. MSVC comes no surprise, v8 failed on MSVC.

15:01:24 C:\workspace\node-compile-windows\node\deps\v8\src\handles\handles.h(143,37): error C2027: use of undefined type 'v8::internal::Object' [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]

targos · 2022-11-14T08:46:57Z

Most platforms build passed. MSVC comes no surprise, v8 failed on MSVC.

15:01:24 C:\workspace\node-compile-windows\node\deps\v8\src\handles\handles.h(143,37): error C2027: use of undefined type 'v8::internal::Object' [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]

See https://bugs.chromium.org/p/chromium/issues/detail?id=1377771#c4

This is fixed in recent versions of MSVC.

gengjiawen · 2022-11-14T13:07:42Z

Most platforms build passed. MSVC comes no surprise, v8 failed on MSVC.
15:01:24 C:\workspace\node-compile-windows\node\deps\v8\src\handles\handles.h(143,37): error C2027: use of undefined type 'v8::internal::Object' [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]
See bugs.chromium.org/p/chromium/issues/detail?id=1377771#c4

This is fixed in recent versions of MSVC.

So when latest MSVC released, we only have issues on freebsd and alpine. Run into less issues than I think. But I still want gcc-10 as a minimum on next major.

targos · 2024-03-14T10:18:52Z

The GCC issue may be fixed by #52083

targos · 2024-03-15T06:56:44Z

Added #52083 to try a new CI run.

nodejs-github-bot · 2024-03-15T06:58:03Z

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

nodejs-github-bot · 2024-03-15T12:53:02Z

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

nodejs-github-bot · 2024-03-16T10:53:15Z

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

targos · 2024-03-16T16:57:08Z

Unfortunately, we're still blocked on the is_trivially_copyable error, which also happens on Debian 12!

V8 and Chromium are starting to use C++20 features. Only Visual Studio 2022 17.6 includes a version of MSVC with sufficient C++20 support to compile V8. Refs: https://bugs.chromium.org/p/chromium/issues/detail?id=1284275 Refs: nodejs#45427 PR-URL: nodejs#49051 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Ruy Adorno <ruyadorno@google.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Closes: nodejs#45402

nodejs-github-bot · 2024-05-03T12:28:18Z

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

targos · 2024-05-03T14:58:00Z

Here's a repro of the compiler error: godbolt link

gcc 12.2 with gnu++17: pass
gcc 12.2 with gnu++20: fail
gcc 12.3 with gnu++20: pass

Edit: forgot about #45427 (comment), which shows the same.

Required to enable C++20 support. Refs: nodejs/node#45427

Refs: nodejs/node#45427

targos · 2024-05-03T15:28:09Z

I opened nodejs/build#3700 to skip Alpine 3.18 and I'm working on downgrading GCC to version 11 on Debian 12.
Hopefully, it will unlock this PR!

Refs: nodejs/node#45427

Required to enable C++20 support. Refs: nodejs/node#45427

anonrig

🫡

lemire · 2024-05-03T15:42:13Z

gcc 12.2 with gnu++17: pass
gcc 12.2 with gnu++20: fail
gcc 12.3 with gnu++20: pass

For people reading this: I strongly suspect that the problem is not with V8 or Node.js, but rather a compiler/runtime library issue. That is, it is warranted to 'force' the issue.

richardlau · 2024-05-03T18:13:26Z

Here's a repro of the compiler error: godbolt link
* gcc 12.2 with gnu++17: pass

* gcc 12.2 with gnu++20: fail

* gcc 12.3 with gnu++20: pass
Edit: forgot about #45427 (comment), which shows the same.

Interestingly https://ci.nodejs.org/job/node-test-commit-linuxone/43448/nodes=rhel8-s390x/ succeeded with gcc 12.2.1 (gcc-toolset-12 on RHEL 8).

lemire · 2024-05-03T18:45:47Z

@richardlau Yes. It is probably not a compiler bug per se... hence "compiler/runtime library issue".

nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels Nov 11, 2022

targos mentioned this pull request Nov 11, 2022

Enable C++20 #45402

Open

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2022

targos added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2022

This comment was marked as outdated.

Sign in to view

github-actions bot mentioned this pull request Nov 13, 2022

CI Reliability 2022-11-13 nodejs/reliability#427

Open

25 tasks

gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 13, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 13, 2022

This comment was marked as outdated.

Sign in to view

github-actions bot mentioned this pull request Nov 14, 2022

CI Reliability 2022-11-14 nodejs/reliability#428

Open

25 tasks

github-actions bot mentioned this pull request Nov 15, 2022

CI Reliability 2022-11-15 nodejs/reliability#429

Open

23 tasks

gengjiawen mentioned this pull request Nov 15, 2022

Reduce the number of patches we float on V8 #45118

Open

7 tasks

github-actions bot mentioned this pull request Nov 16, 2022

CI Reliability 2022-11-16 nodejs/reliability#430

Open

17 tasks

richardlau mentioned this pull request Mar 12, 2024

Update compiler on Linux s390x nodejs/build#3630

Open

targos mentioned this pull request Mar 13, 2024

build: wasm-to-js-tq-csa.cc takes ages to compile #52068

Closed

targos force-pushed the c++20 branch from b5d4cca to 62275a3 Compare March 15, 2024 06:56

StefanStojanovic mentioned this pull request Apr 19, 2024

deps: update V8 to 12.4 #52465

Closed

targos mentioned this pull request Apr 19, 2024

build: compile with C++20 support on Windows #52594

Closed

richardlau mentioned this pull request Apr 25, 2024

V8 CI build error on ppc #52661

Closed

build: compile with C++20 support

43a222a

Closes: nodejs#45402

targos force-pushed the c++20 branch from 62275a3 to 43a222a Compare May 3, 2024 12:06

targos added a commit to targos/nodejs-build that referenced this pull request May 3, 2024

jenkins: skip Alpine 3.18 on Node.js >=23

80d62a2

Required to enable C++20 support. Refs: nodejs/node#45427

targos mentioned this pull request May 3, 2024

jenkins: skip Alpine 3.18 on Node.js >=23 nodejs/build#3700

Merged

targos added a commit to targos/nodejs-build that referenced this pull request May 3, 2024

ansible: install GCC 11 on Debian 12

52c994d

Refs: nodejs/node#45427

targos mentioned this pull request May 3, 2024

ansible: install GCC 11 on Debian 12 nodejs/build#3701

Merged

targos added a commit to nodejs/build that referenced this pull request May 3, 2024

ansible: install GCC 11 on Debian 12 (#3701)

98917df

Refs: nodejs/node#45427

targos added a commit to nodejs/build that referenced this pull request May 3, 2024

jenkins: skip Alpine 3.18 on Node.js >=23 (#3700)

238989f

Required to enable C++20 support. Refs: nodejs/node#45427

targos mentioned this pull request May 3, 2024

build: compile with C++20 support when using Clang #52819

Open

anonrig approved these changes May 3, 2024

View reviewed changes

targos mentioned this pull request May 3, 2024

jenkins: select GCC 11 on Debian 12 nodejs/build#3702

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: compile with C++20 support #45427

build: compile with C++20 support #45427

targos commented Nov 11, 2022

nodejs-github-bot commented Nov 11, 2022

targos commented Nov 11, 2022

targos commented Nov 11, 2022

targos commented Nov 11, 2022

gengjiawen commented Nov 12, 2022

gengjiawen commented Nov 12, 2022

joyeecheung commented Nov 12, 2022 •

edited

nodejs-github-bot commented Nov 12, 2022

targos commented Nov 12, 2022

joyeecheung commented Nov 12, 2022

This comment was marked as outdated.

This comment was marked as outdated.

gengjiawen commented Nov 13, 2022

targos commented Nov 14, 2022

gengjiawen commented Nov 14, 2022 •

edited

targos commented Mar 14, 2024

targos commented Mar 15, 2024

nodejs-github-bot commented Mar 15, 2024

nodejs-github-bot commented Mar 15, 2024

nodejs-github-bot commented Mar 16, 2024

targos commented Mar 16, 2024 •

edited

nodejs-github-bot commented May 3, 2024

targos commented May 3, 2024 •

edited

targos commented May 3, 2024

anonrig left a comment

lemire commented May 3, 2024

richardlau commented May 3, 2024

lemire commented May 3, 2024

build: compile with C++20 support #45427

Are you sure you want to change the base?

build: compile with C++20 support #45427

Conversation

targos commented Nov 11, 2022

nodejs-github-bot commented Nov 11, 2022

targos commented Nov 11, 2022

targos commented Nov 11, 2022

targos commented Nov 11, 2022

gengjiawen commented Nov 12, 2022

gengjiawen commented Nov 12, 2022

joyeecheung commented Nov 12, 2022 • edited

nodejs-github-bot commented Nov 12, 2022

targos commented Nov 12, 2022

joyeecheung commented Nov 12, 2022

This comment was marked as outdated.

This comment was marked as outdated.

gengjiawen commented Nov 13, 2022

targos commented Nov 14, 2022

gengjiawen commented Nov 14, 2022 • edited

targos commented Mar 14, 2024

targos commented Mar 15, 2024

nodejs-github-bot commented Mar 15, 2024

nodejs-github-bot commented Mar 15, 2024

nodejs-github-bot commented Mar 16, 2024

targos commented Mar 16, 2024 • edited

nodejs-github-bot commented May 3, 2024

targos commented May 3, 2024 • edited

targos commented May 3, 2024

anonrig left a comment

Choose a reason for hiding this comment

lemire commented May 3, 2024

richardlau commented May 3, 2024

lemire commented May 3, 2024

joyeecheung commented Nov 12, 2022 •

edited

gengjiawen commented Nov 14, 2022 •

edited

targos commented Mar 16, 2024 •

edited

targos commented May 3, 2024 •

edited