Add the `Verify` mode #726

zhouwfang · 2025-01-22T07:32:35Z

crates/interpreter/src/valid.rs

zhouwfang · 2025-01-27T04:03:15Z

crates/interpreter/src/valid.rs

+        // expected_target.parser += delta_ip;
+        // expected_target.branch_table += delta_stp;


The remaining question is how to format and read the side table from the custom section of the WASM module.

In terms of the format, how about using the format that was intended for flash storage?

It may not be efficient to keep the whole side table in RAM. How about keeping a parser for it and reading the data on demand?

More precisely,

[Section ID: 0] [Section Size: ...] [Section Name Length: 18] [Section Name: "Wasefire-SideTable"] [Indices Array Size (LEB128): num_functions + 1] [Indices Array: (u16 * (num_functions + 1))] [Metadata Array Size (LEB128): ...] [Metadata Array: (u16 * ...)]

In terms of the format, how about using the format that was intended for flash storage?

Yes, that's the idea, even if we didn't have a proper parsing function yet. That's the format I wrote there (updated below):

side-table := index flatten-metadata (16-bits aligned) index := u16 u16 ... u16 (N + 1 times where N is the number of functions in the module) flatten-metadata := metadata metadata ... metadata (N times where `flatten-metadata[i]` starts at byte position `index[i]` and ends at `index[i + 1]`) metadata := type:u16 parser:(u32 u32) branch-table branch-table := branch branch ... branch (length is implicitly defined by `index[i + 1]` see above) branch := delta-ip:u16 delta-stp:u16 val-cnt:u8 pop-cnt:u12

In particular, we shouldn't use LEB128 since u16 is enough and the benefit from using only one byte up to 127 is not worth it.

ia0

I've pushed a commit to better factorize end_label and br_label. It's not finished because similarly to the ValidMode::Branches, we also need ValidMode::BranchTable, which is either prepared or verified. In particular it must be read from ValidMode::patch_branch() for Verify.

crates/interpreter/src/valid.rs

zhouwfang · 2025-02-06T07:05:41Z

crates/interpreter/src/valid.rs

+        // metadata
+        Err(Invalid)


It seems I couldn't return a mutable reference to metadata (local). (I had a design for returning by value.)

Yes, it's because Verify::BranchTable<'m> shouldn't be Metadata<'m> but some BranchTableView<'m> that contains a Metadata<'m> and a current branch index.

I'm not sure I understand (the returned reference must be part of Self::SideTable) but I have a different solution. PTAL.

ia0

Thanks! I think it's getting there.

crates/interpreter/src/valid.rs

crates/interpreter/src/side_table.rs

crates/interpreter/src/parser.rs

crates/interpreter/src/valid.rs

ia0 · 2025-02-06T10:11:12Z

crates/interpreter/src/valid.rs

+        // metadata
+        Err(Invalid)


Yes, it's because Verify::BranchTable<'m> shouldn't be Metadata<'m> but some BranchTableView<'m> that contains a Metadata<'m> and a current branch index.

crates/interpreter/src/valid.rs

ia0 · 2025-02-06T10:15:11Z

crates/interpreter/src/valid.rs

@@ -928,7 +1053,7 @@ impl<'a, 'm, M: ValidMode> Expr<'a, 'm, M> {
    fn branch(&self) -> SideTableBranch<'m> {
        SideTableBranch {
            parser: self.parser.save(),
-            branch_table: self.branch_table.save(),
+            branch_table: self.branch_table.as_ref().map_or(0, |x| x.next_index()),


It doesn't work to unwrap? I would expect branch to be only called outside const context.

No, it could be None. I guess it is because branch_target() is called for Verify.

It's because of the End instruction for consts. I'm fixing it in the review commit.

ia0 · 2025-02-06T10:16:30Z

crates/interpreter/src/valid.rs

+    type Branches<'m> = Vec<SideTableBranch<'m>>;
+    type BranchTable<'m> = Vec<BranchTableEntry>;
+    type SideTable<'m> = Vec<MetadataEntry>;
+    type Result = Vec<MetadataEntry>;


We could probably have this be Vec<u8> ultimately (and represent the content of the custom section).

Can we convert it in merge() in zhouwfang#8? Please also take a look at the design in that PR. Thanks!

Yes, we could do it in merge too. Ideally this would be an opaque type to the user. Something called SideTable.

ia0

Thanks! We can do the rest in followup PRs.

ia0 · 2025-02-07T08:05:32Z

crates/interpreter/src/side_table.rs

+pub struct SideTableView<'m> {
+    pub func_idx: usize,
+    pub indices: &'m [u16], // including 0 and the length of metadata_array
+    pub metadata: &'m [u16],


We should change those to u8. We can't guarantee those will be aligned in flash. This could be done in another PR though. Let's add a TODO.

ia0 · 2025-02-07T08:28:24Z

crates/interpreter/src/side_table.rs

+}
+
+fn parse_side_table_field<'m>(parser: &mut crate::valid::Parser<'m>) -> Result<&'m [u16], Error> {
+    let len = parse_u16(parser.save()) as usize;


