node icon indicating copy to clipboard operation
node copied to clipboard

build: compile with C++20 support

Open targos opened this issue 3 years ago • 47 comments

Closes: https://github.com/nodejs/node/issues/45402

targos avatar Nov 11 '22 12:11 targos

Review requested:

  • [ ] @nodejs/gyp

nodejs-github-bot avatar Nov 11 '22 12:11 nodejs-github-bot

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 avatar Nov 11 '22 12:11 targos

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 avatar Nov 11 '22 12:11 targos

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.

targos avatar Nov 11 '22 15:11 targos

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 avatar Nov 12 '22 04:11 gengjiawen

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

gengjiawen avatar Nov 12 '22 05:11 gengjiawen

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}?

joyeecheung avatar Nov 12 '22 12:11 joyeecheung

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

nodejs-github-bot avatar Nov 12 '22 12:11 nodejs-github-bot

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

targos avatar Nov 12 '22 16:11 targos

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);

joyeecheung avatar Nov 12 '22 16:11 joyeecheung

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

nodejs-github-bot avatar Nov 12 '22 16:11 nodejs-github-bot

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

nodejs-github-bot avatar Nov 13 '22 06:11 nodejs-github-bot

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]

gengjiawen avatar Nov 13 '22 09:11 gengjiawen

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.

targos avatar Nov 14 '22 08:11 targos

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.

gengjiawen avatar Nov 14 '22 13:11 gengjiawen

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

nodejs-github-bot avatar Mar 30 '23 08:03 nodejs-github-bot

Windows build failure on both vs2019 and vs2022:

D:\a\node\node\src\node_threadsafe_cow-inl.h(10,14): error C2039: 'unique': is not a member of 'std::shared_ptr<T>' [D:\a\node\node\libnode.vcxproj]

@nodejs/cpp-reviewers

On vs2019 only:

D:\a\node\node\deps\v8\src\handles\handles.h(145,37): error C2027: use of undefined type 'v8::internal::Object' [D:\a\node\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]

I suggest to try again with a more recent V8 for this one

targos avatar Mar 30 '23 09:03 targos

Interesting new MSVC error:

D:\a\node\node\deps\v8\src\compiler\turboshaft\optimization-phase.h(25): fatal  error C1075: '{': no matching token found [D:\a\node\node\tools\v8_gypfiles\v8_compiler.vcxproj]

targos avatar Mar 31 '23 15:03 targos

