Compute delta_ip and delta_stp in side table

[zhouwfang]

#46

[ia0]

There are a few issues:

• side_table_last_entry is wrong, you need to supported nested blocks. You might need to put an index to the side table in Label or LabelKind.
• I don't think instruction index is the correct abstraction, I think we want instruction address. So you don't need instr_idx at all and can use parser.save().as_ptr() as i32.

Then in terms of design, I think we should keep Context constant in Expr and make Expr::check_body return the side-table. We can then set it from Context::check_module where check_body is called.

[zhouwfang]

There are a few issues:

• side_table_last_entry is wrong, you need to supported nested blocks. You might need to put an index to the side table in Label or LabelKind.
• I don't think instruction index is the correct abstraction, I think we want instruction address. So you don't need instr_idx at all and can use parser.save().as_ptr() as i32.

Then in terms of design, I think we should keep Context constant in Expr and make Expr::check_body return the side-table. We can then set it from Context::check_module where check_body is called.

Thanks! The issues with parser.save().as_ptr() as i32 are

1. parser.save().as_ptr() overflows.

2. The DELTA_IP_MASK is too small to hold it.

[ia0]

I'm not sure it's worth iterating too much. It's simpler to just design correctly from the start, which means understanding the goal. Essentially, we want all occurrences of skip_to_{else,end}() in exec.rs to be resolved from the side-table (to which we can add Loop to handle everything uniformly). This means If, Else, Br{,If} should each have an entry in the side-table and BrTable as much as its arguments. This defines when an entry is added. Then we need to fill their content, which is done on Loop, Else, and End (with Loop done before the entry is added). This means that for Loop we can store the parser position in the LabelKind (like done in exec.rs). For Else and End, we need the side-table entries to register themselves in their labels. This means that Label should contain an entries: Vec<(&'m [u8], usize)> recording for each side-table entry the parser position and the side-table length when it was added. Then in end_label() we can simply go over all entries compute the difference between the current parser position and the saved one to compute the delta IP, as well as the difference between the current side-table length and the index for the delta STP. We can then update the side-table entry.

[ia0]

Thanks! The issues with parser.save().as_ptr() as i32 are

1. parser.save().as_ptr() overflows.
2. The DELTA_IP_MASK is too small to hold it.

Those are not issues because we only care about deltas between parser.save(). In the proposed design, I store directly the &'m [u8] delays converting to i32 until after we compute the delta.

[zhouwfang]

I'm not sure it's worth iterating too much. It's simpler to just design correctly from the start, which means understanding the goal. Essentially, we want all occurrences of skip_to_{else,end}() in exec.rs to be resolved from the side-table (to which we can add Loop to handle everything uniformly). This means If, Else, Br{,If} should each have an entry in the side-table and BrTable as much as its arguments. This defines when an entry is added. Then we need to fill their content, which is done on Loop, Else, and End (with Loop done before the entry is added). This means that for Loop we can store the parser position in the LabelKind (like done in exec.rs). For Else and End, we need the side-table entries to register themselves in their labels. This means that Label should contain an entries: Vec<(&'m [u8], usize)> recording for each side-table entry the parser position and the side-table length when it was added. Then in end_label() we can simply go over all entries compute the difference between the current parser position and the saved one to compute the delta IP, as well as the difference between the current side-table length and the index for the delta STP. We can then update the side-table entry.

Thanks! I implemented this design except Loop for the moment. I'll run some tests in the meanwhile.

[zhouwfang]

@ia0 Could you explain the purpose of this if block? Thanks.

[ia0]

This is because the binary format allows an if without else (see the note of https://webassembly.github.io/spec/core/binary/instructions.html#control-instructions) and defines it as having an empty sequence for the else. So here we are validating the empty sequence according to https://webassembly.github.io/spec/core/valid/instructions.html#empty-instruction-sequence-epsilon which essentially says that the params and results must be the same.

[ia0]

Thanks! It starts to get in shape but there is still some confusion between "branches" and "entries". I introduced those new names to help make the distinction. An entry is an element in the side table. A branch is a marker left by an entry in the label it wishes to know the End (or Loop for Loop). There is thus a one-to-one mapping between branches and entries. This is ensured by used Option<SideTableEntryView> and making sure we write Some only on None and we only have Some at the end. We have a stitch function that stitches an entry based on its branch (with assumption that the parser is at the End of the label in which the branch is, or Else for If).

[ia0]

This is because the binary format allows an if without else (see the note of https://webassembly.github.io/spec/core/binary/instructions.html#control-instructions) and defines it as having an empty sequence for the else. So here we are validating the empty sequence according to https://webassembly.github.io/spec/core/valid/instructions.html#empty-instruction-sequence-epsilon which essentially says that the params and results must be the same.

[ia0]

Thanks! There are still some fundamental issues but the overall shape is getting there.

[ia0]

Thanks, it's almost there. The main question is how to deal with pathological examples in the test suite. I think the strategy of using a sentinel value in the side table entries and returning an error at runtime is a good option. If this results in performance problems (because we need to check the sentinel at runtime), we can gate this behavior behind a feature (like toctou). The feature would move returning an error from exec.rs to valid.rs ensuring that there are no sentinel values at runtime.

[ia0]

I've merge main into dev/fast-interp. You might want to pull and merge your add-delta-ip branch into dev/fast-interp. This adds better support for unsupported operations, such that we can still run parts of a test instead of having to ignore it completely.

[zhouwfang]

Could you clarify the hw-host error? I guess it results from the side table bit mask size issue.

[zhouwfang]

I posted zhouwfang#2 to add val_cnt and pop_cnt on top of this PR. Could you also take a look at that? Thanks.

[ia0]

Thanks! I think this should work modulo off-by-one errors. While updating exec.rs we might want to change the exact location of the branches or how to interpret the side table.

[ia0]

Could you clarify the hw-host error? I guess it results from the side table bit mask size issue.

Yes, this is indeed the side table masks being too short. So we'll probably have to use a u64 for the bit fields. You can also apply the following patch:

--- a/crates/interpreter/src/bit_field.rs
+++ b/crates/interpreter/src/bit_field.rs
@@ -29,6 +29,8 @@ fn offset(mask: u32) -> i32 {
 pub fn into_field(mask: u32, value: u32) -> Result<u32, Error> {
     let field = (value << mask.trailing_zeros()) & mask;
     if from_field(mask, field) != value {
+        #[cfg(feature = "debug")]
+        eprintln!("Bit field value {value:08x} doesn't fit in mask {mask:08x}.");
         return Err(unsupported(if_debug!(Unsupported::SideTable)));
     }
     Ok(field)

This is similar to what we do in the memory_too_small function.

[ia0]

Thanks! I've refactored the side table out to help the borrow checker understand it's alright. I also split branch() into branch_source() and branch_target() to be more explicit why we're taking a branch and avoid accidents of forgetting to allocate a side-table entry when taking a source branch.