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: cherry-pick 46c4979e86 from upstream v8 #18920

Closed
wants to merge 2 commits into from
Closed

deps: cherry-pick 46c4979e86 from upstream v8 #18920

wants to merge 2 commits into from

Conversation

bnoordhuis
Copy link
Member

Original commit message:

Use wider types for max_old_space_size and co.

Make --max_old_space_size and friends work with values >= 2**31.
Such values did not work reliably (or sometimes not all) due to
signed integer overflow in size computations, which is UB.

Fixes #18786.

Bug: chromium:814138
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: Ibe23cef2417fd5b4a727022b8b0d4b50f1417182
Reviewed-on: https://chromium-review.googlesource.com/927063
Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51433}

Fixes: #18786

The second commit restores ABI for v8::ResourceConstraints. Not strictly needed on master but important for back-porting.

@bnoordhuis
deps: cherry-pick 46c4979e86 from upstream v8
0f20724 
Original commit message:

    Use wider types for max_old_space_size and co.

    Make --max_old_space_size and friends work with values >= 2**31.
    Such values did not work reliably (or sometimes not all) due to
    signed integer overflow in size computations, which is UB.

    Fixes nodejs#18786.

    Bug: chromium:814138
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: Ibe23cef2417fd5b4a727022b8b0d4b50f1417182
    Reviewed-on: https://chromium-review.googlesource.com/927063
    Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#51433}

Fixes: nodejs#18786
@bnoordhuis
squash! restore abi compat
a68a1db
@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 Feb 21, 2018
@BridgeAR BridgeAR added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 22, 2018
@BridgeAR
Copy link
Member

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

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 22, 2018
@bnoordhuis @BridgeAR
deps: cherry-pick 46c4979e86 from upstream v8
13cb056 
Original commit message:

    Use wider types for max_old_space_size and co.

    Make --max_old_space_size and friends work with values >= 2**31.
    Such values did not work reliably (or sometimes not all) due to
    signed integer overflow in size computations, which is UB.

    Fixes nodejs#18786.

    Bug: chromium:814138
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: Ibe23cef2417fd5b4a727022b8b0d4b50f1417182
    Reviewed-on: https://chromium-review.googlesource.com/927063
    Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#51433}

PR-URL: nodejs#18920
Fixes: nodejs#18786
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR
Copy link
Member

Landed in 13cb056 🎉

@BridgeAR BridgeAR closed this Feb 22, 2018
@targos
Copy link
Member

targos commented Feb 22, 2018

V8 CI to make sure it's not broken: https://ci.nodejs.org/job/node-test-commit-v8-linux/1231/

MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
@bnoordhuis @MylesBorins
deps: cherry-pick 46c4979e86 from upstream v8
502bb86 
Original commit message:

    Use wider types for max_old_space_size and co.

    Make --max_old_space_size and friends work with values >= 2**31.
    Such values did not work reliably (or sometimes not all) due to
    signed integer overflow in size computations, which is UB.

    Fixes #18786.

    Bug: chromium:814138
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: Ibe23cef2417fd5b4a727022b8b0d4b50f1417182
    Reviewed-on: https://chromium-review.googlesource.com/927063
    Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#51433}

PR-URL: #18920
Fixes: #18786
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Mar 6, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
@bnoordhuis @MylesBorins
deps: cherry-pick 46c4979e86 from upstream v8
d3d1719 
Original commit message:

    Use wider types for max_old_space_size and co.

    Make --max_old_space_size and friends work with values >= 2**31.
    Such values did not work reliably (or sometimes not all) due to
    signed integer overflow in size computations, which is UB.

    Fixes #18786.

    Bug: chromium:814138
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: Ibe23cef2417fd5b4a727022b8b0d4b50f1417182
    Reviewed-on: https://chromium-review.googlesource.com/927063
    Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#51433}

PR-URL: #18920
Fixes: #18786
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
@bnoordhuis @MylesBorins
deps: cherry-pick 46c4979e86 from upstream v8
818c2fb 
Original commit message:

    Use wider types for max_old_space_size and co.

    Make --max_old_space_size and friends work with values >= 2**31.
    Such values did not work reliably (or sometimes not all) due to
    signed integer overflow in size computations, which is UB.

    Fixes #18786.

    Bug: chromium:814138
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: Ibe23cef2417fd5b4a727022b8b0d4b50f1417182
    Reviewed-on: https://chromium-review.googlesource.com/927063
    Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#51433}

PR-URL: #18920
Fixes: #18786
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
@bnoordhuis @MylesBorins
deps: cherry-pick 46c4979e86 from upstream v8
819ff1a 
Original commit message:

    Use wider types for max_old_space_size and co.

    Make --max_old_space_size and friends work with values >= 2**31.
    Such values did not work reliably (or sometimes not all) due to
    signed integer overflow in size computations, which is UB.

    Fixes #18786.

    Bug: chromium:814138
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: Ibe23cef2417fd5b4a727022b8b0d4b50f1417182
    Reviewed-on: https://chromium-review.googlesource.com/927063
    Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#51433}

PR-URL: #18920
Fixes: #18786
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
Copy link
Contributor

We ended up having to back this out of 9.x as it was breaking the V8 CI

Looks like the error is:

20:38:11 ../src/api.cc:892:26: error: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int' [-Werror,-Wshorten-64-to-32]
20:38:11   set_max_old_space_size(i::Heap::ComputeMaxOldGenerationSize(physical_memory));
20:38:11   ~~~~~~~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
failing on Ubuntu 16.04, so should be easy to reproduce.

Would someone be willing to backport?

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@bnoordhuis @MayaLekova
deps: cherry-pick 46c4979e86 from upstream v8
b3f4145 
Original commit message:

    Use wider types for max_old_space_size and co.

    Make --max_old_space_size and friends work with values >= 2**31.
    Such values did not work reliably (or sometimes not all) due to
    signed integer overflow in size computations, which is UB.

    Fixes nodejs#18786.

    Bug: chromium:814138
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: Ibe23cef2417fd5b4a727022b8b0d4b50f1417182
    Reviewed-on: https://chromium-review.googlesource.com/927063
    Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#51433}

PR-URL: nodejs#18920
Fixes: nodejs#18786
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
Copy link
Contributor

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hashseed hashseed hashseed approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 48 hours to land. 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.

None yet
6 participants
@bnoordhuis @BridgeAR @targos @MylesBorins @hashseed @nodejs-github-bot