[wasm-interpreter] Coerce JS call results against the callee signature [v8/v8 : main]
Daniel Lehmann (Gerrit)
Daniel Lehmann voted and added 5 comments![]( "Open in Gerrit")
Votes added by Daniel Lehmann
| Code-Review | +1 |
5 comments
LGTM with nits. I didn't really follow the changes in `wasm-interpreter-runtime.cc` in detail (as usual for interpreter-only changes), but some background questions/nits regardless.
DCHECK(mode != Mode::kInternalCall ||
func_index_ != kInvalidFunctionIndex);
DCHECK(mode != Mode::kExternalCall ||
func_index_ == kInvalidFunctionIndex);
}Just to double check/clarify: `mode` can either be `kInternalCall` or `kExternalCall` (i.e., calling an import?), and in the latter case, `func_index_` must be `kInvalidFunctionIndex`, since there's no associated local/internal function, right?
In other words, these DCHECKs would be equivalent:
```
DCHECK_NE(mode, Mode::kInvalid);
DCHECK_EQ(mode == Mode::kExternalCall,
func_index_ == kInvalidFunctionIndex);
```
Or does `internal` mean Wasm? Maybe the enum values should be renamed, or at least the enum itself be documented.
static const uint32_t kInlineSignatureSentinel = UINT_MAX;So IIUC, this CL removes the signature caching optimization completely. Out of interest, how much did that affect performance?
internal->sig()->index());This could be `actual_sig_index` IIUC.
// Regression test for a call_indirect bug. The interpreter cached theIs there precedent for adding regression test cases to this (already very long) file? I'd prefer the usual convention, i.e., a file in `test/mjsunit/wasm/regress-<bugid>.js`, this way it's also more likely to be picked up by fuzzers and mutated in the future.
Related details
- Paolo Severini
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paolo Severini (Gerrit)
Paolo Severini added 4 comments![]( "Open in Gerrit")
DCHECK(mode != Mode::kInternalCall ||
func_index_ != kInvalidFunctionIndex);
DCHECK(mode != Mode::kExternalCall ||
func_index_ == kInvalidFunctionIndex);
}Just to double check/clarify: `mode` can either be `kInternalCall` or `kExternalCall` (i.e., calling an import?), and in the latter case, `func_index_` must be `kInvalidFunctionIndex`, since there's no associated local/internal function, right?
In other words, these DCHECKs would be equivalent:
```
DCHECK_NE(mode, Mode::kInvalid);
DCHECK_EQ(mode == Mode::kExternalCall,
func_index_ == kInvalidFunctionIndex);
```Or does `internal` mean Wasm? Maybe the enum values should be renamed, or at least the enum itself be documented.
Thanks, these DCHECKs are equivalent but clearer, changing them.
kExternalCall is not "JS import" specifically. The modes are about where dispatch goes, decided at cache-fill time:
- kInternalCall: target is a Wasm function in the current instance
- kExternalCall: target leaves this interpreter/instance. Two sub-cases, both with func_index == kInvalidFunctionIndex: a Wasm function in a different instance → CallExternalWasmFunction (line 1946), or a JS import. In both cases, the runtime re-reads entry.function_index() fresh rather than the stored index, which is why caching kInvalidFunctionIndex is correct. I will try to document this better.
static const uint32_t kInlineSignatureSentinel = UINT_MAX;So IIUC, this CL removes the signature caching optimization completely. Out of interest, how much did that affect performance?
I have not taken perf measurements, but the caching of the signature was also affecting the external call_indirect path (cross-instance Wasm and JS imports) and the signature can be very efficiently retrieved with `module_->signature({sig_index});` -> `return types[index.index].function_sig;`. It was a very useless optimization.
This could be `actual_sig_index` IIUC.
Good catch! Yes, `actual_sig_index` is `internal->sig()->index()` computed above. Switched to reuse it.
// Regression test for a call_indirect bug. The interpreter cached theIs there precedent for adding regression test cases to this (already very long) file? I'd prefer the usual convention, i.e., a file in `test/mjsunit/wasm/regress-<bugid>.js`, this way it's also more likely to be picked up by fuzzers and mutated in the future.
This was done in the past not to "pollute" the list of mjsunit tests with regression tests that were only related to the Wasm interpreter. Almost all test cases in this file are regression tests. Consider also that in most cases the bug was opened "downstream" in Edge (and there was actually already a bug downstream even for this specific issue) and in the past we didn't also open a corresponding bug upstream. I realize however that this test file is really becoming too long, and more importantly, that now that the interpreter os being used for tvOS it is important to document all the bugs upstream.
Related details
- Daniel Lehmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann voted and added 4 comments![]( "Open in Gerrit")
Votes added by Daniel Lehmann
| Code-Review | +1 |
LGTM with nits. I didn't really follow the changes in `wasm-interpreter-runtime.cc` in detail (as usual for interpreter-only changes), but some background questions/nits regardless.
Still LGTM
DCHECK(mode != Mode::kInternalCall ||
func_index_ != kInvalidFunctionIndex);
DCHECK(mode != Mode::kExternalCall ||
func_index_ == kInvalidFunctionIndex);
}Paolo SeveriniJust to double check/clarify: `mode` can either be `kInternalCall` or `kExternalCall` (i.e., calling an import?), and in the latter case, `func_index_` must be `kInvalidFunctionIndex`, since there's no associated local/internal function, right?
In other words, these DCHECKs would be equivalent:
```
DCHECK_NE(mode, Mode::kInvalid);
DCHECK_EQ(mode == Mode::kExternalCall,
func_index_ == kInvalidFunctionIndex);
```Or does `internal` mean Wasm? Maybe the enum values should be renamed, or at least the enum itself be documented.
Thanks, these DCHECKs are equivalent but clearer, changing them.
kExternalCall is not "JS import" specifically. The modes are about where dispatch goes, decided at cache-fill time:
- kInternalCall: target is a Wasm function in the current instance
- kExternalCall: target leaves this interpreter/instance. Two sub-cases, both with func_index == kInvalidFunctionIndex: a Wasm function in a different instance → CallExternalWasmFunction (line 1946), or a JS import. In both cases, the runtime re-reads entry.function_index() fresh rather than the stored index, which is why caching kInvalidFunctionIndex is correct. I will try to document this better.
Thanks for the background and additional comments.
static const uint32_t kInlineSignatureSentinel = UINT_MAX;Paolo SeveriniSo IIUC, this CL removes the signature caching optimization completely. Out of interest, how much did that affect performance?
I have not taken perf measurements, but the caching of the signature was also affecting the external call_indirect path (cross-instance Wasm and JS imports) and the signature can be very efficiently retrieved with `module_->signature({sig_index});` -> `return types[index.index].function_sig;`. It was a very useless optimization.
Sounds good, simplifying ftw.
// Regression test for a call_indirect bug. The interpreter cached thePaolo SeveriniIs there precedent for adding regression test cases to this (already very long) file? I'd prefer the usual convention, i.e., a file in `test/mjsunit/wasm/regress-<bugid>.js`, this way it's also more likely to be picked up by fuzzers and mutated in the future.
This was done in the past not to "pollute" the list of mjsunit tests with regression tests that were only related to the Wasm interpreter. Almost all test cases in this file are regression tests. Consider also that in most cases the bug was opened "downstream" in Edge (and there was actually already a bug downstream even for this specific issue) and in the past we didn't also open a corresponding bug upstream. I realize however that this test file is really becoming too long, and more importantly, that now that the interpreter os being used for tvOS it is important to document all the bugs upstream.
Mhm, maybe for bugs where we already have a Chromium issue, we could have a separate regression test, although I'm not sure this is consensus in the team (so it's not too important either way).
Related details
- Paolo Severini
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paolo Severini (Gerrit)
Paolo Severini voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
v8-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
v8-s...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
[wasm-interpreter] Coerce JS call results against the callee signature
The interpreter cached a FunctionSig* on each indirect dispatch table
entry, derived from the first call_indirect signature that populated the
slot. A later call on the same entry through a subtype signature passed
the canonical subtype check but then reused the stale cached signature
to coerce the imported JS function's return values. As a result the
JS<->Wasm boundary could convert return values against a wider supertype
than the one the callee actually declared, so a value valid for the
cached signature but not for the callee's signature could reach the
typed value stack.
Stop caching the FunctionSig* in IndirectCallValue. Instead look up the
two signatures that are actually needed at the call site:
- callsite_signature: drives the reference stack / slot layout, matching
how arguments and results were laid out by the caller.
- callee_canonical_sig: the callee's declared canonical signature, used
to validate and coerce the JS return values, matching the behavior of
compiled Wasm-to-JS wrappers.
For direct imports the two coincide, so callee_canonical_sig stays null
and behavior is unchanged. Also add DCHECK(IsWasmStruct(...)) guards to
the struct.get/struct.set handlers.
Adds a regression test that exercises a wide-then-narrow call_indirect
sequence on the same table entry and checks that the narrow call still
rejects a non-conforming JS return value.
- M src/wasm/interpreter/wasm-interpreter-runtime.cc
- M src/wasm/interpreter/wasm-interpreter-runtime.h
- M src/wasm/interpreter/wasm-interpreter.cc
- A test/mjsunit/regress/wasm/regress-518064995.js
Code-Review: +1 by Daniel Lehmann
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |