search

nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.13k stars 29.36k forks source link

'use strict' is ignored on mips64el/riscv64 when compiled with gcc 12 #44126

Open kapouer opened 2 years ago

kapouer commented 2 years ago

Version

18.6.0

Platform

SMP Debian 5.10.103-1 (2022-03-07) mips64 GNU/Linux

Subsystem

build

What steps will reproduce the bug?

Build node 18.6.0 with gcc 11.2.0 and 12.1.0 on mips64el (or riscv64 too), and compare results.

How often does it reproduce? Is there a required condition?

Always. Only gcc change between the two builds.

What is the expected behavior?

Same tests pass.

What do you see instead?

Several tests fail: parallel/test-crypto-sign-verify parallel/test-freeze-intrinsics parallel/test-tls-root-certificates parallel/test-vm-global-non-writable-properties parallel/test-vm-module-synthetic parallel/test-vm-strict-assign

Additional information

Attached are the log files.

gcc11.log.gz gcc12.log.gz

Note several things:

There are some new warnings

In file included from /usr/include/c++/12/atomic:41,
                 from ../deps/v8/src/base/atomic-utils.h:10,
                 from ../deps/v8/src/common/globals.h:15,
                 from ../deps/v8/src/builtins/builtins.h:10,
                 from ../deps/v8/src/builtins/builtins-utils.h:9,
                 from ../deps/v8/src/builtins/builtins-utils-inl.h:8,
                 from ../deps/v8/src/builtins/builtins-arraybuffer.cc:5:
In member function 'std::__atomic_base<_IntTp>::__int_type std::__atomic_base<_IntTp>::load(std::memory_order) const [with _ITp = long unsigned int]',
    inlined from 'size_t v8::internal::BackingStore::byte_length(std::memory_order) const' at ../deps/v8/src/objects/backing-store.h:91:29,
    inlined from 'size_t v8::internal::JSArrayBuffer::GetByteLength() const' at ../deps/v8/src/objects/js-array-buffer-inl.h:59:42,
    inlined from 'size_t v8::internal::JSArrayBuffer::GetByteLength() const' at ../deps/v8/src/objects/js-array-buffer-inl.h:53:8,
    inlined from 'v8::internal::Object v8::internal::Builtin_Impl_SharedArrayBufferPrototypeGetByteLength(BuiltinArguments, Isolate*)' at ../deps/v8/src/builtins/builtins-arraybuffer.cc:478:51,
    inlined from 'v8::internal::Address v8::internal::Builtin_SharedArrayBufferPrototypeGetByteLength(int, Address*, Isolate*)' at ../deps/v8/src/builtins/builtins-arraybuffer.cc:465:1:
/usr/include/c++/12/bits/atomic_base.h:488:31: warning: 'long unsigned int __atomic_load_8(const volatile void*, int)' writing 8 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
  488 |         return __atomic_load_n(&_M_i, int(__m));
      |                ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
In member function 'std::__atomic_base<_IntTp>::__int_type std::__atomic_base<_IntTp>::load(std::memory_order) const [with _ITp = long unsigned int]',
    inlined from 'size_t v8::internal::BackingStore::byte_length(std::memory_order) const' at ../deps/v8/src/objects/backing-store.h:91:29,
    inlined from 'size_t v8::internal::JSArrayBuffer::GetByteLength() const' at ../deps/v8/src/objects/js-array-buffer-inl.h:59:42,
    inlined from 'size_t v8::internal::JSArrayBuffer::GetByteLength() const' at ../deps/v8/src/objects/js-array-buffer-inl.h:53:8:
/usr/include/c++/12/bits/atomic_base.h:488:31: warning: 'long unsigned int __atomic_load_8(const volatile void*, int)' writing 8 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
  488 |         return __atomic_load_n(&_M_i, int(__m));
      |                ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~

That some crypto fails points to a openssl runtime bug maybe. However that some vm tests fail too is confusing.

kvakil commented 2 years ago

I coudln't reproduce on amd64 with gcc 12, so I tagged this as mips/risc-v specific.

kapouer commented 2 years ago

log for riscv64

bnoordhuis commented 2 years ago

Looking at the test failures, it's either a gcc bug (unlikely) or UB in V8 that causes miscompiles with gcc-12.

The mips64el and riscv ports are maintained externally and don't get the same coverage as the upstream maintained ports so bugs like these are not too surprising.

Turning on sanitizers (ASan, TSan, etc.) may help track down the cause.

kapouer commented 1 year ago

Maybe those failures have nothing to do with gcc...

This doesn't throw on mips64el:

node --frozen-intrinsics -e "'use strict'; globalThis.globalThis = null;"

but does throw on amd64.

More precisely, on mips64el, it doesn't throw but doesn't set globalThis either. The flag is honored, just not the fact it should throw with "use strict".

kapouer commented 1 year ago

Changed the title: all the failing tests fail the same way on amd64 when removing "use strict".

bnoordhuis commented 1 year ago

Can you report that upstream? That's a bug in V8's mips port.

kapouer commented 1 year ago

https://bugs.chromium.org/p/v8/issues/detail?id=13437

luyahan commented 1 year ago

Hi～ I am maintain v8 for riscv. I have a question about this issue. I don't understand the function of this parameter frozen_intrinsics. It isn't parameter for v8 and I can't find any code in the src of node except its declaration and definition

src/node_options.cc

  AddOption("--frozen-intrinsics",
            "experimental frozen intrinsics support",
            &EnvironmentOptions::frozen_intrinsics,
            kAllowedInEnvvar);

src/node_options.h

  bool frozen_intrinsics = false;
RaisinTen commented 1 year ago

@luyahan it's true that it's not used inside src/ but we do have some code in lib/ that runs internal/freeze_intrinsics if the option is passed, see - https://github.com/nodejs/node/blob/d5d7a416c72a46a91dbe2daf14239a55143a52f8/lib/internal/process/pre_execution.js#L611-L615

luyahan commented 1 year ago

test on commit d5d7a416c72a46a91dbe2daf14239a55143a52f8 It's is ok

luyahan@plct-dev-7:~/source/node/out/Release $ qemu-riscv64 -L /home/luyahan/riscv64/sysroot/ ./node --frozen-intrinsics -e "'use strict'; globalThis.globalThis = null;"
[eval]:1
'use strict'; globalThis.globalThis = null;
                                    ^

TypeError: Cannot assign to read only property 'globalThis' of object '[object global]'
    at [eval]:1:37
    at Script.runInThisContext (node:vm:129:12)
    at Object.runInThisContext (node:vm:307:38)
    at node:internal/process/execution:83:21
    at [eval]-wrapper:6:24
    at runScript (node:internal/process/execution:82:62)
    at evalScript (node:internal/process/execution:104:10)
    at node:internal/main/eval_string:50:3

Node.js v20.0.0-pre

Maybe when V8 Release , the RV does not PORT upstream of a certain patch in time.

@kapouer Can you double check it? Thanks

kapouer commented 1 year ago

Using commit 78954846 and testing with ./out/Release/node --frozen-intrinsics -e "'use strict'; globalThis.globalThis = null;",

Correct error is thrown:

./configure --shared-zlib --shared-cares --shared-nghttp2 --shared-brotli --shared-openssl --openssl-use-def-ca-store --shared-libuv --with-intl=system-icu --dest-cpu=riscv64

No error thrown:

./configure --shared-zlib --shared-cares --shared-nghttp2 --shared-brotli --shared-openssl --openssl-use-def-ca-store --shared-libuv --shared --with-intl=system-icu --dest-cpu=riscv64

However, building node 18.13.0 with or without the --shared flag gives the same result.

kapouer commented 1 year ago

Or, maybe it's something with how "use strict" is activated: starting from https://github.com/nodejs/node/blob/e7504f2f5949c677fddb9b30efba85e03cdea2d9/deps/v8/include/v8-function-callback.h#L223-L230 hinting at IntToSmi in https://github.com/nodejs/node/blob/e7504f2f5949c677fddb9b30efba85e03cdea2d9/deps/v8/include/v8-internal.h#L154-L157 and then V8_31BIT_SMIS_ON_64BIT_ARCH and corresponding configure flag v8_enable_31bit_smis_on_64bit_arch, which is defined here: https://github.com/nodejs/node/blob/e7504f2f5949c677fddb9b30efba85e03cdea2d9/common.gypi#L105-L109

That would explain very well why I have an issue with two unrelated architectures.

kapouer commented 1 year ago

Building node 18 on riscv64 with --experimental-enable-pointer-compression, to check if that works around the issue.

kapouer commented 1 year ago

Result with node 18 is

g++-12 -o /home/kapouer/nodejs->18.13.0+dfsg1/out/Release/obj.target/v8_base_without_compiler/deps/v8/src/baseline/bytecode-offset-iterator.o >../deps/v8/src/baseline/bytecode-offset-iterator.cc '-D_GLIBCXX_USE_CXX11_ABI=1' '->DNODE_OPENSSL_CONF_NAME=nodejs_conf' '-DNODE_OPENSSL_CERT_STORE' '->DNODE_SHARED_BUILTIN_CJS_MODULE_LEXER_LEXER_PATH=/usr/share/nodejs/cjs-module-lexer/lexer.js' '->DNODE_SHARED_BUILTIN_CJS_MODULE_LEXER_DIST_LEXER_PATH=/usr/share/nodejs/cjs-module-lexer/dist/lexer.js' '->DNODE_SHARED_BUILTIN_UNDICI_UNDICI_PATH=/usr/share/nodejs/undici/undici-fetch.js' '->DNODE_SHARED_BUILTIN_ACORN_PATH=/usr/share/nodejs/acorn/dist/acorn.js' '->DNODE_SHARED_BUILTIN_ACORN_WALK_PATH=/usr/share/nodejs/acorn-walk/dist/walk.js' '-DV8_GYP_BUILD' '->DV8_TYPED_ARRAY_MAX_SIZE_IN_HEAP=64' '-DV8_COMPRESS_POINTERS' '->DV8_COMPRESS_POINTERS_IN_ISOLATE_CAGE' '-DV8_31BIT_SMIS_ON_64BIT_ARCH' '-DSTDC_FORMAT_MACROS' '->DV8_TARGET_ARCH_RISCV64' '-Driscv_xlen=64' '-DCAN_USE_FPU_INSTRUCTIONS' '-DV8_HAVE_TARGET_OS' '->DV8_TARGET_OS_LINUX' '-DV8_EMBEDDER_STRING="-node.21"' '-DENABLE_DISASSEMBLER' '->DV8_PROMISE_INTERNAL_FIELD_COUNT=1' '-DOBJECT_PRINT' '-DV8_INTL_SUPPORT' '->DV8_ATOMIC_OBJECT_FIELD_WRITES' '-DV8_ENABLE_LAZY_SOURCE_POSITIONS' '-DV8_USE_SIPHASH' '->DV8_WIN64_UNWINDING_INFO' '-DV8_ENABLE_REGEXP_INTERPRETER_THREADED_DISPATCH' '->DV8_ENABLE_WEBASSEMBLY' '-DV8_ENABLE_JAVASCRIPT_PROMISE_HOOKS' '-DV8_ALLOCATION_FOLDING' '->DV8_ALLOCATION_SITE_TRACKING' '-DV8_SCRIPTORMODULE_LEGACY_LIFETIME' '->DV8_ADVANCED_BIGINT_ALGORITHMS' '-DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_STATIC' -I../deps/v8 ->I../deps/v8/include -I/home/kapouer/nodejs-18.13.0+dfsg1/out/Release/obj/gen/inspector-generated-output-root ->I../deps/v8/third_party/inspector_protocol -I/home/kapouer/nodejs-18.13.0+dfsg1/out/Release/obj/gen ->I/home/kapouer/nodejs-18.13.0+dfsg1/out/Release/obj/gen/generate-bytecode-output-root ->I../deps/v8/third_party/zlib -I../deps/v8/third_party/zlib/google -pthread -Wno-unused-parameter -fPIC -Wno-return->type -fno-strict-aliasing -O3 -fno-omit-frame-pointer -fdata-sections -ffunction-sections -O3 -fno-rtti -fno-exceptions ->std=gnu++17 -MMD -MF /home/kapouer/nodejs-18.13.0+dfsg1/out/Release/.deps//home/kapouer/nodejs->18.13.0+dfsg1/out/Release/obj.target/v8_base_without_compiler/deps/v8/src/baseline/bytecode-offset->iterator.o.d.raw -fPIC -g -fPIC -g -fPIC -g -fPIC -g -fPIC -g -c In file included from ../deps/v8/src/baseline/baseline->compiler.cc:51: ../deps/v8/src/baseline/riscv64/baseline-compiler-riscv64-inl.h: In member function 'void >v8::internal::baseline::BaselineCompiler::PrologueFillFrame()': ../deps/v8/src/baseline/riscv64/baseline-compiler-riscv64-inl.h:42:40: error: 'kPointerSize' was not declared in this scope 42 | masm()->Add64(sp, sp, Operand(-(kPointerSize new_target_index))); | ^~~~ ../deps/v8/src/baseline/riscv64/baseline->compiler-riscv64-inl.h:52:40: error: 'kPointerSize' was not declared in this scope 52 | __ masm()->Add64(sp, sp, >Operand(-(kPointerSize register_count))); | ^~~~ >../deps/v8/src/baseline/riscv64/baseline-compiler-riscv64-inl.h:57:40: error: 'kPointerSize' was not declared in this >scope 57 | masm()->Add64(sp, sp, Operand(-(kPointerSize * register_count))); | >^~~~

