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

src: fix pointer compression build #50680

Closed
wants to merge 2 commits into from
Closed

src: fix pointer compression build #50680

wants to merge 2 commits into from

Conversation

kvakil
Copy link
Contributor

@kvakil kvakil commented Nov 12, 2023

These two commits combined fix the pointer compression build. The build was
broken starting in Node v21.0.0 due to upgrading past a regression first
introduced in V8 11.4. See the refs for more information.

deps: V8: cherry-pick 475c8cdf9a95

Original commit message:

[ptr-compr] Fix multi-cage mode

This CL introduces PtrComprCageAccessScope which sets/restores current
thread's pointer compression cage base values. It's supposed to be used
by V8 jobs accessing V8 heap outside of v8::Isolate::Scope or
i::LocalHeap or i::LocalIsolate scopes (they already ensure that the
cage base values are properly initialized).
For all other build modes PtrComprCageAccessScope is a no-op.

For simplicity reasons the multi-cage mode is made incompatible with
external code space.

Bug: v8:13788, v8:14292
Change-Id: I06c2d19a1eb7254fa7af07a17617e22d98abea9f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4846592
Reviewed-by: Jakob Linke <jgruber@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#90075}

Refs: v8/v8@475c8cd

src: add IsolateScopes before using isolates

The V8 API requires entering an isolate before using it. We were often
not doing this, which worked fine in practice. However when (multi-cage)
pointer compression is enabled, the correct isolate needs to be active
in order to decompress pointers correctly, otherwise it causes crashes.

Fix this by sprinkling in some calls to v8::Isolate::Scope::Scope where
they were missing.

Tested by compiling with --experimental-enable-pointer-compression
locally and running all tests.

Refs: nodejs/build#3204 (comment)
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 12, 2023
@kvakil kvakil mentioned this pull request Nov 12, 2023
Open
@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2023
@nodejs-github-bot
Copy link
Collaborator

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

bnoordhuis
bnoordhuis reviewed Nov 12, 2023
View reviewed changes
src/json_parser.h Outdated Show resolved Hide resolved
kvakil
kvakil commented Nov 12, 2023
View reviewed changes
Copy link
Contributor Author

@kvakil kvakil left a comment

Choose a reason for hiding this comment

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

@RaisinTen @bnoordhuis your two suggestions have a small logical conflict. Right now, JSONParser initializes a RAIIIsolate. If RAIIIsolate enters the Isolate then constructing a JSONParser would cause us to enter the temporary isolate while it's in scope. That would conflict with @bnoordhuis's desire that we avoid JSONParser leaking its internals into the outer scope.

I suppose the easiest way to resolve this would be to have JSONParser just use Isolate * instead of RAIIIsolate, and only enter/exit in the member functions rather than in the ctor. This is sort of logically a different change so I would do it in a separate PR.

Then we can change RAIIIsolate to just enter the isolate & possibly construct the handle scope too. That seems to fit fine in this PR.

WDYT?

src/json_parser.h Outdated Show resolved Hide resolved
src/node_sea.cc Show resolved Hide resolved
kvakil added a commit to kvakil/node that referenced this pull request Nov 12, 2023
@kvakil
src: fix JSONParser leaking internal V8 scopes
8eabb5b 
JSONParser uses V8's JSON.parse (for now), meaning that its uses handles
and contexts. JSONParser was leaking its internal HandleScope and
Context::Scope.

Move the scope construction to the member functions to prevent those
scopes from leaking. Introduce a new private inner class
HandleAndContextScope to reduce boilerplate.

Refs: nodejs#50680 (comment)
@kvakil kvakil mentioned this pull request Nov 12, 2023
Merged
kvakil added a commit to kvakil/node that referenced this pull request Nov 12, 2023
@kvakil
src: fix JSONParser leaking internal V8 scopes
bc466e6 
JSONParser uses V8's JSON.parse (for now), meaning that its uses handles
and contexts. JSONParser was leaking its internal HandleScope and
Context::Scope.

Move the scope construction to the member functions to prevent those
scopes from leaking.

Refs: nodejs#50680 (comment)
bnoordhuis
bnoordhuis approved these changes Nov 12, 2023
View reviewed changes
src/json_parser.h Outdated Show resolved Hide resolved
@bnoordhuis
Copy link
Member

and only enter/exit in the member functions rather than in the ctor [..] in a separate PR

Yes, good idea.

This was referenced Nov 13, 2023
Open
Open
RaisinTen
RaisinTen approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

Separate PR sounds good 👍

src/node_sea.cc Show resolved Hide resolved
nodejs-github-bot pushed a commit that referenced this pull request Nov 14, 2023
@kvakil
src: fix JSONParser leaking internal V8 scopes
09f4aa9 
JSONParser uses V8's JSON.parse (for now), meaning that its uses handles
and contexts. JSONParser was leaking its internal HandleScope and
Context::Scope.

Move the scope construction to the member functions to prevent those
scopes from leaking.

Refs: #50680 (comment)
PR-URL: #50688
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@github-actions github-actions bot mentioned this pull request Nov 15, 2023
Open
26 tasks
@kvakil
deps: V8: cherry-pick 475c8cdf9a95
7923103 
Original commit message:

    [ptr-compr] Fix multi-cage mode

    This CL introduces PtrComprCageAccessScope which sets/restores current
    thread's pointer compression cage base values. It's supposed to be used
    by V8 jobs accessing V8 heap outside of v8::Isolate::Scope or
    i::LocalHeap or i::LocalIsolate scopes (they already ensure that the
    cage base values are properly initialized).
    For all other build modes PtrComprCageAccessScope is a no-op.

    For simplicity reasons the multi-cage mode is made incompatible with
    external code space.

    Bug: v8:13788, v8:14292
    Change-Id: I06c2d19a1eb7254fa7af07a17617e22d98abea9f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4846592
    Reviewed-by: Jakob Linke <jgruber@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Commit-Queue: Igor Sheludko <ishell@chromium.org>
    Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#90075}

Refs: v8/v8@475c8cd
@kvakil
src: add IsolateScopes before using isolates
b2b9acb 
The V8 API requires entering an isolate before using it. We were often
not doing this, which worked fine in practice. However when (multi-cage)
pointer compression is enabled, the correct isolate needs to be active
in order to decompress pointers correctly, otherwise it causes crashes.

Fix this by sprinkling in some calls to v8::Isolate::Scope::Scope where
they were missing.

This also introduces RAIIIsolateWithoutEntering which is used in
JSONParser to avoid otherwise exposing the Isolate::Scope outside of the
class.

Tested by compiling with `--experimental-enable-pointer-compression`
locally and running all tests.

Refs: nodejs/build#3204 (comment)
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292
@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 16, 2023
kvakil
kvakil commented Nov 16, 2023
View reviewed changes
Copy link
Contributor Author

@kvakil kvakil left a comment

Choose a reason for hiding this comment

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

@bnoordhuis @RaisinTen rebased on other PR - PTAL.

src/node_sea.cc Show resolved Hide resolved
src/util.cc
@@ -692,9 +692,14 @@ RAIIIsolate::RAIIIsolate(const SnapshotData* data)
Isolate::Initialize(isolate_, params);
}

