src: fix pointer compression build #50680

kvakil · 2023-11-12T02:20:32Z

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 <[email protected]>
Reviewed-by: Jakob Kummerow <[email protected]>
Commit-Queue: Igor Sheludko <[email protected]>
Reviewed-by: Dominik Inführ <[email protected]>
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 · 2023-11-12T02:20:37Z

Review requested:

@nodejs/gyp
@nodejs/v8-update

nodejs-github-bot · 2023-11-12T02:32:04Z

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

src/node_sea.cc

src/json_parser.h

kvakil

@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

src/node_sea.cc

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)

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)

src/json_parser.h

bnoordhuis · 2023-11-12T22:54:17Z

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

Yes, good idea.

RaisinTen

Separate PR sounds good 👍

src/node_sea.cc

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

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 <[email protected]> Reviewed-by: Jakob Kummerow <[email protected]> Commit-Queue: Igor Sheludko <[email protected]> Reviewed-by: Dominik Inführ <[email protected]> Cr-Commit-Position: refs/heads/main@{#90075} Refs: v8/v8@475c8cd

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

@bnoordhuis @RaisinTen rebased on other PR - PTAL.

src/node_sea.cc

kvakil · 2023-11-16T02:58:29Z

src/util.cc

@@ -692,9 +692,14 @@ RAIIIsolate::RAIIIsolate(const SnapshotData* data)
  Isolate::Initialize(isolate_, params);
 }

-RAIIIsolate::~RAIIIsolate() {
+RAIIIsolateWithoutEntering::~RAIIIsolateWithoutEntering() {


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

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

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

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 · 2023-11-20T04:02:09Z

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

nodejs-github-bot · 2023-11-20T05:56:22Z

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

nodejs-github-bot · 2023-11-20T15:43:11Z

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

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 <[email protected]> Reviewed-by: Jakob Kummerow <[email protected]> Commit-Queue: Igor Sheludko <[email protected]> Reviewed-by: Dominik Inführ <[email protected]> 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 <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

kvakil · 2023-11-20T23:13:49Z

Landed in ec023a7...8e60189

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

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 <[email protected]> Reviewed-by: Jakob Kummerow <[email protected]> Commit-Queue: Igor Sheludko <[email protected]> Reviewed-by: Dominik Inführ <[email protected]> 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 <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

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 <[email protected]> Reviewed-by: Jakob Kummerow <[email protected]> Commit-Queue: Igor Sheludko <[email protected]> Reviewed-by: Dominik Inführ <[email protected]> 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 <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>