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.

Sign up for GitHub

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

Jump to bottom

deps: V8: cherry-pick fd75c97d3f56 #38455

Closed
wants to merge 1 commit into from
Closed

deps: V8: cherry-pick fd75c97d3f56 #38455

wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Apr 28, 2021

Fixes interpretation of the following:

null?.(...[], 1)

In Node.js <= 16.0.0, it incorrectly throws a TypeError.

Original commit message:

[interpreter] Apply Reflect.apply transform in BytecodeGenerator

Calls with a spread expression in a non-final position get transformed
to calls to Reflect.apply. This transformation is currently done in
the parser, which does not compose well with other features (e.g.
direct eval checking, optional chaining).

Do this transform in the BytecodeGenerator instead.

Bug: v8:11573, v8:11558, v8:5690
Change-Id: I56c90a2036fe5b43e0897c57766f666bf72bc3a8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2765783
Auto-Submit: Shu-yu Guo <syg@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#73534}

Refs: v8/v8@fd75c97

@github-actions github-actions bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Apr 28, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 28, 2021 •
edited by targos
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/37778/
V8: https://ci.nodejs.org/job/node-test-commit-v8-linux/3962/

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member Author

targos commented Apr 29, 2021

ARM compilation fails systematically with 06:22:01 g++: fatal error: Killed signal terminated program cc1plus
Any ideas?

@richardlau
Copy link
Member

It's running out of memory. I logged into the machine while a build for this PR was in progress and ran top and watched the free memory decrease. This is what it is within minutes after the g++: fatal error: Killed signal terminated program cc1plus error while the job is in the process of exiting:

Tasks: 920 total,  46 running, 873 sleeping,   0 stopped,   1 zombie
%Cpu(s): 13.3 us,  1.1 sy,  0.0 ni, 64.2 id, 21.4 wa,  0.0 hi,  0.0 si,  0.0 st
KiB Mem : 31255808 total,   575424 free, 28573120 used,  2107264 buff/cache

I saw the free value drop to something like 74360 before going back up.

I'm not sure why that would be the case for this PR (other runs for other PRs and the daily masters are passing). FWIW I even forced a run on the other centos7-arm64-gcc8 host, which failed in the same way:
https://ci.nodejs.org/job/node-test-commit-arm/nodes=centos7-arm64-gcc8/37274/

@richardlau
Copy link
Member

FWIW, when idle, top shows:

Tasks: 781 total,   1 running, 778 sleeping,   0 stopped,   2 zombie
%Cpu(s):  0.0 us,  0.0 sy,  0.0 ni,100.0 id,  0.0 wa,  0.0 hi,  0.0 si,  0.0 st
KiB Mem : 31255808 total, 26824832 free,  2187136 used,  2243840 buff/cache

@targos
Copy link
Member Author

targos commented Apr 29, 2021

Is GCC up to date ? (In case it's a compiler bug)

@richardlau
Copy link
Member

The machines are running 8.3.1 from devtoolset-8. I'll check if there are any updates.

@richardlau
Copy link
Member

No available updates for gcc (the only available updates are to Java and nettle).

@targos
deps: V8: cherry-pick fd75c97d3f56
a72b7e0 
Original commit message:

    [interpreter] Apply Reflect.apply transform in BytecodeGenerator

    Calls with a spread expression in a non-final position get transformed
    to calls to Reflect.apply. This transformation is currently done in
    the parser, which does not compose well with other features (e.g.
    direct eval checking, optional chaining).

    Do this transform in the BytecodeGenerator instead.

    Bug: v8:11573, v8:11558, v8:5690
    Change-Id: I56c90a2036fe5b43e0897c57766f666bf72bc3a8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2765783
    Auto-Submit: Shu-yu Guo <syg@chromium.org>
    Commit-Queue: Leszek Swirski <leszeks@chromium.org>
    Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
    Reviewed-by: Leszek Swirski <leszeks@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#73534}

Refs: v8/v8@fd75c97
@targos targos force-pushed the fix-v8-spread-bug branch from 216217f to a72b7e0 Compare May 1, 2021 13:13
@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member Author

targos commented May 1, 2021

FWIW I see this in the logs: make run-ci -j 50 Isn't a parallelism of 50 a bit much?

@richardlau
Copy link
Member

FWIW I see this in the logs: make run-ci -j 50 Isn't a parallelism of 50 a bit much?

🤷 The machines each have 96 cores and 25G of memory. Our ansible scripts have them set up for 50 "jobs" -- it hasn't been an issue before.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member Author

targos commented May 2, 2021

Landed in 69c57e9

targos added a commit that referenced this pull request May 2, 2021
@targos
deps: V8: cherry-pick fd75c97d3f56
69c57e9 
Original commit message:

    [interpreter] Apply Reflect.apply transform in BytecodeGenerator

    Calls with a spread expression in a non-final position get transformed
    to calls to Reflect.apply. This transformation is currently done in
    the parser, which does not compose well with other features (e.g.
    direct eval checking, optional chaining).

    Do this transform in the BytecodeGenerator instead.

    Bug: v8:11573, v8:11558, v8:5690
    Change-Id: I56c90a2036fe5b43e0897c57766f666bf72bc3a8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2765783
    Auto-Submit: Shu-yu Guo <syg@chromium.org>
    Commit-Queue: Leszek Swirski <leszeks@chromium.org>
    Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
    Reviewed-by: Leszek Swirski <leszeks@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#73534}

Refs: v8/v8@fd75c97

PR-URL: #38455
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targos targos closed this May 2, 2021
@targos targos deleted the fix-v8-spread-bug branch May 2, 2021 07:06
targos added a commit that referenced this pull request May 3, 2021
@targos
deps: V8: cherry-pick fd75c97d3f56
e3b75cb 
Original commit message:

    [interpreter] Apply Reflect.apply transform in BytecodeGenerator

    Calls with a spread expression in a non-final position get transformed
    to calls to Reflect.apply. This transformation is currently done in
    the parser, which does not compose well with other features (e.g.
    direct eval checking, optional chaining).

    Do this transform in the BytecodeGenerator instead.

    Bug: v8:11573, v8:11558, v8:5690
    Change-Id: I56c90a2036fe5b43e0897c57766f666bf72bc3a8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2765783
    Auto-Submit: Shu-yu Guo <syg@chromium.org>
    Commit-Queue: Leszek Swirski <leszeks@chromium.org>
    Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
    Reviewed-by: Leszek Swirski <leszeks@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#73534}

Refs: v8/v8@fd75c97

PR-URL: #38455
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targos targos mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@targos @nodejs-github-bot @richardlau @jasnell @cjihrig @VoltrexKeyva