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.5 #34337

Closed
wants to merge 28 commits into from
Closed

deps: update V8 to 8.5 #34337

wants to merge 28 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Jul 13, 2020

Stable Release: Tue, Aug 25, 2020

@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 13, 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 Jul 13, 2020
@targos targos marked this pull request as draft July 13, 2020 13:41
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 13, 2020

@targos
Copy link
Member Author

targos commented Jul 13, 2020

/cc @gengjiawen related to nodejs/node-v8#161

@gengjiawen
Copy link
Member

/cc @gengjiawen related to nodejs/node-v8#161

Yeap. Also msvc only report one error, I will give it a try.

BTW, we need to cherrypick v8/v8@4ece106.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 23, 2020

@targos, since --experimental-wasm-bigint is no longer required for the WASI API, would you like to pull cjihrig@dbff5bc into this upgrade, or would you prefer me to PR it separately after the upgrade lands?

@gengjiawen
Copy link
Member

gengjiawen commented Aug 23, 2020

In case you want patch code for windows, you can get first two commit in https://github.com/gengjiawen/node/commits/v8-8.5-hack before nodejs/gyp-next#60 get fixed. @targos

@bricss
Copy link

bricss commented Sep 4, 2020

Is there any chance that it may get into Node 14 or 15? 😶

targos and others added 6 commits September 7, 2020 09:32
Major V8 updates are usually API/ABI incompatible with previous
versions. This commit adapts NODE_MODULE_VERSION for V8 8.5.

Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md
Original commit message:

    [testrunner] delete ancient junit compatible format support

    Testrunner has ancient support for JUnit compatible XML output.

    This CL removes this old feature.

    R=mstarzinger@chromium.org,jgruber@chromium.org,jkummerow@chromium.org
    CC=​machenbach@chromium.org

    Bug: v8:8728
    Change-Id: I7e1beb011dbaec3aa1a27398a5c52abdd778eaf0
    Reviewed-on: https://chromium-review.googlesource.com/c/1430065
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
    Commit-Queue: Tamer Tas <tmrts@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#59045}

Refs: v8/v8@bd019bd

PR-URL: nodejs#32116
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Patch V8 (compiler/js-heap-broker.cc) to remove the use of an optional
property, which is a fairly new C++ feature, since that requires a newer
XCode version than the minimum requirement in BUILDING.md and thus
breaks CI.

PR-URL: nodejs#32116
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Fixes a compilation issue on some platforms

PR-URL: nodejs#32116
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@targos
Copy link
Member Author

targos commented Sep 26, 2020

@gengjiawen
Copy link
Member

gengjiawen commented Sep 26, 2020

@targos I believe I have find the root cause through the generated mk file. It looks like the assembly file processed both on host and target. So we should limit it only on target (<(V8_ROOT)/src/heap/base/asm/arm/push_registers_asm.cc are valid assembly only for arm, that's why gcc failed on v8_base_without_compiler.host.mk) .

Below is build by removing it in v8_base_without_compiler.host.mk file manully.

image

Maybe only something like (not quite sure about the syntax)

              ['_toolset == "target" and v8_current_cpu=="arm"', {
                'sources': [
                  '<(V8_ROOT)/src/heap/base/asm/arm/push_registers_asm.cc'
                ],
              }],

@targos
Copy link
Member Author

targos commented Sep 27, 2020

I don't know... My condition seems right. I looked at the Jenkins logs and for cross-compiler-ubuntu1804-armv7-gcc-6, host_arch is ia32 and target_arch is arm, so unless I'm missing something with priority of operators in Python, it should pick up the ia32 file.

@targos
Copy link
Member Author

targos commented Sep 27, 2020

So, with the log I added, we can see that the variables have the following values:

  • 'host_arch': 'ia32'
  • 'target_arch': 'arm'
  • '_toolset': 'target'

There's no other log where _toolset is host 🤔. I'm trying one last thing.

CI: https://ci.nodejs.org/job/node-test-commit-arm-fanned/16551/

Edit: doesn't work. I'm out of ideas.

@mmarchini
Copy link
Contributor

@targos if you want to test locally, you can also try https://github.com/mmarchini/node-arm-cross-builder

@mmarchini
Copy link
Contributor

_toolset being always target seems like either a bug, or we're using the wrong variable to test the current toolset.

@gengjiawen
Copy link
Member

gengjiawen commented Sep 27, 2020

_toolset being always target seems like either a bug, or we're using the wrong variable to test the current toolset.

_toolset seems to be 'target' all the time from what I test. Maybe a bug in gyp.

Looks the generated mk file toolset is set correctly. Where _toolset is set in gyp ?

  1 # This file is generated by gyp; do not edit.
  2 
  3 TOOLSET := host
  4 TARGET := v8_base_without_compiler

@gengjiawen

This comment has been minimized.

@sxa
Copy link
Member

sxa commented Sep 29, 2020

I realise I'm somewhat "drive-by commenting" here, but is there a reason why we still have to cross compile for arm32? There's some reasonably quick stuff out there now (e.g. Raspberry Pi4 or ODROID-XU4 which I use) and if those could be used to build natively in a reasonable amount of time and don't exhibit the same problems it might be worth switching.

@gengjiawen
Copy link
Member

gengjiawen commented Sep 29, 2020

So, with the log I added, we can see that the variables have the following values:

  • 'host_arch': 'ia32'
  • 'target_arch': 'arm'
  • '_toolset': 'target'

There's no other log where _toolset is host 🤔. I'm trying one last thing.

CI: ci.nodejs.org/job/node-test-commit-arm-fanned/16551

Edit: doesn't work. I'm out of ideas.

Tinker v8.gyp like will have '_toolset': 'host' in the log, but I still get v8_base_without_compiler.host.mk have $(obj).host/$(TARGET)/deps/v8/src/heap/base/asm/ia32/push_registers_asm.o, not sure why.

    {
      'target_name': 'v8_cppgc_shared',
      'type': 'none',
      # 'conditions': [
      #   ['want_separate_host_toolset', {
      #     'toolsets': ['host', 'target'],
      #   }],
      # ],
      'toolsets': ['host', 'target']

@gengjiawen
Copy link
Member

gengjiawen commented Sep 29, 2020

@sxa It's already tier 1 in build. I suppose even if we want to change it. It will be a much more bigger issue.

Context: nodejs/TSC#931 (comment).

@sxa
Copy link
Member

sxa commented Sep 29, 2020

For completeness, link to a proposed switch to building natively that went stale nodejs/build#1851

@gengjiawen
Copy link
Member

gengjiawen commented Sep 29, 2020

@targos I think I find the fix, not sure the reason why. I tested in the docker, works fine, you can test it in Jenkins.

Change the part like this

    {
      'target_name': 'v8_cppgc_shared',
      'type': 'none',
      # 'conditions': [
      #   ['want_separate_host_toolset', {
      #     'toolsets': ['host', 'target'],
      #   }],
      # ],
      'toolsets': ['host', 'target'],
      'direct_dependent_settings': {
        'sources': [
          '<(V8_ROOT)/src/heap/base/stack.cc',
          '<(V8_ROOT)/src/heap/base/stack.h',
        ],
        'conditions': [
          ['clang or OS!="win"', {
            'conditions': [
              ['_toolset=="target" and target_arch == "arm"', {
                'sources': [
                  '<(V8_ROOT)/src/heap/base/asm/arm/push_registers_asm.cc',
                ],
              }],
              ['_toolset == "target" and target_arch=="arm64"', {
                'sources': [
                  '<(V8_ROOT)/src/heap/base/asm/arm64/push_registers_asm.cc',
                ],
              }],
            ]
          // other omitted.

@targos
Copy link
Member Author

targos commented Sep 29, 2020

@gengjiawen with the change you propose, does it still work for regular compilation, for example x64?

@gengjiawen
Copy link
Member

@gengjiawen with the change you propose, does it still work for regular compilation, for example x64?

Not test it yet, give me a second.

@gengjiawen
Copy link
Member

gengjiawen commented Sep 29, 2020

@gengjiawen with the change you propose, does it still work for regular compilation, for example x64?

@targos Looks like still works (Note: in my snippet I only show two cpus).
image

PS: the full build without ccache is really long.

@targos
Copy link
Member Author

targos commented Sep 29, 2020

Let's try it!

@nodejs-github-bot
Copy link
Collaborator

@gengjiawen

This comment has been minimized.

@targos
Copy link
Member Author

targos commented Sep 29, 2020

It works!!! Thank you @gengjiawen ❤️ . Now I'm going to open a new PR to update directly to 8.6, because it will be stable next week and we need it ASAP for v15.

@gengjiawen
Copy link
Member

It works!!! Thank you @gengjiawen ❤️ . Now I'm going to open a new PR to update directly to 8.6, because it will be stable next week and we need it ASAP for v15.

You want to land this first or drop it ?

@targos
Copy link
Member Author

targos commented Sep 29, 2020

It's easier for me to drop it.

@targos
Copy link
Member Author

targos commented Sep 29, 2020

PR for 8.6: #35415

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. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. 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.