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.

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

[v8.x] Fix abi break #24499

Closed
wants to merge 2 commits into from
Closed

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Nov 20, 2018

This addresses the inadvertent ABI-incompatible change in #21269. The first commit reverts the ABI breaking part of d868eb7. Unfortunately doing so will may reintroduce a memory bug (#21021). This is where the second commit adds back the functionality in an ABI-safe way and then uses it from within core.

Native modules that use AdjustAmountOfExternalAllocatedMemory may still miss the CheckMemoryPressure path. This is unfortunate, but there is no easy way to do so in an ABI compatible way. Going forward, it would be good if V8 was to avoid implementation in the v8.h file as that can lead to ABI issues like this where we can't fix the memory issue in an ABI compatible way. Such modules can use the 8.x specific AdjustAmountOfExternalAllocatedMemoryCustom, but would require a recompilation.

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

CI: https://ci.nodejs.org/job/node-test-pull-request/18778/
V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1887/

d868eb7 ended up being an inadvertant ABI change. Remove the call to
CheckMemoryPressure from the header itself.
@nodejs-github-bot
Copy link
Collaborator

@ofrobots sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v8.x labels Nov 20, 2018
CheckMemoryPressure cannot be used ABI-safely from v8.h. Add a alternate
implementation of AdjustAmountOfExternalAllocatedMemory and then use
that from Node.
@refack
Copy link
Contributor

refack commented Nov 20, 2018

@addaleax addaleax changed the title Fix abi break [v8.x] Fix abi break Nov 20, 2018
@MylesBorins
Copy link
Contributor

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

I think we should rush this into today's release if the CI is green /cc @BethGriggs

@MylesBorins MylesBorins mentioned this pull request Nov 20, 2018
@refack
Copy link
Contributor

refack commented Nov 20, 2018

Both CI jobs are ✔️

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

MylesBorins pushed a commit that referenced this pull request Nov 20, 2018
d868eb7 ended up being an inadvertant ABI change. Remove the call to
CheckMemoryPressure from the header itself.

PR-URL: #24499
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 20, 2018
CheckMemoryPressure cannot be used ABI-safely from v8.h. Add a alternate
implementation of AdjustAmountOfExternalAllocatedMemory and then use
that from Node.

PR-URL: #24499
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@MylesBorins
Copy link
Contributor

landed in 62dd1d7...0157e3e

@ofrobots
Copy link
Contributor Author

ofrobots commented Nov 20, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants