Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: update V8 to 8.4 #33579

Closed
wants to merge 19 commits into from
Closed

deps: update V8 to 8.4 #33579

wants to merge 19 commits into from

Conversation

targos
Copy link
Member

@targos targos commented May 27, 2020

Stable release: Tue, Jul 14, 2020

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels May 27, 2020
@nodejs-github-bot
Copy link
Collaborator

@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 27, 2020
@targos
Copy link
Member Author

targos commented May 27, 2020

/cc @nodejs/platform-smartos

08:51:20 ../deps/v8/src/base/platform/platform-posix.cc: In static member function 'static void* v8::base::Stack::GetStackStart()':
08:51:20 ../deps/v8/src/base/platform/platform-posix.cc:978:15: error: 'pthread_getattr_np' was not declared in this scope
08:51:20    int error = pthread_getattr_np(pthread_self(), &attr);
08:51:20                ^~~~~~~~~~~~~~~~~~
08:51:20 ../deps/v8/src/base/platform/platform-posix.cc:978:15: note: suggested alternative: 'pthread_getname_np'
08:51:20    int error = pthread_getattr_np(pthread_self(), &attr);
08:51:20                ^~~~~~~~~~~~~~~~~~
08:51:20                pthread_getname_np
08:51:20 make[2]: *** [tools/v8_gypfiles/v8_libbase.target.mk:145: /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos18-64/out/Release/obj.target/v8_libbase/deps/v8/src/base/platform/platform-posix.o] Error 1

I fixed a similar error upstream for FreeBSD. The trick was to #include <pthread_np.h>.

@targos
Copy link
Member Author

targos commented May 27, 2020

/cc @nodejs/platform-windows can we work around this error with VS2017?

09:04:25   exported-macros-assembler-tq.cc
09:04:25 c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.16.27023\include\type_traits(616): error C2139: 'v8::internal::JSAggregateError': an undefined class is not allowed as an argument to compiler intrinsic type trait '__is_convertible_to' [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers.vcxproj]
09:04:25   c:\workspace\node-compile-windows\node\out\release\obj\global_intermediate\torque-output-root\torque-generated\internal-class-definitions-tq.h(109): note: see declaration of 'v8::internal::JSAggregateError'
09:04:25   c:\workspace\node-compile-windows\node\deps\v8\src\codegen\tnode.h(262): note: see reference to class template instantiation 'std::is_convertible<T,v8::internal::Object>' being compiled
09:04:25           with
09:04:25           [
09:04:25               T=v8::internal::JSAggregateError
09:04:25           ]
09:04:25   c:\workspace\node-compile-windows\node\out\release\obj\global_intermediate\torque-output-root\torque-generated\exported-macros-assembler-tq.cc(1418): note: see reference to class template instantiation 'v8::internal::is_subtype<v8::internal::JSAggregateError,v8::internal::JSAggregateError>' being compiled
09:04:25   c:\workspace\node-compile-windows\node\deps\v8\src\codegen\x64\register-x64.h(153): note: see reference to class template instantiation 'v8::internal::RegisterBase<v8::internal::XMMRegister,16>' being compiled
09:04:25   c:\workspace\node-compile-windows\node\deps\v8\src\codegen\x64\register-x64.h(53): note: see reference to class template instantiation 'v8::internal::RegisterBase<v8::internal::Register,16>' being compiled

@seishun
Copy link
Contributor

seishun commented May 27, 2020

Why not drop VS2017?

@targos
Copy link
Member Author

targos commented May 27, 2020

If we just drop VS2017, we won't be able to do this V8 update on Node 14.

@targos
Copy link
Member Author

targos commented May 29, 2020

@targos
Copy link
Member Author

targos commented May 29, 2020

10:05:06 not ok 1 v8-updates/test-postmortem-metadata
10:05:06   ---
10:05:06   duration_ms: 0.918
10:05:06   severity: fail
10:05:06   exitcode: 1
10:05:06   stack: |-
10:05:06     assert.js:103
10:05:06       throw new AssertionError(obj);
10:05:06       ^
10:05:06     
10:05:06     AssertionError [ERR_ASSERTION]: Missing constants: 
10:05:06     v8dbg_class_FixedArrayBase__length__SMI
10:05:06     v8dbg_class_JSTypedArray__base_pointer__Object
10:05:06     v8dbg_class_JSArrayBufferView__buffer__Object
10:05:06     v8dbg_class_UncompiledData__inferred_name__String
10:05:06         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-v8-linux/nodes/benchmark-ubuntu1604-intel-64/v8test/v8test/test/v8-updates/test-postmortem-metadata.js:51:8)
10:05:06         at Module._compile (internal/modules/cjs/loader.js:1215:30)
10:05:06         at Object.Module._extensions..js (internal/modules/cjs/loader.js:1236:10)
10:05:06         at Module.load (internal/modules/cjs/loader.js:1064:32)
10:05:06         at Function.Module._load (internal/modules/cjs/loader.js:952:14)
10:05:06         at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
10:05:06         at internal/main/run_main_module.js:17:47 {
10:05:06       generatedMessage: false,
10:05:06       code: 'ERR_ASSERTION',
10:05:06       actual: 4,
10:05:06       expected: 0,
10:05:06       operator: 'strictEqual'
10:05:06     }
10:05:06   ...

@cjihrig

Edit: and PTAL at #33579 (comment) in case you missed it.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@cjihrig
Copy link
Contributor

cjihrig commented May 31, 2020

@targos the SmartOS error appears to be the same one from the V8 8.3 update. I think you should just need to cherry-pick v8/v8@9868b2a to fix it.

Regarding the test-postmortem-metadata failure: it seems like @mmarchini has sort of taken over that. If I'm wrong and that isn't the case, I'm happy to look into it.

@gengjiawen
Copy link
Member

/cc @nodejs/platform-windows can we work around this error with VS2017?

09:04:25   exported-macros-assembler-tq.cc
09:04:25 c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.16.27023\include\type_traits(616): error C2139: 'v8::internal::JSAggregateError': an undefined class is not allowed as an argument to compiler intrinsic type trait '__is_convertible_to' [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers.vcxproj]
09:04:25   c:\workspace\node-compile-windows\node\out\release\obj\global_intermediate\torque-output-root\torque-generated\internal-class-definitions-tq.h(109): note: see declaration of 'v8::internal::JSAggregateError'
09:04:25   c:\workspace\node-compile-windows\node\deps\v8\src\codegen\tnode.h(262): note: see reference to class template instantiation 'std::is_convertible<T,v8::internal::Object>' being compiled
09:04:25           with
09:04:25           [
09:04:25               T=v8::internal::JSAggregateError
09:04:25           ]
09:04:25   c:\workspace\node-compile-windows\node\out\release\obj\global_intermediate\torque-output-root\torque-generated\exported-macros-assembler-tq.cc(1418): note: see reference to class template instantiation 'v8::internal::is_subtype<v8::internal::JSAggregateError,v8::internal::JSAggregateError>' being compiled
09:04:25   c:\workspace\node-compile-windows\node\deps\v8\src\codegen\x64\register-x64.h(153): note: see reference to class template instantiation 'v8::internal::RegisterBase<v8::internal::XMMRegister,16>' being compiled
09:04:25   c:\workspace\node-compile-windows\node\deps\v8\src\codegen\x64\register-x64.h(53): note: see reference to class template instantiation 'v8::internal::RegisterBase<v8::internal::Register,16>' being compiled

This should fix it. cc @targos

diff --git a/deps/v8/src/torque/implementation-visitor.cc b/deps/v8/src/torque/implementation-visitor.cc
index bee31b4d32..1537e33bab 100644
--- a/deps/v8/src/torque/implementation-visitor.cc
+++ b/deps/v8/src/torque/implementation-visitor.cc
@@ -4616,6 +4616,7 @@ void ImplementationVisitor::GenerateExportedMacrosAssembler(
     cc_contents << "#include \"src/objects/fixed-array-inl.h\"\n";
     cc_contents << "#include \"src/objects/free-space.h\"\n";
     cc_contents << "#include \"src/objects/js-regexp-string-iterator.h\"\n";
+    cc_contents << "#include \"src/objects/js-aggregate-error.h\"\n";
     cc_contents << "#include \"src/objects/ordered-hash-table.h\"\n";
     cc_contents << "#include \"src/objects/property-descriptor-object.h\"\n";
     cc_contents << "#include \"src/objects/synthetic-module.h\"\n";

@gengjiawen
Copy link
Member

@targos Next time if you see vs2017 build problem, feel free to @ me.

I am +1 on remove vs2017 in Node.js 15.

Also great thanks for @appveyor to let me use their machine for debug.

@bzoz
Copy link
Contributor

bzoz commented Jun 1, 2020

can confirm, @gengjiawen patch fixes the build

@gengjiawen
Copy link
Member

gengjiawen commented Jun 2, 2020

@targos Next time if you see vs2017 build problem, feel free to @ me.

I am +1 on remove vs2017 in Node.js 15.

Also great thanks for @appveyor to let me use their machine for debug.

PR to V8: https://chromium-review.googlesource.com/c/v8/v8/+/2226123.

@targos
Copy link
Member Author

targos commented Jun 2, 2020

@cjihrig right thanks. Somehow I though this commit was already in 8.4.

@gengjiawen I included your change, thanks!

@mmarchini are you able to handle #33579 (comment) ?

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member Author

targos commented Jun 2, 2020

There's another SmartOS error:

10:57:10 dtrace: failed to compile script src/v8ustack.d: line 502: failed to resolve V8DBG_CLASS_FIXEDARRAYBASE__LENGTH__SMI: Unknown variable name
10:57:10 make[2]: *** [node_dtrace_ustack.target.mk:26: /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos18-64/out/Release/obj.target/libnode/src/node_dtrace_ustack.o] Error 1

@targos
Copy link
Member Author

targos commented Jun 16, 2020

ping @cjihrig / @mmarchini

@mmarchini
Copy link
Contributor

I'll try to send a patch after the TSC meeting.

@mmarchini
Copy link
Contributor

Sorry, didn't had time to do it last week and I'm mostly away this week due to Summit + personal stuff, don't think I'll be able to get to it before next Wednesday (or after).

@mmarchini
Copy link
Contributor

mmarchini commented Jun 25, 2020

FWIW I'm not looking at test-postmortem-metadata anymore since I moved metadata changes detection that I use for llnode to https://github.com/mmarchini/llnode-v8-tester, so unless @cjihrig objects I wouldn't mind this test being marked as flaky or removed.

It doesn't solve the dtrace issues though.

@targos targos removed their assignment Oct 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. semver-major PRs that contain breaking changes and should be released in the next major version. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.