RAIIIsolate::~RAIIIsolate() {
RAIIIsolateWithoutEntering::~RAIIIsolateWithoutEntering() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This name is icky. Better suggestions are welcome - maybe RAIIIsolate needs to be renamed too?

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this special case? Isn't re-entering of isolates allowed anyways?

Copy link
Member

Choose a reason for hiding this comment

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

If this is only for the theoretical interference with outer Isolate scopes, I think we can simply CHECK_NULL(Isolate::TryGetCurrent()) in the JSONParser constructor and call it a day, or just add a comment to JSONParser "For now, don't use this when there's an entered Isolate while JSONParser is alive". The only usage of this class doesn't have outer isolate scopes and we can probably just switch to simdjson later. There's no need to make it super robust for a theoretical case that probably would not happen any time soon / will just be rewritten anyway.

joyeecheung
joyeecheung approved these changes Nov 18, 2023
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

I think it's somewhat funny that we are defending against theoretical cases for a class that only has one usage in core which doesn't exercise the case. But that's just a nit. LGTM.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

kvakil added a commit that referenced this pull request Nov 20, 2023
@kvakil
deps: V8: cherry-pick 475c8cdf9a95
ec023a7 
Original commit message:

    [ptr-compr] Fix multi-cage mode

    This CL introduces PtrComprCageAccessScope which sets/restores current
    thread's pointer compression cage base values. It's supposed to be used
    by V8 jobs accessing V8 heap outside of v8::Isolate::Scope or
    i::LocalHeap or i::LocalIsolate scopes (they already ensure that the
    cage base values are properly initialized).
    For all other build modes PtrComprCageAccessScope is a no-op.

    For simplicity reasons the multi-cage mode is made incompatible with
    external code space.

    Bug: v8:13788, v8:14292
    Change-Id: I06c2d19a1eb7254fa7af07a17617e22d98abea9f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4846592
    Reviewed-by: Jakob Linke <jgruber@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Commit-Queue: Igor Sheludko <ishell@chromium.org>
    Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#90075}

Refs: v8/v8@475c8cd
PR-URL: #50680
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
kvakil added a commit that referenced this pull request Nov 20, 2023
@kvakil
src: add IsolateScopes before using isolates
8e60189 
The V8 API requires entering an isolate before using it. We were often
not doing this, which worked fine in practice. However when (multi-cage)
pointer compression is enabled, the correct isolate needs to be active
in order to decompress pointers correctly, otherwise it causes crashes.

Fix this by sprinkling in some calls to v8::Isolate::Scope::Scope where
they were missing.

This also introduces RAIIIsolateWithoutEntering which is used in
JSONParser to avoid otherwise exposing the Isolate::Scope outside of the
class.

Tested by compiling with `--experimental-enable-pointer-compression`
locally and running all tests.

Refs: nodejs/build#3204 (comment)
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292
PR-URL: #50680
Refs: v8/v8@475c8cd
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@kvakil
Copy link
Contributor Author

kvakil commented Nov 20, 2023

Landed in ec023a7...8e60189

@kvakil kvakil closed this Nov 20, 2023
targos pushed a commit that referenced this pull request Nov 23, 2023
@kvakil @targos
src: fix JSONParser leaking internal V8 scopes
2e8e237 
JSONParser uses V8's JSON.parse (for now), meaning that its uses handles
and contexts. JSONParser was leaking its internal HandleScope and
Context::Scope.

Move the scope construction to the member functions to prevent those
scopes from leaking.

Refs: #50680 (comment)
PR-URL: #50688
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
targos pushed a commit that referenced this pull request Nov 23, 2023
@kvakil @targos
deps: V8: cherry-pick 475c8cdf9a95
6caf72c 
Original commit message:

    [ptr-compr] Fix multi-cage mode

    This CL introduces PtrComprCageAccessScope which sets/restores current
    thread's pointer compression cage base values. It's supposed to be used
    by V8 jobs accessing V8 heap outside of v8::Isolate::Scope or
    i::LocalHeap or i::LocalIsolate scopes (they already ensure that the
    cage base values are properly initialized).
    For all other build modes PtrComprCageAccessScope is a no-op.

    For simplicity reasons the multi-cage mode is made incompatible with
    external code space.

    Bug: v8:13788, v8:14292
    Change-Id: I06c2d19a1eb7254fa7af07a17617e22d98abea9f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4846592
    Reviewed-by: Jakob Linke <jgruber@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Commit-Queue: Igor Sheludko <ishell@chromium.org>
    Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#90075}

Refs: v8/v8@475c8cd
PR-URL: #50680
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Nov 23, 2023
@kvakil @targos
src: add IsolateScopes before using isolates
d4bfa39 
The V8 API requires entering an isolate before using it. We were often
not doing this, which worked fine in practice. However when (multi-cage)
pointer compression is enabled, the correct isolate needs to be active
in order to decompress pointers correctly, otherwise it causes crashes.

Fix this by sprinkling in some calls to v8::Isolate::Scope::Scope where
they were missing.

This also introduces RAIIIsolateWithoutEntering which is used in
JSONParser to avoid otherwise exposing the Isolate::Scope outside of the
class.

Tested by compiling with `--experimental-enable-pointer-compression`
locally and running all tests.

Refs: nodejs/build#3204 (comment)
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292
PR-URL: #50680
Refs: v8/v8@475c8cd
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
martenrichter pushed a commit to martenrichter/node that referenced this pull request Nov 26, 2023
@kvakil @martenrichter
src: fix JSONParser leaking internal V8 scopes
e9c6d17 
JSONParser uses V8's JSON.parse (for now), meaning that its uses handles
and contexts. JSONParser was leaking its internal HandleScope and
Context::Scope.

Move the scope construction to the member functions to prevent those
scopes from leaking.

Refs: nodejs#50680 (comment)
PR-URL: nodejs#50688
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
martenrichter pushed a commit to martenrichter/node that referenced this pull request Nov 26, 2023
@kvakil @martenrichter
deps: V8: cherry-pick 475c8cdf9a95
9770ddb 
Original commit message:

    [ptr-compr] Fix multi-cage mode

    This CL introduces PtrComprCageAccessScope which sets/restores current
    thread's pointer compression cage base values. It's supposed to be used
    by V8 jobs accessing V8 heap outside of v8::Isolate::Scope or
    i::LocalHeap or i::LocalIsolate scopes (they already ensure that the
    cage base values are properly initialized).
    For all other build modes PtrComprCageAccessScope is a no-op.

    For simplicity reasons the multi-cage mode is made incompatible with
    external code space.

    Bug: v8:13788, v8:14292
    Change-Id: I06c2d19a1eb7254fa7af07a17617e22d98abea9f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4846592
    Reviewed-by: Jakob Linke <jgruber@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Commit-Queue: Igor Sheludko <ishell@chromium.org>
    Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#90075}

Refs: v8/v8@475c8cd
PR-URL: nodejs#50680
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
martenrichter pushed a commit to martenrichter/node that referenced this pull request Nov 26, 2023
@kvakil @martenrichter
src: add IsolateScopes before using isolates
7a9d2c8 
The V8 API requires entering an isolate before using it. We were often
not doing this, which worked fine in practice. However when (multi-cage)
pointer compression is enabled, the correct isolate needs to be active
in order to decompress pointers correctly, otherwise it causes crashes.

Fix this by sprinkling in some calls to v8::Isolate::Scope::Scope where
they were missing.

This also introduces RAIIIsolateWithoutEntering which is used in
JSONParser to avoid otherwise exposing the Isolate::Scope outside of the
class.

Tested by compiling with `--experimental-enable-pointer-compression`
locally and running all tests.

Refs: nodejs/build#3204 (comment)
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292
PR-URL: nodejs#50680
Refs: v8/v8@475c8cd
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
lucshi pushed a commit to lucshi/node that referenced this pull request Nov 27, 2023
@kvakil @lucshi
deps: V8: cherry-pick 475c8cdf9a95
e91566f 
Original commit message:

    [ptr-compr] Fix multi-cage mode

    This CL introduces PtrComprCageAccessScope which sets/restores current
    thread's pointer compression cage base values. It's supposed to be used
    by V8 jobs accessing V8 heap outside of v8::Isolate::Scope or
    i::LocalHeap or i::LocalIsolate scopes (they already ensure that the
    cage base values are properly initialized).
    For all other build modes PtrComprCageAccessScope is a no-op.

    For simplicity reasons the multi-cage mode is made incompatible with
    external code space.

    Bug: v8:13788, v8:14292
    Change-Id: I06c2d19a1eb7254fa7af07a17617e22d98abea9f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4846592
    Reviewed-by: Jakob Linke <jgruber@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Commit-Queue: Igor Sheludko <ishell@chromium.org>
    Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#90075}

Refs: v8/v8@475c8cd
PR-URL: nodejs#50680
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
lucshi pushed a commit to lucshi/node that referenced this pull request Nov 27, 2023
@kvakil @lucshi
src: add IsolateScopes before using isolates
f4da4ab 
The V8 API requires entering an isolate before using it. We were often
not doing this, which worked fine in practice. However when (multi-cage)
pointer compression is enabled, the correct isolate needs to be active
in order to decompress pointers correctly, otherwise it causes crashes.

Fix this by sprinkling in some calls to v8::Isolate::Scope::Scope where
they were missing.

This also introduces RAIIIsolateWithoutEntering which is used in
JSONParser to avoid otherwise exposing the Isolate::Scope outside of the
class.

Tested by compiling with `--experimental-enable-pointer-compression`
locally and running all tests.

Refs: nodejs/build#3204 (comment)
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292
PR-URL: nodejs#50680
Refs: v8/v8@475c8cd
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Nov 28, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 29, 2023
@kvakil @RafaelGSS
deps: V8: cherry-pick 475c8cdf9a95
00dab30 
Original commit message:

    [ptr-compr] Fix multi-cage mode

    This CL introduces PtrComprCageAccessScope which sets/restores current
    thread's pointer compression cage base values. It's supposed to be used
    by V8 jobs accessing V8 heap outside of v8::Isolate::Scope or
    i::LocalHeap or i::LocalIsolate scopes (they already ensure that the
    cage base values are properly initialized).
    For all other build modes PtrComprCageAccessScope is a no-op.

    For simplicity reasons the multi-cage mode is made incompatible with
    external code space.

    Bug: v8:13788, v8:14292
    Change-Id: I06c2d19a1eb7254fa7af07a17617e22d98abea9f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4846592
    Reviewed-by: Jakob Linke <jgruber@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Commit-Queue: Igor Sheludko <ishell@chromium.org>
    Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#90075}

Refs: v8/v8@475c8cd
PR-URL: #50680
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Nov 29, 2023
@kvakil @RafaelGSS
src: add IsolateScopes before using isolates
faf6a04 
The V8 API requires entering an isolate before using it. We were often
not doing this, which worked fine in practice. However when (multi-cage)
pointer compression is enabled, the correct isolate needs to be active
in order to decompress pointers correctly, otherwise it causes crashes.

Fix this by sprinkling in some calls to v8::Isolate::Scope::Scope where
they were missing.

This also introduces RAIIIsolateWithoutEntering which is used in
JSONParser to avoid otherwise exposing the Isolate::Scope outside of the
class.

Tested by compiling with `--experimental-enable-pointer-compression`
locally and running all tests.

Refs: nodejs/build#3204 (comment)
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292
PR-URL: #50680
Refs: v8/v8@475c8cd
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Nov 30, 2023
@kvakil @RafaelGSS
deps: V8: cherry-pick 475c8cdf9a95
9aee38f 
Original commit message:

    [ptr-compr] Fix multi-cage mode

    This CL introduces PtrComprCageAccessScope which sets/restores current
    thread's pointer compression cage base values. It's supposed to be used
    by V8 jobs accessing V8 heap outside of v8::Isolate::Scope or
    i::LocalHeap or i::LocalIsolate scopes (they already ensure that the
    cage base values are properly initialized).
    For all other build modes PtrComprCageAccessScope is a no-op.

    For simplicity reasons the multi-cage mode is made incompatible with
    external code space.

    Bug: v8:13788, v8:14292
    Change-Id: I06c2d19a1eb7254fa7af07a17617e22d98abea9f
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4846592
    Reviewed-by: Jakob Linke <jgruber@chromium.org>
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Commit-Queue: Igor Sheludko <ishell@chromium.org>
    Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#90075}

Refs: v8/v8@475c8cd
PR-URL: #50680
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Nov 30, 2023
@kvakil @RafaelGSS
src: add IsolateScopes before using isolates
080f23e 
The V8 API requires entering an isolate before using it. We were often
not doing this, which worked fine in practice. However when (multi-cage)
pointer compression is enabled, the correct isolate needs to be active
in order to decompress pointers correctly, otherwise it causes crashes.

Fix this by sprinkling in some calls to v8::Isolate::Scope::Scope where
they were missing.

This also introduces RAIIIsolateWithoutEntering which is used in
JSONParser to avoid otherwise exposing the Isolate::Scope outside of the
class.

Tested by compiling with `--experimental-enable-pointer-compression`
locally and running all tests.

Refs: nodejs/build#3204 (comment)
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292
PR-URL: #50680
Refs: v8/v8@475c8cd
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
@kvakil @UlisesGascon
src: fix JSONParser leaking internal V8 scopes
19c8da5 
JSONParser uses V8's JSON.parse (for now), meaning that its uses handles
and contexts. JSONParser was leaking its internal HandleScope and
Context::Scope.

Move the scope construction to the member functions to prevent those
scopes from leaking.

Refs: #50680 (comment)
PR-URL: #50688
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
@kvakil @UlisesGascon
src: add IsolateScopes before using isolates
f804b3a 
The V8 API requires entering an isolate before using it. We were often
not doing this, which worked fine in practice. However when (multi-cage)
pointer compression is enabled, the correct isolate needs to be active
in order to decompress pointers correctly, otherwise it causes crashes.

Fix this by sprinkling in some calls to v8::Isolate::Scope::Scope where
they were missing.

This also introduces RAIIIsolateWithoutEntering which is used in
JSONParser to avoid otherwise exposing the Isolate::Scope outside of the
class.

Tested by compiling with `--experimental-enable-pointer-compression`
locally and running all tests.

Refs: nodejs/build#3204 (comment)
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292
PR-URL: #50680
Refs: v8/v8@475c8cd
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
Merged
UlisesGascon pushed a commit that referenced this pull request Dec 19, 2023
@kvakil @UlisesGascon
src: fix JSONParser leaking internal V8 scopes
436c3ae 
JSONParser uses V8's JSON.parse (for now), meaning that its uses handles
and contexts. JSONParser was leaking its internal HandleScope and
Context::Scope.

Move the scope construction to the member functions to prevent those
scopes from leaking.

Refs: #50680 (comment)
PR-URL: #50688
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
UlisesGascon pushed a commit that referenced this pull request Dec 19, 2023
@kvakil @UlisesGascon
src: add IsolateScopes before using isolates
f054c33 
The V8 API requires entering an isolate before using it. We were often
not doing this, which worked fine in practice. However when (multi-cage)
pointer compression is enabled, the correct isolate needs to be active
in order to decompress pointers correctly, otherwise it causes crashes.

Fix this by sprinkling in some calls to v8::Isolate::Scope::Scope where
they were missing.

This also introduces RAIIIsolateWithoutEntering which is used in
JSONParser to avoid otherwise exposing the Isolate::Scope outside of the
class.

Tested by compiling with `--experimental-enable-pointer-compression`
locally and running all tests.

Refs: nodejs/build#3204 (comment)
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14292
PR-URL: #50680
Refs: v8/v8@475c8cd
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@kvakil kvakil mentioned this pull request Jan 24, 2024
Open
@richardlau richardlau added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

@bnoordhuis bnoordhuis Awaiting requested review from bnoordhuis

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@kvakil @nodejs-github-bot @bnoordhuis @mcollina @jasnell @joyeecheung @RaisinTen @richardlau