We shouldn't parse like WASM bytecode. The format can simply be #726 (comment). This assumes we know the number of functions, which we can cheaply get from the length of the function section. We could decide to store it in the side table though, but we only need this number. We don't need 2 lengths for each slice, and we don't need them to be in bytes since we know the size of the elements.

ia0 · 2025-02-07T08:29:46Z

crates/interpreter/src/side_table.rs

-impl<'m> SideTable<'m> {
-    fn metadata(&self, func_idx: usize) -> Metadata<'m> {
+impl<'m> SideTableView<'m> {
+    pub fn new(parser: &mut crate::valid::Parser<'m>) -> Result<Self, Error> {


We shouldn't take a parser as argument. See other comment. We should just take the binary data and the number of functions. We could simplify by not taking the number of functions and storing it in the binary data as the first field (u16 should be enough).

ia0 · 2025-02-07T08:30:56Z

crates/interpreter/src/valid.rs

+    fn stitch_branch(
+        &mut self, source: SideTableBranch<'m>, target: SideTableBranch<'m>,
+    ) -> CheckResult;
+


nit: let's also pack those declarations by removing the empty lines

ia0 · 2025-02-07T08:31:38Z

crates/interpreter/src/valid.rs

+    type Branches<'m> = Vec<SideTableBranch<'m>>;
+    type BranchTable<'m> = Vec<BranchTableEntry>;
+    type SideTable<'m> = Vec<MetadataEntry>;
+    type Result = Vec<MetadataEntry>;


Yes, we could do it in merge too. Ideally this would be an opaque type to the user. Something called SideTable.

ia0 · 2025-02-07T08:32:53Z

crates/interpreter/src/valid.rs

+        side_table.func_idx += 1;
+        check(
+            side_table.branch_table_view.type_idx() == type_idx
+                && side_table.branch_table_view.parser_range() == parser_range,


nit: let's have 2 checks instead of a conjunction (simpler to read and more precise panic in debug mode)

ia0 · 2025-02-07T08:48:50Z

crates/interpreter/src/valid.rs

@@ -928,7 +1053,7 @@ impl<'a, 'm, M: ValidMode> Expr<'a, 'm, M> {
    fn branch(&self) -> SideTableBranch<'m> {
        SideTableBranch {
            parser: self.parser.save(),
-            branch_table: self.branch_table.save(),
+            branch_table: self.branch_table.as_ref().map_or(0, |x| x.next_index()),


It's because of the End instruction for consts. I'm fixing it in the review commit.

ia0 · 2025-02-07T08:50:08Z

crates/interpreter/src/valid.rs

+        Ok(source)
+    }
+
+    fn allocate_branch(&mut self) {}


We should do something here and below.

zhouwfang requested a review from ia0 as a code owner January 22, 2025 07:32

zhouwfang changed the title ~~Discuss the design for the Verify mode~~ Discuss the Verify mode design Jan 22, 2025

ia0 mentioned this pull request Jan 22, 2025

Add empty ValidMode and Prepare #725

Merged

ia0 reviewed Jan 22, 2025

View reviewed changes

crates/interpreter/src/valid.rs Outdated Show resolved Hide resolved

crates/interpreter/src/valid.rs Outdated Show resolved Hide resolved

crates/interpreter/src/valid.rs Outdated Show resolved Hide resolved

crates/interpreter/src/valid.rs Outdated Show resolved Hide resolved

zhouwfang added 5 commits January 23, 2025 22:23

docs: add design

5f8c0f6

docs: minor

aab0f90

style: minor

4b45d08

refacotr: rename validate() as prepare()

06900d8

feat!: add Verify mode

5dd00cf

zhouwfang force-pushed the add-verify-mode branch from 75024f7 to 5dd00cf Compare January 27, 2025 03:46

style: minor

2bf1ea3

zhouwfang requested a review from ia0 January 27, 2025 03:49

docs: minor

54846cc

zhouwfang changed the title ~~Discuss the Verify mode design~~ Add the Verify mode Jan 27, 2025

zhouwfang commented Jan 27, 2025

View reviewed changes

docs: minor

2f1e05d

ia0 reviewed Jan 30, 2025

View reviewed changes

refactor!: merge "Abstract lower-level components"

cda46ed

zhouwfang force-pushed the add-verify-mode branch from 6e65644 to cda46ed Compare January 31, 2025 04:31

zhouwfang commented Jan 31, 2025

View reviewed changes

crates/interpreter/src/valid.rs Outdated Show resolved Hide resolved

refactor!: add BranchesApi and BranchTableApi

88b7395

zhouwfang requested a review from ia0 February 3, 2025 05:25

ia0 reviewed Feb 3, 2025

View reviewed changes

zhouwfang added 2 commits February 3, 2025 20:04

refactor: relocate docs and rename functions

7d5786d

docs: add design comments

cac7a85

zhouwfang commented Feb 4, 2025

View reviewed changes

crates/interpreter/src/valid.rs Outdated Show resolved Hide resolved

zhouwfang commented Feb 4, 2025

View reviewed changes

crates/interpreter/src/valid.rs Outdated Show resolved Hide resolved

zhouwfang added 2 commits February 5, 2025 19:51

refactor!: add skeleton for Verify

46fec6d

feat!: implement Verify methods

352561b

zhouwfang commented Feb 6, 2025

View reviewed changes

crates/interpreter/src/valid.rs Outdated Show resolved Hide resolved

zhouwfang commented Feb 6, 2025

View reviewed changes

crates/interpreter/src/valid.rs Outdated Show resolved Hide resolved

zhouwfang commented Feb 6, 2025

View reviewed changes

zhouwfang requested a review from ia0 February 6, 2025 07:06

ia0 reviewed Feb 6, 2025

View reviewed changes

refactor!: review

23f461b

zhouwfang requested a review from ia0 February 7, 2025 05:02

review

e296676

ia0 approved these changes Feb 7, 2025

View reviewed changes

Merge branch 'dev/fast-interp' into add-verify-mode

fec66b8

ia0 merged commit e16faf8 into google:dev/fast-interp Feb 7, 2025
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the `Verify` mode #726

Add the `Verify` mode #726

zhouwfang commented Jan 22, 2025 •

edited

Loading

zhouwfang Jan 27, 2025

zhouwfang Jan 29, 2025

ia0 Jan 30, 2025

ia0 left a comment

zhouwfang Feb 6, 2025 •

edited

Loading

ia0 Feb 6, 2025

zhouwfang Feb 7, 2025 •

edited

Loading

ia0 left a comment

ia0 Feb 6, 2025

ia0 Feb 6, 2025

zhouwfang Feb 7, 2025

ia0 Feb 7, 2025

ia0 Feb 6, 2025

zhouwfang Feb 7, 2025

ia0 Feb 7, 2025

ia0 left a comment

ia0 Feb 7, 2025

ia0 Feb 7, 2025

ia0 Feb 7, 2025

ia0 Feb 7, 2025

ia0 Feb 7, 2025

ia0 Feb 7, 2025

ia0 Feb 7, 2025

ia0 Feb 7, 2025

		// expected_target.parser += delta_ip;
		// expected_target.branch_table += delta_stp;

Add the Verify mode #726

Add the Verify mode #726

Conversation

zhouwfang commented Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ia0 left a comment

Choose a reason for hiding this comment

zhouwfang Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouwfang Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

ia0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ia0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add the `Verify` mode #726

Add the `Verify` mode #726

zhouwfang commented Jan 22, 2025 •

edited

Loading

zhouwfang Feb 6, 2025 •

edited

Loading

zhouwfang Feb 7, 2025 •

edited

Loading