[wasm-custom-desc][test] Exhaustive boundary tests [v8/v8 : main]
Jakob Kummerow (Gerrit)
Jakob Kummerow voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Commit-Queue | +1 |
1 comment
PTAL.
As the description says: this exercises all paths I could think of how a CD object might travel from Wasm to JS, both in terms of spec (functions, tables, etc) and in terms of implementation (different static types, generic/compiled wrappers). Can you think of anything I've forgotten?
The motivation is to prepare upcoming refactorings of the implementation, where we want to minimize the risk of introducing bugs.
Note: this CL was AI generated, with some manual touch-ups.
Related details
- Matthias Liedtke
- Thomas Lively
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke voted and added 6 comments![]( "Open in Gerrit")
Votes added by Matthias Liedtke
| Code-Review | +1 |
6 comments
LGTM with comments
// Flags: --experimental-wasm-custom-descriptors --wasm-wrapper-tiering-budget=1What's the motivation for this?
The default variant should cover the generic wrappers and the stress variant should cover the compiled wrappers. Having a low tiering budget might result in the default variant only partially running with the generic wrappers?
[kGCPrefix, kExprStructNewDefault, $desc]);
builder.addExportOfKind('g_desc', kExternalGlobal, $g_desc.index);Nit:
```suggestion
[kGCPrefix, kExprStructNewDefault, $desc]
).exportAs('g_desc');
```
(and similarly below)
builder.addGlobal(wasmRefType($desc3).exact(), false, false, [Nit: We could add a non-exact version as well? (If I didn't miss it.)
m: {js_func_any, js_func_extern, js_func_indexed},A descriptor can also have a descriptor, right?
So can we have a case where the `this` in JS is actually the wrapped desriptor?
If yes, that should warrant a test case where we `assertSame(identity, this)` and we should also be able to call `this.method42` if we register `method42` on the descriptor's descriptor?
assertSame(identity, wasm.g_desc.value);```suggestion
assertSame(identity, wasm.g_desc.value);
assertSame(identity, wasm.g_desc2.value);
```
Related details
- Jakob Kummerow
- Thomas Lively
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow voted and added 6 comments![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Commit-Queue | +1 |
6 comments
Thanks for the review (and the offline discussion). Comments addressed, need a fresh LGTM.
// Flags: --experimental-wasm-custom-descriptors --wasm-wrapper-tiering-budget=1What's the motivation for this?
The default variant should cover the generic wrappers and the stress variant should cover the compiled wrappers. Having a low tiering budget might result in the default variant only partially running with the generic wrappers?
Done
[kGCPrefix, kExprStructNewDefault, $desc]);
builder.addExportOfKind('g_desc', kExternalGlobal, $g_desc.index);Nit:
```suggestion
[kGCPrefix, kExprStructNewDefault, $desc]
).exportAs('g_desc');
```
(and similarly below)
Done
builder.addGlobal(wasmRefType($desc3).exact(), false, false, [Nit: We could add a non-exact version as well? (If I didn't miss it.)
Using it as an import to `struct.new_desc` (line 81) requires exactness. This global isn't exported.
A descriptor can also have a descriptor, right?
So can we have a case where the `this` in JS is actually the wrapped desriptor?
If yes, that should warrant a test case where we `assertSame(identity, this)` and we should also be able to call `this.method42` if we register `method42` on the descriptor's descriptor?
`g_desc2` is a descriptor that has a descriptor, yes. It's not the same object as `identity` (which is `g_desc`), but we can do something similar.
Done.
```suggestion
assertSame(identity, wasm.g_desc.value);
assertSame(identity, wasm.g_desc2.value);
```
- Matthias Liedtke
- Thomas Lively
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke voted and added 2 comments![]( "Open in Gerrit")
Votes added by Matthias Liedtke
| Code-Review | +1 |
2 comments
Thanks!
builder.addGlobal(wasmRefType($desc3).exact(), false, false, [Jakob KummerowNit: We could add a non-exact version as well? (If I didn't miss it.)
Using it as an import to `struct.new_desc` (line 81) requires exactness. This global isn't exported.
- Jakob Kummerow
- Thomas Lively
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |