Skip to content
This repository has been archived by the owner on Mar 25, 2018. It is now read-only.

Make V8 5.5 ABI compatible with 5.4 #2

Closed
targos opened this issue Nov 5, 2016 · 6 comments
Closed

Make V8 5.5 ABI compatible with 5.4 #2

targos opened this issue Nov 5, 2016 · 6 comments

Comments

@targos
Copy link
Member

targos commented Nov 5, 2016

It seems that there are not a lot of things to do to achieve this goal.

I compared the headers of 5.5 and 5.4 with abi-compliance-checker and the report can be viewed at https://direct.lactame.com/files/nodejs/v8/compat_report.html.

I already fixed the kJSApiObjectType and kJSObjectType values (see this commit) and now I wonder if anything else is needed.
Also, do we only care about the changes in v8.h ?

/cc @nodejs/v8

@matthewloring
Copy link

The ABI patch for 5.0/5.1 fixed all incompatibilities in the include directory (v8-debug.h, v8-platform.h, etc).

@rvagg
Copy link
Member

rvagg commented Nov 8, 2016

@nodejs/ctc we need to have further discussion about this because of async/await. @ofrobots raised this at the last meeting - V8 doesn't yet have hooks for proper Promise inspection with AsyncHooks et. al. but exposing async/await is going to drive much heavier adoption of native Promises. Is this a situation we are comfortable with in the short term while we wait for deeper hooks or do we want to postpone until we have the tools we need to provide proper runtime inspection?

FYI if you didn't realise already, 5.5 is the one that unflags async/await, it's really the only major JS feature.

@bnoordhuis
Copy link
Member

bnoordhuis commented Nov 8, 2016

I already fixed the kJSApiObjectType and kJSObjectType values (see this commit) and now I wonder if anything else is needed.

IndexedPropertyHandlerConfiguration and NamedPropertyHandlerConfiguration picked up extra fields.

EmbedderHeapTracer has new virtual methods. I would expect that to change the vtable of derived classes but abi-compliance-checker doesn't seem to think it's an issue.

@targos
Copy link
Member Author

targos commented Nov 10, 2016

EmbedderHeapTracer has new virtual methods

What would be the way to handle it?

Re async/await: We could float a patch to keep it behind a flag until we decide it's ready for production.

@bnoordhuis
Copy link
Member

You could try reverting it to the old API but in the specific case of EmbedderHeapTracer I'm not sure if it's worth the hassle. It's only useful to embedders (i.e., node.js, but we don't use it), it's not something add-ons would or should use. If a tree falls and no one is around to hear it...

@targos
Copy link
Member Author

targos commented Nov 15, 2016

Thanks. I'll move the discussion to a PR where the changes can be more easily seen

@targos targos closed this as completed Nov 15, 2016
targos pushed a commit that referenced this issue Nov 17, 2016
Revision: 8f22fce

BUG=chromium:662674
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
[email protected]

Review URL: https://codereview.chromium.org/2490023004 .

Cr-Commit-Position: refs/branch-heads/5.4@{v8#81}
Cr-Branched-From: 5ce2827-refs/heads/5.4.500@{#2}
Cr-Branched-From: ad07b49-refs/heads/master@{#38841}
targos pushed a commit that referenced this issue Nov 17, 2016
Cr-Commit-Position: refs/branch-heads/5.4@{v8#82}
Cr-Branched-From: 5ce2827-refs/heads/5.4.500@{#2}
Cr-Branched-From: ad07b49-refs/heads/master@{#38841}
targos pushed a commit that referenced this issue Nov 17, 2016
Merged: Add test for making private symbols non-enumerable
Revision: 942604d

Merged: Make private symbols non-enumerable
Revision: 135b9f9

BUG=chromium:664411,chromium:664411
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
[email protected]

Review URL: https://codereview.chromium.org/2497213003 .

Cr-Commit-Position: refs/branch-heads/5.4@{v8#83}
Cr-Branched-From: 5ce2827-refs/heads/5.4.500@{#2}
Cr-Branched-From: ad07b49-refs/heads/master@{#38841}
targos pushed a commit that referenced this issue Nov 17, 2016
Cr-Commit-Position: refs/branch-heads/5.4@{v8#84}
Cr-Branched-From: 5ce2827-refs/heads/5.4.500@{#2}
Cr-Branched-From: ad07b49-refs/heads/master@{#38841}
targos pushed a commit that referenced this issue Nov 17, 2016
#2 id:20001 of https://codereview.chromium.org/2480223002/ )

Reason for revert:
http://build.chromium.org/p/client.v8/builders/V8%20Linux%20gcc%204.8/builds/9724

Original issue's description:
> [debugger] Migrate more debugger tests to inspector
>
> This moves all tests currently working with the inspector debugger wrapper to
> test/debugger.
>
> BUG=v8:5530

[email protected]
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:5530

Review-Url: https://codereview.chromium.org/2480283002
Cr-Commit-Position: refs/heads/master@{#40805}
targos pushed a commit that referenced this issue Nov 17, 2016
…set #2 id:20001 of https://codereview.chromium.org/2495213003/ )

Reason for revert:
Breaks https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20vtunejit/builds/14690 compile

Original issue's description:
> [serializer] print use count of external references.
>
> [email protected], [email protected]
> BUG=chromium:617892
> NOPRESUBMIT=true

[email protected],[email protected],[email protected]
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=chromium:617892

Review-Url: https://codereview.chromium.org/2498163003
Cr-Commit-Position: refs/heads/master@{#40980}
targos pushed a commit that referenced this issue Nov 19, 2016
Cr-Commit-Position: refs/branch-heads/5.6@{#2}
Cr-Branched-From: bdd3886-refs/heads/5.6.326@{#1}
Cr-Branched-From: 879f659-refs/heads/master@{#41014}
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants