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

[v12.x] deps: V8: backport 07ee86a5a28b #32619

Closed
wants to merge 6 commits into from
Closed

[v12.x] deps: V8: backport 07ee86a5a28b #32619

wants to merge 6 commits into from

Conversation

miladfarca
Copy link
Contributor

@miladfarca miladfarca commented Apr 2, 2020 •
edited
Loading

Original commit message:

PPC: allow for calling CFunctions without function descriptors on AIX.

The calling conventions on AIX uses function descriptors,
which means that pointers to functions do not point to code,
but instead point to metadata about them. When calling JITed code,
we must assure to use function descriptors instead of raw pointers when
needed. Before this CL 213504b, all CallCFunction on AIX were guaranteed to have
function descriptors. Starting form the CL mentioned above, CallCFunction can also
Jump to a Trampoline which does not have a function descriptor, hence a new
"CallCFunctionWithoutFunctionDescriptor" method is proposed to deal with this issue.

BUG= v8:9766

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

Refs: v8/v8@07ee86a

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

deps: V8: backport 07ee86a5a28b
ee5d4da 
Original commit message:

    PPC: allow for calling CFunctions without function descriptors on AIX.

    The calling conventions on AIX uses function descriptors,
    which means that pointers to functions do not point to code,
    but instead point to metadata about them. When calling JITed code,
    we must assure to use function descriptors instead of raw pointers when
    needed. Before this CL 213504b, all CallCFunction on AIX were guaranteed to have
    function descriptors. Starting form the CL mentioned above, CallCFunction can also
    Jump to a Trampoline which does not have a function descriptor, hence a new
    "CallCFunctionWithoutFunctionDescriptor" method is proposed to deal with this issue.

    BUG= v8:9766

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

Refs: v8/v8@07ee86a
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v12.x v8 engine Issues and PRs related to the V8 dependency. labels Apr 2, 2020
@miladfarca miladfarca mentioned this pull request Apr 2, 2020
Closed
@targos targos changed the title deps: V8: backport 07ee86a5a28b [v12.x] deps: V8: backport 07ee86a5a28b Apr 2, 2020
@nodejs-github-bot
Copy link
Collaborator

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

richardlau
richardlau approved these changes Apr 2, 2020
View reviewed changes
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

RSLGTM. I've added a commit to undo the skip of the --jitless test on AIX from #32594.

@miladfarca
Copy link
Contributor Author

@richardlau Could you please start another pr test

mhdawson
mhdawson approved these changes Apr 2, 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.

Rubber Stamp LGTM

@nodejs-github-bot
Copy link
Collaborator

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

@richardlau
Copy link
Member

@richardlau Could you please start another pr test

GitHub has been a bit unstable this evening. Will wait a bit for things to stabilise.

@miladfarca
Copy link
Contributor Author

@richardlau sounds good, seems like one of my commits still hasn't showed up.

bnoordhuis
bnoordhuis approved these changes Apr 2, 2020
View reviewed changes
deps/v8/src/common/globals.h
@@ -404,6 +404,7 @@ enum TypeofMode : int { INSIDE_TYPEOF, NOT_INSIDE_TYPEOF };
// Enums used by CEntry.
enum SaveFPRegsMode { kDontSaveFPRegs, kSaveFPRegs };
enum ArgvMode { kArgvOnStack, kArgvInRegister };
enum FunctionDescriptorMode { kNoFunctionDescriptor, kHasFunctionDescriptor };
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this have been enum FunctionDescriptorMode : bool { /* ... */ }, or (probably more idiomatic for V8) use the enum instead of bool? Relying on int-to-bool coercion looks a bit footgunny.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm guessing they were ok with it as it's following the previous enums SaveFPRegsMode and ArgvMode.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but those are of type SaveFPRegsMode and ArgvMode in function parameter lists, there's no coercion to bool taking place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's right however the int value 0 (kNoFunctionDescriptor) is implicitly translating to false at all cases, and true otherwise. You're right that marking it as bool could have made it more clear.

@MylesBorins
Copy link
Contributor

V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/2994/

@miladfarca
Copy link
Contributor Author

Should be safe to run tests now, files are synced.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@miladfarca
Copy link
Contributor Author

FYI, implementation under src/compiler/backend/ppc/code-generator-ppc.cc and src/execution/simulator.h had to be slightly modified from the original CL as certain changes are not present in this version if V8.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 3, 2020 •
edited by MylesBorins
Loading

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

MylesBorins pushed a commit that referenced this pull request Apr 3, 2020
@MylesBorins
deps: V8: backport 07ee86a5a28b
a1be327 
Original commit message:

    PPC: allow for calling CFunctions without function descriptors on AIX.

    The calling conventions on AIX uses function descriptors,
    which means that pointers to functions do not point to code,
    but instead point to metadata about them. When calling JITed code,
    we must assure to use function descriptors instead of raw pointers when
    needed. Before this CL 213504b, all CallCFunction on AIX were guaranteed to have
    function descriptors. Starting form the CL mentioned above, CallCFunction can also
    Jump to a Trampoline which does not have a function descriptor, hence a new
    "CallCFunctionWithoutFunctionDescriptor" method is proposed to deal with this issue.

    BUG= v8:9766

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

Refs: v8/v8@07ee86a

PR-URL: #32619
Refs: v8/v8@07ee86a
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Apr 3, 2020
@richardlau @MylesBorins
test: restore --jitless test on AIX
e685f12 
PR-URL: #32619
Refs: v8/v8@07ee86a
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins
Copy link
Contributor

landed in e88960d...e685f12

@MylesBorins MylesBorins closed this Apr 3, 2020
@codebytere codebytere mentioned this pull request Apr 4, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@mhdawson mhdawson mhdawson approved these changes

@richardlau richardlau richardlau approved these changes

Assignees
No one assigned
Labels
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.
6 participants
@miladfarca @nodejs-github-bot @richardlau @MylesBorins @bnoordhuis @mhdawson