kapouer commented 5 months ago

@luyahan same tests are still failing with nodejs 20.12.2 on mips64el and loong64 https://buildd.debian.org/status/fetch.php?pkg=nodejs&arch=mips64el&ver=20.12.2%2Bdfsg-1&stamp=1713937710&raw=0 https://buildd.debian.org/status/fetch.php?pkg=nodejs&arch=loong64&ver=20.12.2%2Bdfsg-1&stamp=1713910684&raw=0

(for comparison, armhf build gives https://buildd.debian.org/status/fetch.php?pkg=nodejs&arch=armhf&ver=20.12.2%2Bdfsg-1&stamp=1713920235&raw=0)

mhdawson commented 5 months ago

@shipujin is this something you could look at from the loong64 perspective?

shipujin commented 5 months ago

@shipujin is this something you could look at from the loong64 perspective?

ok, i test this locally

Best regards

shipujin commented 5 months ago

https://buildd.debian.org/status/fetch.php?pkg=nodejs&arch=loong64&ver=20.12.2%2Bdfsg-1&stamp=1713910684&raw=0

Hi @kapouer @mhdawson , I looked at the debian loong64 nodejs compilation log and found:

cat build.log| grep "not ok"

not ok 456 parallel/test-crypto-sign-verify # TODO : Fix flaky test
not ok 794 parallel/test-freeze-intrinsics # TODO : Fix flaky test
not ok 1167 parallel/test-debugger-preserve-breaks # TODO : Fix flaky test
not ok 2879 parallel/test-tls-root-certificates # TODO : Fix flaky test
not ok 3054 parallel/test-vm-global-non-writable-properties # TODO : Fix flaky test
not ok 3056 parallel/test-vm-global-setter # TODO : Fix flaky test
not ok 3076 parallel/test-vm-module-synthetic # TODO : Fix flaky test
not ok 3093 parallel/test-vm-strict-assign # TODO : Fix flaky test
not ok 3426 sequential/test-cpu-prof-dir-worker # TODO : Fix flaky test

I guess the failed test item seems to be caused by the use of --openssl-no-asm, It is recommended to temporarily "PASS, FLAKY" in debian loong64

--- a/test/parallel/parallel.status                                                                                                                                                   [2/8]
+++ b/test/parallel/parallel.status

+[$arch==loong64]                 
+test-crypto-sign-verify: PASS, FLAKY
+test-freeze-intrinsics: PASS, FLAKY
+test-debugger-preserve-breaks: PASS, FLAKY
+test-tls-root-certificates: PASS, FLAKY
+test-vm-global-non-writable-properties: PASS, FLAKY
+test-vm-global-setter: PASS, FLAKY   
+test-vm-module-synthetic: PASS, FLAKY
+test-vm-strict-assign: PASS, FLAKY
+
+
--- a/test/sequential/sequential.status
+++ b/test/sequential/sequential.status

+test-cpu-prof-dir-worker: PASS, FLAKY
+
kapouer commented 5 months ago

(Yes, I have marked them as PASS, FLAKY because it's easier to keep nodejs in loong64, even with test failures)

How is --openssl-no-asm enabled here ? openssl 3.2.1-3, the one used as shared lib in that case, seems to be compiled with asm support: https://buildd.debian.org/status/fetch.php?pkg=openssl&arch=loong64&ver=3.2.1-3&stamp=1712268679&raw=0

shipujin commented 5 months ago

(Yes, I have marked them as PASS, FLAKY because it's easier to keep nodejs in loong64, even with test failures)

How is --openssl-no-asm enabled here ? openssl 3.2.1-3, the one used as shared lib in that case, seems to be compiled with asm support: https://buildd.debian.org/status/fetch.php?pkg=openssl&arch=loong64&ver=3.2.1-3&stamp=1712268679&raw=0

I'm sorry, but I realize that debian-nodejs uses --shared-openssl, so it's probably not openssl-no-asm

Best regards

shipujin commented 5 months ago

Look the failed items are related to ssl, but the debian-nodejs build uses --shared-openssl, which is weird

Best regards

shipujin commented 3 months ago

Hi @kapouer

nodejs_20.14.0+dfsg-2_changelog: https://metadata.ftp-master.debian.org/changelogs//main/n/nodejs/nodejs_20.14.0+dfsg-2_changelog

not ok parallel/test-debugger-preserve-breaks  
not ok sequential/test-cpu-prof-dir-absolute

loong64 has two new failure entries that may need to be labeled "PASS, FLAKY"

--- a/test/parallel/parallel.status                                                                                                                                                   [2/8]
+++ b/test/parallel/parallel.status

+[$arch==loong64]
+parallel/test-debugger-preserve-breaks: PASS, FLAKY
--- a/test/sequential/sequential.status
+++ b/test/sequential/sequential.status
+[$arch==loong64]
+sequential/test-cpu-prof-dir-absolute: PASS, FLAKY

Best regards

kapouer commented 3 months ago

Thanks

shipujin commented 3 months ago

Hi @kapouer

nodejs_20.14.0+dfsg-3_changelog: https://buildd.debian.org/status/fetch.php?pkg=nodejs&arch=loong64&ver=20.14.0%2Bdfsg-3&stamp=1718796271&raw=0

cat log | grep "not ok"
not ok 467 parallel/test-crypto-sign-verify # TODO : Fix flaky test
not ok 812 parallel/test-freeze-intrinsics # TODO : Fix flaky test
not ok 868 parallel/test-debugger-preserve-breaks # TODO : Fix flaky test
not ok 2911 parallel/test-tls-root-certificates # TODO : Fix flaky test
not ok 3091 parallel/test-vm-global-non-writable-properties # TODO : Fix flaky test
not ok 3095 parallel/test-vm-global-setter # TODO : Fix flaky test
not ok 3111 parallel/test-vm-module-synthetic # TODO : Fix flaky test
not ok 3135 parallel/test-vm-strict-assign # TODO : Fix flaky test
not ok 3551 sequential/test-worker-prof

loong64 has two new failure entries that may need to be labeled "PASS, FLAKY"

--- a/test/sequential/sequential.status
+++ b/test/sequential/sequential.status
+[$arch==loong64]
+sequential/test-worker-prof: PASS, FLAKY

Best regards

shipujin commented 3 months ago

Hi @kapouer

20.15.0+dfsg-1 changelog: https://buildd.debian.org/status/fetch.php?pkg=nodejs&arch=loong64&ver=20.15.0%2Bdfsg-1&stamp=1719326881&raw=0

cat log | grep "not ok"
not ok 467 parallel/test-crypto-sign-verify # TODO : Fix flaky test
not ok 812 parallel/test-freeze-intrinsics # TODO : Fix flaky test
not ok 871 parallel/test-debugger-preserve-breaks # TODO : Fix flaky test
not ok 2920 parallel/test-tls-root-certificates # TODO : Fix flaky test
not ok 3099 parallel/test-vm-global-non-writable-properties # TODO : Fix flaky test
not ok 3101 parallel/test-vm-global-setter # TODO : Fix flaky test
not ok 3118 parallel/test-vm-module-synthetic # TODO : Fix flaky test
not ok 3141 parallel/test-vm-strict-assign # TODO : Fix flaky test
not ok 3462 sequential/test-cpu-prof-dir-relative

loong64 has two new failure entries that may need to be labeled "PASS, FLAKY"

--- a/test/sequential/sequential.status
+++ b/test/sequential/sequential.status
[$arch==loong64]
test-worker-prof: PASS, FLAKY
+test-cpu-prof-dir-relative: PASS, FLAKY

Best regards

thesamesam commented 2 months ago

Hi! At least part of this issue, possibly the whole thing, was debugged by @WhatAmISupposedToPutHere and reported at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116057.

It appears it wasn't an issue in the V8 backend for a specific platform, but rather a miscompilation by GCC of V8. It's still not completely clear whether there may be either UB or ill-advised code in V8, but GCC's fixed it on their side. As of writing, it's not yet been backported. Thanks.