[wasm] Fix DCHECK failure when inlining br_table into JS [v8/v8 : main]

0 views
Skip to first unread message

Daniel Lehmann (Gerrit)

unread,
Oct 22, 2025, 8:08:42 AM (yesterday) Oct 22
to Thibaud Michaud, Matthias Liedtke, V8 LUCI CQ, v8-re...@googlegroups.com, was...@google.com
Attention needed from Thibaud Michaud

Daniel Lehmann voted and added 1 comment

Votes added by Daniel Lehmann

Auto-Submit+1
Commit-Queue+1

1 comment

Patchset-level comments
File-level comment, Patchset 2 (Latest):
Daniel Lehmann . resolved

@Thibaud: Could you take a look, since you were the author of the original fix for try_table in https://crrev.com/c/6618664, this just extends a missed case during br_table decoding.
CC'ing Matthias as he's on top of many Wasm-in-JS inlining things and was the original reviewer for https://crrev.com/c/6618664.

Open in Gerrit

Related details

Attention is currently required from:
  • Thibaud Michaud
Submit Requirements:
  • requirement satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: If353a334942b7d45e9f7c3e924b01ad7364b9c9b
Gerrit-Change-Number: 7072273
Gerrit-PatchSet: 2
Gerrit-Owner: Daniel Lehmann <dleh...@chromium.org>
Gerrit-Reviewer: Daniel Lehmann <dleh...@chromium.org>
Gerrit-Reviewer: Thibaud Michaud <thib...@chromium.org>
Gerrit-CC: Matthias Liedtke <mlie...@chromium.org>
Gerrit-Attention: Thibaud Michaud <thib...@chromium.org>
Gerrit-Comment-Date: Wed, 22 Oct 2025 12:08:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Thibaud Michaud (Gerrit)

unread,
Oct 22, 2025, 8:19:23 AM (yesterday) Oct 22
to Daniel Lehmann, Matthias Liedtke, V8 LUCI CQ, v8-re...@googlegroups.com, was...@google.com
Attention needed from Daniel Lehmann

Thibaud Michaud voted and added 1 comment

Votes added by Thibaud Michaud

Code-Review+1

1 comment

Patchset-level comments
Thibaud Michaud . resolved

Nice, LGTM!

Open in Gerrit

Related details

Attention is currently required from:
  • Daniel Lehmann
Submit Requirements:
  • requirement satisfiedCode-Owners
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: If353a334942b7d45e9f7c3e924b01ad7364b9c9b
Gerrit-Change-Number: 7072273
Gerrit-PatchSet: 2
Gerrit-Owner: Daniel Lehmann <dleh...@chromium.org>
Gerrit-Reviewer: Daniel Lehmann <dleh...@chromium.org>
Gerrit-Reviewer: Thibaud Michaud <thib...@chromium.org>
Gerrit-CC: Matthias Liedtke <mlie...@chromium.org>
Gerrit-Attention: Daniel Lehmann <dleh...@chromium.org>
Gerrit-Comment-Date: Wed, 22 Oct 2025 12:19:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Daniel Lehmann (Gerrit)

unread,
Oct 22, 2025, 8:44:34 AM (yesterday) Oct 22
to Thibaud Michaud, Matthias Liedtke, V8 LUCI CQ, v8-re...@googlegroups.com, was...@google.com

Daniel Lehmann voted Commit-Queue+2

Commit-Queue+2
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Owners
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: If353a334942b7d45e9f7c3e924b01ad7364b9c9b
Gerrit-Change-Number: 7072273
Gerrit-PatchSet: 2
Gerrit-Owner: Daniel Lehmann <dleh...@chromium.org>
Gerrit-Reviewer: Daniel Lehmann <dleh...@chromium.org>
Gerrit-Reviewer: Thibaud Michaud <thib...@chromium.org>
Gerrit-CC: Matthias Liedtke <mlie...@chromium.org>
Gerrit-Comment-Date: Wed, 22 Oct 2025 12:44:30 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

V8 LUCI CQ (Gerrit)

unread,
Oct 22, 2025, 8:46:17 AM (yesterday) Oct 22
to Daniel Lehmann, Thibaud Michaud, Matthias Liedtke, v8-re...@googlegroups.com, was...@google.com

V8 LUCI CQ submitted the change

Change information

Commit message:
[wasm] Fix DCHECK failure when inlining br_table into JS

This is analogous to https://crrev.com/c/6618664, where we forgot to fix
it not just for try_table but also br_table.

The issue is that we may not call `length()` after the decode interface
call, since the latter might have left the decoder in a failed state
(e.g., due to bailing out during Wasm-in-JS-inlining), which trips a
DHCECK in the former (see linked CL, same thing there).
Fixed: 451240002
Change-Id: If353a334942b7d45e9f7c3e924b01ad7364b9c9b
Auto-Submit: Daniel Lehmann <dleh...@chromium.org>
Reviewed-by: Thibaud Michaud <thib...@chromium.org>
Commit-Queue: Daniel Lehmann <dleh...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#103287}
Files:
  • M src/wasm/function-body-decoder-impl.h
  • A test/mjsunit/regress/wasm/regress-451240002.js
Change size: S
Delta: 2 files changed, 35 insertions(+), 4 deletions(-)
Branch: refs/heads/main
Submit Requirements:
  • requirement satisfiedCode-Review: +1 by Thibaud Michaud
Open in Gerrit
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: merged
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: If353a334942b7d45e9f7c3e924b01ad7364b9c9b
Gerrit-Change-Number: 7072273
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Lehmann <dleh...@chromium.org>
Gerrit-Reviewer: Daniel Lehmann <dleh...@chromium.org>
Gerrit-Reviewer: Thibaud Michaud <thib...@chromium.org>
Gerrit-CC: Matthias Liedtke <mlie...@chromium.org>
open
diffy
satisfied_requirement

Matthias Liedtke (Gerrit)

unread,
Oct 22, 2025, 9:27:17 AM (yesterday) Oct 22
to Daniel Lehmann, V8 LUCI CQ, Thibaud Michaud, v8-re...@googlegroups.com, was...@google.com
Attention needed from Daniel Lehmann

Matthias Liedtke added 1 comment

File src/wasm/function-body-decoder-impl.h
Line 924, Patchset 3 (Latest): DCHECK(!has_next());
Matthias Liedtke . unresolved
I am not convinced by this. Can't we make it more robust, e.g.:
```suggestion
DCHECK(decoder_->failed() || !has_next());
```
This function is already bad enough. "Please call it only in this very short time frame, otherwise it isn't going to work" isn't great...
Open in Gerrit

Related details

Attention is currently required from:
  • Daniel Lehmann
Submit Requirements:
  • requirement satisfiedCode-Owners
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: If353a334942b7d45e9f7c3e924b01ad7364b9c9b
Gerrit-Change-Number: 7072273
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Lehmann <dleh...@chromium.org>
Gerrit-Reviewer: Daniel Lehmann <dleh...@chromium.org>
Gerrit-Reviewer: Thibaud Michaud <thib...@chromium.org>
Gerrit-CC: Matthias Liedtke <mlie...@chromium.org>
Gerrit-Attention: Daniel Lehmann <dleh...@chromium.org>
Gerrit-Comment-Date: Wed, 22 Oct 2025 13:27:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
open
diffy

Daniel Lehmann (Gerrit)

unread,
Oct 22, 2025, 10:16:21 AM (yesterday) Oct 22
to V8 LUCI CQ, Thibaud Michaud, Matthias Liedtke, v8-re...@googlegroups.com, was...@google.com

Daniel Lehmann added 1 comment

File src/wasm/function-body-decoder-impl.h
Line 924, Patchset 3 (Latest): DCHECK(!has_next());
Matthias Liedtke . unresolved
I am not convinced by this. Can't we make it more robust, e.g.:
```suggestion
DCHECK(decoder_->failed() || !has_next());
```
This function is already bad enough. "Please call it only in this very short time frame, otherwise it isn't going to work" isn't great...
Daniel Lehmann

Recording offline discussion: Right, let's loosen this invariant but not by the suggestion above but by returning false in `has_next()` when `!decoder->ok()` (in other words: remove the `VALIDATE` wrapper). I'll do that in a follow-up.

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Owners
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: If353a334942b7d45e9f7c3e924b01ad7364b9c9b
Gerrit-Change-Number: 7072273
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Lehmann <dleh...@chromium.org>
Gerrit-Reviewer: Daniel Lehmann <dleh...@chromium.org>
Gerrit-Reviewer: Thibaud Michaud <thib...@chromium.org>
Gerrit-CC: Matthias Liedtke <mlie...@chromium.org>
Gerrit-Comment-Date: Wed, 22 Oct 2025 14:16:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Matthias Liedtke <mlie...@chromium.org>
satisfied_requirement
open
diffy

Daniel Lehmann (Gerrit)

unread,
Oct 22, 2025, 11:10:44 AM (yesterday) Oct 22
to V8 LUCI CQ, Thibaud Michaud, Matthias Liedtke, v8-re...@googlegroups.com, was...@google.com

Daniel Lehmann added 1 comment

File src/wasm/function-body-decoder-impl.h
Line 924, Patchset 3 (Latest): DCHECK(!has_next());
Matthias Liedtke . resolved
I am not convinced by this. Can't we make it more robust, e.g.:
```suggestion
DCHECK(decoder_->failed() || !has_next());
```
This function is already bad enough. "Please call it only in this very short time frame, otherwise it isn't going to work" isn't great...
Daniel Lehmann

Recording offline discussion: Right, let's loosen this invariant but not by the suggestion above but by returning false in `has_next()` when `!decoder->ok()` (in other words: remove the `VALIDATE` wrapper). I'll do that in a follow-up.

Daniel Lehmann

Continuing this proposed cleanup here: https://crrev.com/c/7073593

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Owners
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: If353a334942b7d45e9f7c3e924b01ad7364b9c9b
Gerrit-Change-Number: 7072273
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Lehmann <dleh...@chromium.org>
Gerrit-Reviewer: Daniel Lehmann <dleh...@chromium.org>
Gerrit-Reviewer: Thibaud Michaud <thib...@chromium.org>
Gerrit-CC: Matthias Liedtke <mlie...@chromium.org>
Gerrit-Comment-Date: Wed, 22 Oct 2025 15:10:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Daniel Lehmann <dleh...@chromium.org>
Comment-In-Reply-To: Matthias Liedtke <mlie...@chromium.org>
satisfied_requirement
open
diffy
Reply all
Reply to author
Forward
0 new messages