[heap] Fix CodeRange red zone allocation for contiguous RO space [v8/v8 : main]

[Michael Lippautz (Gerrit)]

Attention needed from Dominik Inführ

New activity on the change

Open in Gerrit

Related details

Attention is currently required from:

• Dominik Inführ

Submit Requirements:

• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I72299ab49878ce853ee6a395d9f9e68f475b4b49

Gerrit-Change-Number: 6913686

Gerrit-PatchSet: 2

Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>

Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>

Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Hannes Payer <hpa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Dominik Inführ <dinf...@chromium.org>

Gerrit-Comment-Date: Thu, 04 Sep 2025 08:58:29 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: No

[Dominik Inführ (Gerrit)]

Attention needed from Michael Lippautz

Dominik Inführ added 3 comments

File src/heap/code-range.cc

Line 255, Patchset 2 (Latest): // red_zone_start ^ ^ red_zone_end

Dominik Inführ . unresolved

Nit: I think the "red zone" should go to both (or either of) data_cage_ro_start/data_cage_ro_end. And those variables don't exist anymore, so probably should be renamed as well.

Line 272, Patchset 2 (Latest): current += (size_t{4} * kPtrComprCageBaseAlignment)) {

Dominik Inführ . unresolved

Isn't this 16GB? kPtrComprCageBaseAlignment is already 1<<32, so 4GB?

Line 283, Patchset 2 (Latest): CHECK_EQ(red_zone_size, region()

Dominik Inführ . unresolved

Shouldn't this be CHECK_LE here? Let's say our code cage starts add 4GB+2MB - then our red zone size should only be 6MB instead of the full 8MB.

Open in Gerrit

Related details

Attention is currently required from:

• Michael Lippautz

Submit Requirements:

• Code-Owners
• Code-Review

• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I72299ab49878ce853ee6a395d9f9e68f475b4b49

Gerrit-Change-Number: 6913686

Gerrit-PatchSet: 2

Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>

Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>

Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Hannes Payer <hpa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>

Gerrit-Comment-Date: Thu, 04 Sep 2025 09:14:09 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Dominik Inführ (Gerrit)]

Attention needed from Michael Lippautz

Dominik Inführ voted and added 1 comment

Votes added by Dominik Inführ

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Dominik Inführ . resolved

Thanks, LGTM % comments

Open in Gerrit

Related details

Attention is currently required from:

• Michael Lippautz

Submit Requirements:

• Code-Owners
• Code-Review

• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I72299ab49878ce853ee6a395d9f9e68f475b4b49

Gerrit-Change-Number: 6913686

Gerrit-PatchSet: 2

Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>

Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>

Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Hannes Payer <hpa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>

Gerrit-Comment-Date: Thu, 04 Sep 2025 09:14:42 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Dominik Inführ (Gerrit)]

Attention needed from Michael Lippautz

Dominik Inführ added 1 comment

File src/heap/code-range.cc

Line 290, Patchset 2 (Latest): }

Dominik Inführ . unresolved

Nit: Maybe as a sanity check, we could check here after the loop that `CHECK_LE(number_of_red_zones, 1)`.

Open in Gerrit

Related details

Attention is currently required from:

• Michael Lippautz

Submit Requirements:

• Code-Owners
• Code-Review
• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I72299ab49878ce853ee6a395d9f9e68f475b4b49

Gerrit-Change-Number: 6913686

Gerrit-PatchSet: 2

Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>

Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>

Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Hannes Payer <hpa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>

Gerrit-Comment-Date: Thu, 04 Sep 2025 09:17:53 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Michael Lippautz (Gerrit)]

Michael Lippautz voted and added 4 comments

Votes added by Michael Lippautz

Commit-Queue

+1

4 comments

File src/heap/code-range.cc

Line 255, Patchset 2: // red_zone_start ^ ^ red_zone_end

Dominik Inführ . resolved

Nit: I think the "red zone" should go to both (or either of) data_cage_ro_start/data_cage_ro_end. And those variables don't exist anymore, so probably should be renamed as well.

Michael Lippautz

Good point. I simplified this picture and removed the variable names. This allowed for adding more cases.

Line 272, Patchset 2: current += (size_t{4} * kPtrComprCageBaseAlignment)) {

Dominik Inführ . resolved

Isn't this 16GB? kPtrComprCageBaseAlignment is already 1<<32, so 4GB?

Michael Lippautz

Done

Line 283, Patchset 2: CHECK_EQ(red_zone_size, region()

Dominik Inführ . resolved

Shouldn't this be CHECK_LE here? Let's say our code cage starts add 4GB+2MB - then our red zone size should only be 6MB instead of the full 8MB.

Michael Lippautz

`red_zone_size` is 6MB in this case. It's computed from the adjusted start/end.

That said, I changed the check now to actually use AddressRegion::GetOverlap() which could actually be used instead if we see the crash going away.

Line 290, Patchset 2: }

Dominik Inführ . resolved

Nit: Maybe as a sanity check, we could check here after the loop that `CHECK_LE(number_of_red_zones, 1)`.

Michael Lippautz

Yes, done

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I72299ab49878ce853ee6a395d9f9e68f475b4b49

Gerrit-Change-Number: 6913686

Gerrit-PatchSet: 3

Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>

Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>

Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Hannes Payer <hpa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Comment-Date: Thu, 04 Sep 2025 09:26:48 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

Comment-In-Reply-To: Dominik Inführ <dinf...@chromium.org>

[Dominik Inführ (Gerrit)]

Attention needed from Michael Lippautz

Dominik Inführ voted and added 1 comment

Votes added by Dominik Inführ

Code-Review

+1

1 comment

File src/heap/code-range.cc

Line 292, Patchset 3 (Latest): CHECK_EQ(number_of_red_zones, 1);

Dominik Inführ . unresolved

Shouldn't this be <= 1? We don't necessarily have a red zone, right?

Open in Gerrit

Related details

Attention is currently required from:

• Michael Lippautz

Submit Requirements:

• Code-Owners
• Code-Review

• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I72299ab49878ce853ee6a395d9f9e68f475b4b49

Gerrit-Change-Number: 6913686

Gerrit-PatchSet: 3

Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>

Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>

Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Hannes Payer <hpa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>

Gerrit-Comment-Date: Thu, 04 Sep 2025 09:29:16 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Michael Lippautz (Gerrit)]

Michael Lippautz added 1 comment

File src/heap/code-range.cc

Line 292, Patchset 3: CHECK_EQ(number_of_red_zones, 1);

Dominik Inführ . resolved

Shouldn't this be <= 1? We don't necessarily have a red zone, right?

Michael Lippautz

Done

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I72299ab49878ce853ee6a395d9f9e68f475b4b49

Gerrit-Change-Number: 6913686

Gerrit-PatchSet: 4

Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>

Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>

Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Hannes Payer <hpa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Comment-Date: Thu, 04 Sep 2025 09:33:10 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Dominik Inführ <dinf...@chromium.org>

[Michael Lippautz (Gerrit)]

Michael Lippautz voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I72299ab49878ce853ee6a395d9f9e68f475b4b49

Gerrit-Change-Number: 6913686

Gerrit-PatchSet: 4

Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>

Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>

Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Hannes Payer <hpa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Comment-Date: Thu, 04 Sep 2025 10:10:42 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[V8 LUCI CQ (Gerrit)]

V8 LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: src/heap/code-range.cc
Insertions: 1, Deletions: 1.

@@ -289,7 +289,7 @@
                                              PageAllocator::kNoAccess));
     }
   }
-  CHECK_EQ(number_of_red_zones, 1);
+  CHECK_LE(number_of_red_zones, 1);
 #endif  // CONTIGUOUS_COMPRESSED_READ_ONLY_SPACE_BOOL
 
 // Don't pre-commit the code cage on Windows since it uses memory and it's not
```

Change information

Commit message:

[heap] Fix CodeRange red zone allocation for contiguous RO space

The computation only computed the first candidate region that would
overlap but there's also a candidate within the code range itself.

Bug: 442942399, 429538831

Change-Id: I72299ab49878ce853ee6a395d9f9e68f475b4b49

Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6913686

Commit-Queue: Michael Lippautz <mlip...@chromium.org>

Reviewed-by: Dominik Inführ <dinf...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#102238}

Files:

• M src/heap/code-range.cc

Change size: M

Delta: 1 file changed, 42 insertions(+), 24 deletions(-)

Branch: refs/heads/main

Submit Requirements:

• Code-Review: +1 by Dominik Inführ

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: merged

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: I72299ab49878ce853ee6a395d9f9e68f475b4b49

Gerrit-Change-Number: 6913686

Gerrit-PatchSet: 5

Gerrit-Owner: Michael Lippautz <mlip...@chromium.org>

Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>

Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>

Gerrit-Reviewer: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

[Nico Hartmann (Gerrit)]

Nico Hartmann has created a revert of this change

Open original change

Related details

Attention set is empty

Submit Requirements:

• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: revert