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: re-enable interpreted frames native stack on S390 #33702

Closed
wants to merge 2 commits into from
Closed

deps: V8: re-enable interpreted frames native stack on S390 #33702

wants to merge 2 commits into from

Conversation

miladfarca
Copy link
Contributor

@miladfarca miladfarca commented Jun 2, 2020 •
edited
Loading

Refs: v8/v8@b5939c7, v8/v8@4e1bf2b

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Milad Farazmand added 2 commits June 2, 2020 18:06
deps: V8: cherry-pick b5939c758924
90fc5fe 
Original commit message:

    Revert "s390: [arm] Add missing RELATIVE_CODE_TARGET iteration"

    This reverts commit 9d3cca1cd3ad7c6653cab1cdf111d356f33f77cd.

    Reason for revert: Only the test needs to be skipped on s390. Refer to this: https://crrev.com/c/1981505

    Original change's description:
    > s390: [arm] Add missing RELATIVE_CODE_TARGET iteration
    >
    > Port b766299d2c382cc9817e73225bbebe29ce62b9d1
    > Port 9592b043eed86db91a441d4bf78b7f0c8c2ce4dd
    > Port d915b8d668615a7d6d75cf7a61d3ca5a3d139799
    >
    > Original Commit Message:
    >
    >     Code object iteration was missing logic for RELATIVE_CODE_TARGET
    >     reloc entries. Garbage collection could thus miss objects that were
    >     referenced only as targets of pc-relative calls or jumps.
    >
    >     RELATIVE_CODE_TARGETs are only used on arm, mips, and s390 and only
    >     at mksnapshot-time.
    >
    >     This exposed another issue in that the interpreter entry trampoline
    >     copy we generate for profiling *did* contain relative calls in
    >     runtime-accessible code. This is a problem, since code space on arm is,
    >     by default, too large to be fully addressable through pc-relative
    >     calls. This CL thus also disables the related
    >     FLAG_interpreted_frames_native_stack feature on arm.
    >
    >     objects.
    >
    > R=​jgruber@chromium.org, joransiu@ca.ibm.com, jyan@ca.ibm.com, michael_dawson@ca.ibm.com
    > BUG=
    > LOG=N
    >
    > Change-Id: Ifbcaed98d90a2730f0d6a8a7d32c621dab1ff5b2
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2087693
    > Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    > Reviewed-by: Junliang Yan <jyan@ca.ibm.com>
    > Commit-Queue: Milad Farazmand <miladfar@ca.ibm.com>
    > Cr-Commit-Position: refs/heads/master@{#66644}

    TBR=michael_dawson@ca.ibm.com,mlippautz@chromium.org,jyan@ca.ibm.com,jgruber@chromium.org,joransiu@ca.ibm.com,miladfar@ca.ibm.com

    # Not skipping CQ checks because original CL landed > 1 day ago.

    Change-Id: Id645a9def23d278235ff77f25249d2187e8105ca
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2196521
    Reviewed-by: Milad Farazmand <miladfar@ca.ibm.com>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Commit-Queue: Milad Farazmand <miladfar@ca.ibm.com>
    Cr-Commit-Position: refs/heads/master@{#67751}

Refs: v8/v8@b5939c7
deps: V8: cherry-pick 4e1bf2bc92bd
e0875fc 
Original commit message:

    Skip InterpreterWithNativeStack on jitless mode

    As discussed under https://crrev.com/c/1981505,
    Test requires an executable CODE_SPACE and is thus incompatible with
    jitless mode.

    Change-Id: Icddad50a3484f0cfc5fb4abd7175058d50bc06d3
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2193911
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Commit-Queue: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67726}

Refs: v8/v8@4e1bf2b
@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 Jun 2, 2020
@miladfarca miladfarca mentioned this pull request Jun 2, 2020
Closed
5 tasks
@miladfarca
Copy link
Contributor Author

@targos Would you please help review this PR, thank you.

@targos targos added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 6, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 6, 2020 •
edited by richardlau
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/31749/ (💛)
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/3164/ (✔️)

mhdawson
mhdawson approved these changes Jun 8, 2020
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@miladfarca
Copy link
Contributor Author

@richardlau should we kick another test as above links seem to be inactive?

@richardlau
Copy link
Member

@richardlau should we kick another test as above links seem to be inactive?

The links are okay if you're logged into the CI. There was some blip on GitHub's side last week re. logins (nodejs/build#2338) and since then sessions seem to be expiring much quicker than before.

@miladfarca
Copy link
Contributor Author

@richardlau Tried log in again, even different browsers, still getting the same 404 error.

@richardlau
Copy link
Member

@richardlau Tried log in again, even different browsers, still getting the same 404 error.

@miladfarca Can you try again? The "job read" permission was missing for "authenticated users" (but was set for collaborators) and I've just set it.

@miladfarca
Copy link
Contributor Author

@richardlau perfect, now I can see them thank you.

richardlau pushed a commit to richardlau/node-1 that referenced this pull request Jun 8, 2020
@richardlau
deps: V8: cherry-pick b5939c758924
de501d2 
Original commit message:

    Revert "s390: [arm] Add missing RELATIVE_CODE_TARGET iteration"

    This reverts commit 9d3cca1cd3ad7c6653cab1cdf111d356f33f77cd.

    Reason for revert: Only the test needs to be skipped on s390. Refer to this: https://crrev.com/c/1981505

    Original change's description:
    > s390: [arm] Add missing RELATIVE_CODE_TARGET iteration
    >
    > Port b766299d2c382cc9817e73225bbebe29ce62b9d1
    > Port 9592b043eed86db91a441d4bf78b7f0c8c2ce4dd
    > Port d915b8d668615a7d6d75cf7a61d3ca5a3d139799
    >
    > Original Commit Message:
    >
    >     Code object iteration was missing logic for RELATIVE_CODE_TARGET
    >     reloc entries. Garbage collection could thus miss objects that were
    >     referenced only as targets of pc-relative calls or jumps.
    >
    >     RELATIVE_CODE_TARGETs are only used on arm, mips, and s390 and only
    >     at mksnapshot-time.
    >
    >     This exposed another issue in that the interpreter entry trampoline
    >     copy we generate for profiling *did* contain relative calls in
    >     runtime-accessible code. This is a problem, since code space on arm is,
    >     by default, too large to be fully addressable through pc-relative
    >     calls. This CL thus also disables the related
    >     FLAG_interpreted_frames_native_stack feature on arm.
    >
    >     objects.
    >
    > R=​jgruber@chromium.org, joransiu@ca.ibm.com, jyan@ca.ibm.com, michael_dawson@ca.ibm.com
    > BUG=
    > LOG=N
    >
    > Change-Id: Ifbcaed98d90a2730f0d6a8a7d32c621dab1ff5b2
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2087693
    > Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    > Reviewed-by: Junliang Yan <jyan@ca.ibm.com>
    > Commit-Queue: Milad Farazmand <miladfar@ca.ibm.com>
    > Cr-Commit-Position: refs/heads/master@{#66644}

    TBR=michael_dawson@ca.ibm.com,mlippautz@chromium.org,jyan@ca.ibm.com,jgruber@chromium.org,joransiu@ca.ibm.com,miladfar@ca.ibm.com

    # Not skipping CQ checks because original CL landed > 1 day ago.

    Change-Id: Id645a9def23d278235ff77f25249d2187e8105ca
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2196521
    Reviewed-by: Milad Farazmand <miladfar@ca.ibm.com>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Commit-Queue: Milad Farazmand <miladfar@ca.ibm.com>
    Cr-Commit-Position: refs/heads/master@{#67751}

Refs: v8/v8@b5939c7

PR-URL: nodejs#33702
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
richardlau pushed a commit to richardlau/node-1 that referenced this pull request Jun 8, 2020
@richardlau
deps: V8: cherry-pick 4e1bf2bc92bd
651088c 
Original commit message:

    Skip InterpreterWithNativeStack on jitless mode

    As discussed under https://crrev.com/c/1981505,
    Test requires an executable CODE_SPACE and is thus incompatible with
    jitless mode.

    Change-Id: Icddad50a3484f0cfc5fb4abd7175058d50bc06d3
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2193911
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Commit-Queue: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67726}

Refs: v8/v8@4e1bf2b

PR-URL: nodejs#33702
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@richardlau
Copy link
Member

Landed in 689680a...651088c.

@richardlau richardlau closed this Jun 8, 2020
@miladfarca
Copy link
Contributor Author

@targos Is it possible to re-enable this test on s390: #32831 (comment)

@richardlau richardlau mentioned this pull request Jun 8, 2020
Merged
targos added a commit to targos/node that referenced this pull request Jun 8, 2020
@targos
Revert "test: stop testing --interpreted-frames-native-stack for s390x"
805c172 
This reverts commit 38fd93c.

Refs: nodejs#33702
@targos targos mentioned this pull request Jun 8, 2020
Closed
@targos
Copy link
Member

targos commented Jun 8, 2020

@miladfarca : #33794

targos added a commit that referenced this pull request Jun 12, 2020
@targos
Revert "test: stop testing --interpreted-frames-native-stack for s390x"
76ceaff 
This reverts commit 38fd93c.

Refs: #33702

PR-URL: #33794
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
codebytere pushed a commit that referenced this pull request Jun 18, 2020
@codebytere
deps: V8: cherry-pick b5939c758924
0524c7a 
Original commit message:

    Revert "s390: [arm] Add missing RELATIVE_CODE_TARGET iteration"

    This reverts commit 9d3cca1cd3ad7c6653cab1cdf111d356f33f77cd.

    Reason for revert: Only the test needs to be skipped on s390. Refer to this: https://crrev.com/c/1981505

    Original change's description:
    > s390: [arm] Add missing RELATIVE_CODE_TARGET iteration
    >
    > Port b766299d2c382cc9817e73225bbebe29ce62b9d1
    > Port 9592b043eed86db91a441d4bf78b7f0c8c2ce4dd
    > Port d915b8d668615a7d6d75cf7a61d3ca5a3d139799
    >
    > Original Commit Message:
    >
    >     Code object iteration was missing logic for RELATIVE_CODE_TARGET
    >     reloc entries. Garbage collection could thus miss objects that were
    >     referenced only as targets of pc-relative calls or jumps.
    >
    >     RELATIVE_CODE_TARGETs are only used on arm, mips, and s390 and only
    >     at mksnapshot-time.
    >
    >     This exposed another issue in that the interpreter entry trampoline
    >     copy we generate for profiling *did* contain relative calls in
    >     runtime-accessible code. This is a problem, since code space on arm is,
    >     by default, too large to be fully addressable through pc-relative
    >     calls. This CL thus also disables the related
    >     FLAG_interpreted_frames_native_stack feature on arm.
    >
    >     objects.
    >
    > R=​jgruber@chromium.org, joransiu@ca.ibm.com, jyan@ca.ibm.com, michael_dawson@ca.ibm.com
    > BUG=
    > LOG=N
    >
    > Change-Id: Ifbcaed98d90a2730f0d6a8a7d32c621dab1ff5b2
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2087693
    > Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    > Reviewed-by: Junliang Yan <jyan@ca.ibm.com>
    > Commit-Queue: Milad Farazmand <miladfar@ca.ibm.com>
    > Cr-Commit-Position: refs/heads/master@{#66644}

    TBR=michael_dawson@ca.ibm.com,mlippautz@chromium.org,jyan@ca.ibm.com,jgruber@chromium.org,joransiu@ca.ibm.com,miladfar@ca.ibm.com

    # Not skipping CQ checks because original CL landed > 1 day ago.

    Change-Id: Id645a9def23d278235ff77f25249d2187e8105ca
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2196521
    Reviewed-by: Milad Farazmand <miladfar@ca.ibm.com>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Commit-Queue: Milad Farazmand <miladfar@ca.ibm.com>
    Cr-Commit-Position: refs/heads/master@{#67751}

Refs: v8/v8@b5939c7

PR-URL: #33702
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
codebytere pushed a commit that referenced this pull request Jun 18, 2020
@codebytere
deps: V8: cherry-pick 4e1bf2bc92bd
499c740 
Original commit message:

    Skip InterpreterWithNativeStack on jitless mode

    As discussed under https://crrev.com/c/1981505,
    Test requires an executable CODE_SPACE and is thus incompatible with
    jitless mode.

    Change-Id: Icddad50a3484f0cfc5fb4abd7175058d50bc06d3
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2193911
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Commit-Queue: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67726}

Refs: v8/v8@4e1bf2b

PR-URL: #33702
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
codebytere pushed a commit that referenced this pull request Jun 18, 2020
@targos @codebytere
Revert "test: stop testing --interpreted-frames-native-stack for s390x"
b8fdde4 
This reverts commit 38fd93c.

Refs: #33702

PR-URL: #33794
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere mentioned this pull request Jun 28, 2020
Merged
codebytere pushed a commit that referenced this pull request Jun 30, 2020
@codebytere
deps: V8: cherry-pick b5939c758924
8991467 
Original commit message:

    Revert "s390: [arm] Add missing RELATIVE_CODE_TARGET iteration"

    This reverts commit 9d3cca1cd3ad7c6653cab1cdf111d356f33f77cd.

    Reason for revert: Only the test needs to be skipped on s390. Refer to this: https://crrev.com/c/1981505

    Original change's description:
    > s390: [arm] Add missing RELATIVE_CODE_TARGET iteration
    >
    > Port b766299d2c382cc9817e73225bbebe29ce62b9d1
    > Port 9592b043eed86db91a441d4bf78b7f0c8c2ce4dd
    > Port d915b8d668615a7d6d75cf7a61d3ca5a3d139799
    >
    > Original Commit Message:
    >
    >     Code object iteration was missing logic for RELATIVE_CODE_TARGET
    >     reloc entries. Garbage collection could thus miss objects that were
    >     referenced only as targets of pc-relative calls or jumps.
    >
    >     RELATIVE_CODE_TARGETs are only used on arm, mips, and s390 and only
    >     at mksnapshot-time.
    >
    >     This exposed another issue in that the interpreter entry trampoline
    >     copy we generate for profiling *did* contain relative calls in
    >     runtime-accessible code. This is a problem, since code space on arm is,
    >     by default, too large to be fully addressable through pc-relative
    >     calls. This CL thus also disables the related
    >     FLAG_interpreted_frames_native_stack feature on arm.
    >
    >     objects.
    >
    > R=​jgruber@chromium.org, joransiu@ca.ibm.com, jyan@ca.ibm.com, michael_dawson@ca.ibm.com
    > BUG=
    > LOG=N
    >
    > Change-Id: Ifbcaed98d90a2730f0d6a8a7d32c621dab1ff5b2
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2087693
    > Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    > Reviewed-by: Junliang Yan <jyan@ca.ibm.com>
    > Commit-Queue: Milad Farazmand <miladfar@ca.ibm.com>
    > Cr-Commit-Position: refs/heads/master@{#66644}

    TBR=michael_dawson@ca.ibm.com,mlippautz@chromium.org,jyan@ca.ibm.com,jgruber@chromium.org,joransiu@ca.ibm.com,miladfar@ca.ibm.com

    # Not skipping CQ checks because original CL landed > 1 day ago.

    Change-Id: Id645a9def23d278235ff77f25249d2187e8105ca
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2196521
    Reviewed-by: Milad Farazmand <miladfar@ca.ibm.com>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Commit-Queue: Milad Farazmand <miladfar@ca.ibm.com>
    Cr-Commit-Position: refs/heads/master@{#67751}

Refs: v8/v8@b5939c7

PR-URL: #33702
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
codebytere pushed a commit that referenced this pull request Jun 30, 2020
@codebytere
deps: V8: cherry-pick 4e1bf2bc92bd
a96c68c 
Original commit message:

    Skip InterpreterWithNativeStack on jitless mode

    As discussed under https://crrev.com/c/1981505,
    Test requires an executable CODE_SPACE and is thus incompatible with
    jitless mode.

    Change-Id: Icddad50a3484f0cfc5fb4abd7175058d50bc06d3
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2193911
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Commit-Queue: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#67726}

Refs: v8/v8@4e1bf2b

PR-URL: #33702
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
codebytere pushed a commit that referenced this pull request Jun 30, 2020
@targos @codebytere
Revert "test: stop testing --interpreted-frames-native-stack for s390x"
bce2bba 
This reverts commit 38fd93c.

Refs: #33702

PR-URL: #33794
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
This was referenced May 22, 2023
Open
Open
Open
@137717unity 137717unity mentioned this pull request May 22, 2023
Open
@snyk-bot snyk-bot mentioned this pull request May 23, 2023
Open
@wedataintelligence wedataintelligence mentioned this pull request May 24, 2023
Open
@baby636 baby636 mentioned this pull request May 24, 2023
Open
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Nov 25, 2023
Open
@wedataintelligence wedataintelligence mentioned this pull request Nov 28, 2023
Open
@baby636 baby636 mentioned this pull request Nov 28, 2023
Open
@ron-a20 ron-a20 mentioned this pull request Dec 19, 2023
Open
@137717unity 137717unity mentioned this pull request Dec 19, 2023
Open
@wedataintelligence wedataintelligence mentioned this pull request Dec 19, 2023
Open
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Dec 19, 2023
Open
@baby636 baby636 mentioned this pull request Dec 20, 2023
Open
@137717unity 137717unity mentioned this pull request Apr 14, 2024
Open
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Apr 14, 2024
Open
@ron-a20 ron-a20 mentioned this pull request Apr 14, 2024
Open
@molllyn1 molllyn1 mentioned this pull request Apr 14, 2024
Open
@kissofire kissofire mentioned this pull request Apr 14, 2024
Open
@wedataintelligence wedataintelligence mentioned this pull request Apr 14, 2024
Open
@baby636 baby636 mentioned this pull request Apr 15, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@richardlau richardlau richardlau approved these changes

@targos targos targos 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. 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.
5 participants
@miladfarca @nodejs-github-bot @richardlau @targos @mhdawson