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.

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 · 2024-05-05T08:55:15Z

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

targos · 2024-05-05T14:17:19Z

I'll land manually because node-test-linux-alpine-last-latest-x64 cannot be green (it's disabled now)

MoLow · 2024-05-05T14:29:09Z

I'll land manually because node-test-linux-alpine-last-latest-x64 cannot be green (it's disabled now)

https://ci.nodejs.org/view/All/job/post-build-status-update/ can be triggered manually

MoLow · 2024-05-05T14:31:31Z

https://ci.nodejs.org/view/All/job/post-build-status-update/2249452/

nodejs-github-bot · 2024-05-05T14:44:05Z

Landed in c7e4209

This is necessary to compile V8 with C++20 support. Refs: nodejs/node#45427

Closes: nodejs#45402 PR-URL: nodejs#45427 Fixes: nodejs#45402 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Steven R Loomis <srl295@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>

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

Closed

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

targos mentioned this pull request May 3, 2024

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

Merged

targos added the request-ci Add this label to start a Jenkins CI on a PR. label May 4, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 4, 2024

This comment was marked as outdated.

Sign in to view

This was referenced May 5, 2024

build, tools: copy release assets to staging R2 bucket once built #51394

Merged

[v22.x] build: compile with C++20 support #52838

Closed

MoLow approved these changes May 5, 2024

View reviewed changes

targos added the commit-queue Add this label to land a pull request using GitHub Actions. label May 5, 2024

UlisesGascon approved these changes May 5, 2024

View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 5, 2024

nodejs-github-bot merged commit c7e4209 into nodejs:main May 5, 2024
54 checks passed

targos deleted the c++20 branch May 5, 2024 14:44

targos added a commit to nodejs/unofficial-builds that referenced this pull request May 5, 2024

chore: update Alpine to 3.19

7247235

This is necessary to compile V8 with C++20 support. Refs: nodejs/node#45427

targos mentioned this pull request May 5, 2024

chore: update Alpine to 3.19 nodejs/unofficial-builds#144

Merged

rvagg pushed a commit to nodejs/unofficial-builds that referenced this pull request May 6, 2024

chore: update Alpine to 3.19

d8eaee5

This is necessary to compile V8 with C++20 support. Refs: nodejs/node#45427

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

lemire commented May 3, 2024

richardlau commented May 3, 2024

lemire commented May 3, 2024

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented May 5, 2024 •

edited by targos

targos commented May 5, 2024

MoLow commented May 5, 2024

MoLow commented May 5, 2024

nodejs-github-bot commented May 5, 2024

build: compile with C++20 support #45427

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

lemire commented May 3, 2024

richardlau commented May 3, 2024

lemire commented May 3, 2024

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented May 5, 2024 • edited by targos

targos commented May 5, 2024

MoLow commented May 5, 2024

MoLow commented May 5, 2024

nodejs-github-bot commented May 5, 2024

joyeecheung commented Nov 12, 2022 •

edited

gengjiawen commented Nov 14, 2022 •

edited

nodejs-github-bot commented May 5, 2024 •

edited by targos