Ha, msvc gets confused by the namespace foo::bar { syntax. That's a C++17-ism.

You can fix it by breaking it up into namespace foo { namespace bar { (and make sure you fix up the closing braces too) but V8 is using that syntax in a lot of places now...

bnoordhuis avatar Mar 31 '23 18:03 bnoordhuis

Ha, msvc gets confused by the namespace foo::bar { syntax. That's a C++17-ism.

You can fix it by breaking it up into namespace foo { namespace bar { (and make sure you fix up the closing braces too) but V8 is using that syntax in a lot of places now...

https://godbolt.org/z/e1fPba7hf Looks like MSVC supported it, not sure it's a MSVC bug or something else.

Given to https://github.com/nodejs/node/issues/43092#issuecomment-1492920069, I don't hold much hope.

But anyway upstream bugreport: https://developercommunity.visualstudio.com/t/cpp20-nested-namespace-cant-compile/10328893

gengjiawen avatar Apr 01 '23 10:04 gengjiawen

Ha, msvc gets confused by the namespace foo::bar { syntax. That's a C++17-ism.

You can fix it by breaking it up into namespace foo { namespace bar { (and make sure you fix up the closing braces too) but V8 is using that syntax in a lot of places now...

That doesn't fix it. I think it's something inside the namespace that confuses MSVC. I opened the file in Visual Studio but it doesn't show me any error in the code editor.

targos avatar Apr 01 '23 11:04 targos

~This is related to precompiled headers. If I remove v8_pch.h from the arguments passed to cl.exe, the error disappears.~ edit: I'm not sure anymore...

targos avatar Apr 01 '23 12:04 targos

I was able to reduce a reproduction to one preprocessed file, but it's big (14 MiB, too big for godbolt):

PS D:\Git\nodejs\node\tools\v8_gypfiles> & 'C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.35.32215\bin\HostX64\x64\CL.exe' /c /Z7 /nologo /W3 /WX- /diagnostics:column /MP /O2 /Ob2 /Oi /Oy /GF /Gm- /MT /GS /Gy /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /GR- /Fo"pipeline.obj" /external:W3 /Gd /TP /wd4351 /wd4355 /wd4800 /wd4251 /wd4275 /wd4244 /wd4267 /wd4129 /wd4245 /wd4324 /wd4506 /wd4661 /wd4701 /wd4702 /wd4703 /wd4709 /wd4714 /wd4715 /wd4718 /wd4723 /wd4724 /FC /errorReport:prompt /utf-8 /Zc:__cplusplus -std:c++20 pipeline.i
pipeline.i
D:\Git\nodejs\node\tools\v8_gypfiles\pipeline.i(111666,31): warning C4146: opérateur moins unaire appliqué à un type non signé, le résultat sera non signé
D:\Git\nodejs\node\tools\v8_gypfiles\pipeline.i(454888,17): warning C4996: 'std::atomic_load': warning STL4029: std::atomic_*() overloads for shared_ptr are deprecated in C++20. The shared_ptr specialization of std::atomic provides superior functionality. You can define _SILENCE_CXX20_OLD_SHARED_PTR_ATOMIC_SUPPORT_DEPRECATION_WARNING or _SILENCE_ALL_CXX20_DEPRECATION_WARNINGS to suppress this warning.
D:\Git\nodejs\node\tools\v8_gypfiles\pipeline.i(454934,28): warning C4996: 'std::atomic_load': warning STL4029: std::atomic_*() overloads for shared_ptr are deprecated in C++20. The shared_ptr specialization of std::atomic provides superior functionality. You can define _SILENCE_CXX20_OLD_SHARED_PTR_ATOMIC_SUPPORT_DEPRECATION_WARNING or _SILENCE_ALL_CXX20_DEPRECATION_WARNINGS to suppress this warning.
D:\Git\nodejs\node\tools\v8_gypfiles\pipeline.i(532263): fatal error C1075: '{' : jeton correspondant introuvable

pipeline.zip

Happy to look at anything if you have suggestions.

targos avatar Apr 01 '23 13:04 targos

Another version of the file without the empty lines:

> & 'C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.35.32215\bin\HostX64\x64\CL.exe' /c /Z7 /nologo /W3 /WX- /diagnostics:column /MP /O2 /Ob2 /Oi /Oy /GF /Gm- /MT /GS /Gy /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline /GR- /Fo"pipeline.obj" /external:W3 /Gd /TP /wd4351 /wd4355 /wd4800 /wd4251 /wd4275 /wd4244 /wd4267 /wd4129 /wd4245 /wd4324 /wd4506 /wd4661 /wd4701 /wd4702 /wd4703 /wd4709 /wd4714 /wd4715 /wd4718 /wd4723 /wd4724 /FC /errorReport:prompt /utf-8 /Zc:__cplusplus -std:c++20 pipeline.i
pipeline.i
D:\Git\nodejs\node\tools\v8_gypfiles\pipeline.i(57686,31): warning C4146: opérateur moins unaire appliqué à un type non signé, le résultat sera non signé
D:\Git\nodejs\node\tools\v8_gypfiles\pipeline.i(156616,17): warning C4996: 'std::atomic_load': warning STL4029: std::atomic_*() overloads for shared_ptr are deprecated in C++20. The shared_ptr specialization of std::atomic provides superior functionality. You can define _SILENCE_CXX20_OLD_SHARED_PTR_ATOMIC_SUPPORT_DEPRECATION_WARNING or _SILENCE_ALL_CXX20_DEPRECATION_WARNINGS to suppress this warning.
D:\Git\nodejs\node\tools\v8_gypfiles\pipeline.i(156654,28): warning C4996: 'std::atomic_load': warning STL4029: std::atomic_*() overloads for shared_ptr are deprecated in C++20. The shared_ptr specialization of std::atomic provides superior functionality. You can define _SILENCE_CXX20_OLD_SHARED_PTR_ATOMIC_SUPPORT_DEPRECATION_WARNING or _SILENCE_ALL_CXX20_DEPRECATION_WARNINGS to suppress this warning.
D:\Git\nodejs\node\tools\v8_gypfiles\pipeline.i(196094): fatal error C1075: '{' : jeton correspondant introuvable

pipeline.zip

targos avatar Apr 01 '23 13:04 targos

The file looks well-formed to me. 🤷

bnoordhuis avatar Apr 01 '23 18:04 bnoordhuis

Issue should be fixed in the next minor version of VS2022: https://developercommunity.visualstudio.com/t/cpp20-nested-namespace-cant-compile/10328893#T-ND10330571

targos avatar Apr 05 '23 09:04 targos

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

nodejs-github-bot avatar Apr 17 '23 09:04 nodejs-github-bot

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

nodejs-github-bot avatar Apr 20 '23 12:04 nodejs-github-bot

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

nodejs-github-bot avatar May 05 '23 14:05 nodejs-github-bot

Issue should be fixed in the next minor version of VS2022: developercommunity.visualstudio.com/t/cpp20-nested-namespace-cant-compile/10328893#T-ND10330571

17.6 is released, but not on github actions yet.

gengjiawen avatar May 17 '23 13:05 gengjiawen