GeezerVison

Review requested:

• @nodejs/gyp

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.

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 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.

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

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

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

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

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

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

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]

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.

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.

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

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

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

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

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

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

Landed in c7e4209