Add generic/typed take_struct imports + ops
#11713
Conversation
khagankhan
requested review from
alexcrichton
and removed request for
a team
github-actions
bot
added
the
fuzzing
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "fuzzing"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
| const STRUCT_BASE: u32 = 5; | ||
| const TYPED_FN_BASE: u32 = 4; |
There was a problem hiding this comment.
Not necessary in this PR, but it would probably be easier/cleaner down the line to switch from constants to using the wasm_encoder::{Type,Func}Section::len methods to keep track of these things, so that we don't need to update these constants every time we unconditionally add a new type/function.
https://docs.rs/wasm-encoder/latest/wasm_encoder/struct.TypeSection.html#method.len
|
|
||
| for i in 0..struct_count { | ||
| let concrete = STRUCT_BASE + i; | ||
| let ty_idx = typed_ft_base + i; // |
There was a problem hiding this comment.
Empty comment at the end of line?
| let name = format!("take_struct_{concrete}"); | ||
| typed_names.push(name); | ||
| imports.import( | ||
| "", | ||
| typed_names.last().unwrap().as_str(), | ||
| EntityType::Function(ty_idx), | ||
| ); |
There was a problem hiding this comment.
You can avoid the wonky .last() stuff by pushing the name after defining the import:
| let name = format!("take_struct_{concrete}"); | |
| typed_names.push(name); | |
| imports.import( | |
| "", | |
| typed_names.last().unwrap().as_str(), | |
| EntityType::Function(ty_idx), | |
| ); | |
| let name = format!("take_struct_{concrete}"); | |
| imports.import( | |
| "", | |
| &name, | |
| EntityType::Function(ty_idx), | |
| ); | |
| typed_names.push(name); |
| let mut typed_names: Vec<String> = Vec::new(); | ||
|
|
||
| for i in 0..struct_count { |
There was a problem hiding this comment.
Would be nice to have a single-sentence comment for readers here describing what this code is doing, so that they don't have to scrutinize it to figure out what is going on. Something like
For each of our concrete struct types, define a function import that takes an argument of that concrete type.
| // Define the "run" function export. | ||
| let mut functions = FunctionSection::new(); | ||
| functions.function(1); | ||
|
|
||
| let mut exports = ExportSection::new(); | ||
| exports.export("run", ExportKind::Func, 3); | ||
| let imported_fn_count: u32 = 4 + struct_count; | ||
| exports.export("run", ExportKind::Func, imported_fn_count); |
There was a problem hiding this comment.
As mentioned earlier, things get a bit simpler and less brittle if we use the len functions on the sections that define index spaces:
| exports.export("run", ExportKind::Func, imported_fn_count); | |
| let mut functions = FunctionSection::new(); | |
| let mut exports = ExportSection::new(); | |
| // Define the "run" function export. | |
| let run_index = functions.len(); | |
| functions.function(1); | |
| exports.export("run", ExportKind::Func, run_index); |
I think it is worth doing this to avoid adding more opaque <constant> + <count> incantations.
| func.instruction(&Instruction::StructNew(x + 5)); | ||
| func.instruction(&Instruction::Call(take_structref_idx)); | ||
| } | ||
| Self::TakeStructCall(x) => { | ||
| func.instruction(&Instruction::StructNew(x + 5)); | ||
| func.instruction(&Instruction::Call(take_structref_idx)); | ||
| } | ||
| Self::TakeTypedStructCall(x) => { | ||
| let s = STRUCT_BASE + x; | ||
| let f = TYPED_FN_BASE + x; | ||
| func.instruction(&Instruction::StructNew(s)); | ||
| func.instruction(&Instruction::Call(f)); |
There was a problem hiding this comment.
Nothing to change in this PR, but I want to point out that these emitted instruction sequences are not our ideal end state. We want to separate struct.new from various calls and drops and have the existing TableOp::Drop be used to drop struct refs and we want the take_* calls to take a struct ref from the stack, not create a new one to pass to the import. This will require updating the abstract stack representation to track types, however, so it should be left to a follow up PR.
|
Thanks @fitzgen. Working on them! |
…ableOps to GcOps as well as other related methods or DSs
|
@khagankhan can you delay the split into multiple modules for a follow up PR that contains nothing but the code motion? Otherwise it makes reading and interpreting the diff very hard, as a reviewer. |
|
@fitzgen I can do that. However, if you want I can point the code that needs review (that changed other than names) |
|
I'd prefer to just review one change at a time instead of multiple things, so please split up the PR. Thanks! |
|
Sure thing! o7 |
|
@fitzgen Reverted! Ready for review |
2 participants
structrefargument to our Wasm binary skeleton(type (;4;) (func (param (ref any))))is called afterStructNewop instead ofDropto observe that specific struct and also maintain stack discipline.cc @fitzgen @eeide