Serialize side table and integrate `verify` #758

zhouwfang · 2025-02-23T00:29:49Z

#46

ia0

Thanks! I think the issues come from the fact that the side-table format is not correct yet.

crates/interpreter/src/format.rs

crates/interpreter/src/module.rs

crates/interpreter/src/side_table.rs

crates/interpreter/src/format.rs

crates/interpreter/src/module.rs

crates/interpreter/src/side_table.rs

zhouwfang · 2025-02-28T07:07:56Z

Now that valid.rs becomes generic over ValidMode, I wonder how I could print debugging logs for generic objects. For example, I'm not sure how to print the next branch table. I added Debug at some places, but there still seems something missing.

ia0 · 2025-02-28T10:36:42Z

Now that valid.rs becomes generic over ValidMode, I wonder how I could print debugging logs for generic objects.

I've pushed a commit for that (called debug). For generic contexts you need the Debug bound on the associated types (not just on the final types used for those associated types in the trait implementations).

zhouwfang · 2025-03-02T07:33:33Z

crates/interpreter/src/valid.rs

+            source.parser == target.parser
+                && source.result == target.result
+                && source.branch_table == target.branch_table
+                && source.stack <= target.stack,


@ia0 This is a necessary condition of the logic in pop_cnt(). The actual equivalent condition should be source.stack == target.stack || (pop_cnt == 0 && source.stack < target.stack), but it does not seem straightforward to obtain pop_cnt wherever stitch_branch() is called.

Update: we can store pop_cnt as Option<usize> in SideTableBranch. WDYT?

I'll need to look into this when I get time. I think the whole situation around pop_cnt feels wrong, but maybe I'm missing something. I've added some TODOs to not lose track of this. This should not block this PR.

zhouwfang · 2025-03-02T21:21:42Z

The only test that fails in the test suite now is the huge br_table because of "index of MetadataEntry overflow" (the index is 97252). Do we want to change it to u32?

I suspect the error in ./scripts/hwci.sh is due to the same reason (but I'm not sure how to check that).

ia0

Thanks! That's a huge step, but I'm not convinced about the pop_cnt story yet. I'll need to look into it. Ideally we should do some differential fuzzing, I don't trust the test suite to provide good coverage.

ia0 · 2025-03-03T07:46:21Z

crates/interpreter/src/module.rs

+        let mut parser = unsafe { Parser::new(self.binary) };
+        parser.parse_side_table().unwrap();


This is costly. I'm pushing an optimize Module::func() commit to fix it. The idea is to have self.binary start at the same place as module_start in Context::check_module().

ia0 · 2025-03-03T09:45:36Z

crates/interpreter/src/valid.rs

+            source.parser == target.parser
+                && source.result == target.result
+                && source.branch_table == target.branch_table
+                && source.stack <= target.stack,


I'll need to look into this when I get time. I think the whole situation around pop_cnt feels wrong, but maybe I'm missing something. I've added some TODOs to not lose track of this. This should not block this PR.

ia0 · 2025-03-03T09:48:51Z

The only test that fails in the test suite now is the huge br_table because of "index of MetadataEntry overflow" (the index is 97252). Do we want to change it to u32?

I suspect the error in ./scripts/hwci.sh is due to the same reason (but I'm not sure how to check that).

I'm not sure we want to support such corner-cases. For now I'm ignoring all 149 tests in br_table, but maybe some of them we shouldn't. We can deal with that later.

If hwci is still failing, I'll take a look and work around it to merge. It might be because of the pop_cnt story which might be wrong and is not caught by the test suite.

ia0 · 2025-03-03T09:59:41Z

Yes, hwci is still failing, I'm disabling it in this branch.

zhouwfang · 2025-03-04T04:30:34Z

crates/interpreter/src/valid.rs

+    // TODO(dev/fast-interp): Figure out why we can't simply source.stack - target.stack and
+    // document it.


@ia0 I thought we shouldn't always expect source.stack to be >= target.stack. One example is the as-if-cond in br_if.wast. In pop_cnt() for Else, we have source=SideTableBranch { parser: [65, 2, 5, 65, 3, 11, 11, 11], branch_table: 1, stack: 0, result: 0 }, target=SideTableBranch { parser: [65, 3, 11, 11, 11], branch_table: 3, stack: 1, result: 1 }.

zhouwfang · 2025-03-04T04:36:51Z

The only test that fails in the test suite now is the huge br_table because of "index of MetadataEntry overflow" (the index is 97252). Do we want to change it to u32?
I suspect the error in ./scripts/hwci.sh is due to the same reason (but I'm not sure how to check that).

I'm not sure we want to support such corner-cases. For now I'm ignoring all 149 tests in br_table, but maybe some of them we shouldn't. We can deal with that later.

If hwci is still failing, I'll take a look and work around it to merge. It might be because of the pop_cnt story which might be wrong and is not caught by the test suite.

I had checked br_table.wast throughly, and large is only function that fails due to the u16 overflow (after I commented out this function, the test suite passes).

pop_cnt should be correct for hwci because I had completed its creation in valid.rs and usage in exec.rs long time ago, and there has been no issue with hwci. This CL does not make any change on pop_cnt itself. The hwci error should be due to u16 overflow.
Proof: in zhouwfang#10, I only added a commit on top of my last one in this PR that removes source.stack <= target.stack in stitch_branch(), which is the only code that is related to pop_cnt in this PR. But the hwci error remains.

zhouwfang requested a review from ia0 as a code owner February 23, 2025 00:29

zhouwfang mentioned this pull request Feb 23, 2025

Add merge() and serialize() zhouwfang/wasefire#9

Closed

ia0 reviewed Feb 26, 2025

View reviewed changes

zhouwfang added 15 commits February 26, 2025 18:38

feat!: add merge() and serialize()

eda11c8

fix: review

fb458d9

refactor: try_from

264540f

refactor: store num_functions as u16

f4f6766

refactor: remove Parser::parse_u16()

48f2613

refactor: merge()

9ee3f2a

refactor: add section()

b28956c

refactor!: merge->prepare; Module stores STView

25193e5

style: minor

8b366cd

docs: remove

395540b

fix: name serialization

b127178

fix: assert_{malformed,invalid} call prepare

3f3b07c

fix: Module::binary should ignore side table

12d6c10

fix: first validate in prepare

961e676

fix: branch_table()

0e701b9

zhouwfang force-pushed the serialize-side-table branch from edd20af to 0e701b9 Compare February 27, 2025 04:15

ia0 reviewed Feb 27, 2025

View reviewed changes

refactor!: add parse_side_table() in parser.rs

cf8c31c

zhouwfang requested a review from ia0 February 28, 2025 04:23

zhouwfang added 3 commits February 27, 2025 20:38

fix: import

b7def87

fix: parse_side_table()

59b4cf5

fix: side_table_result() in valid.rs

bc7d576

ia0 added 2 commits February 28, 2025 11:27

review

ffe3ad8

debug

3b73282

fix: patch_branch() off by 1

5f5d9c3

zhouwfang added 6 commits February 28, 2025 18:52

fix: remove assignment in patch_branch()

b7e3b3a

refactor

a6ca8a7

fix!: verify()

14b1300

fix!: call parse_side_table() in func()

09579e4

fix: add with overflow

c37e31b

fix: weaken the stack check in stitch_branch()

b1e4cb9

zhouwfang commented Mar 2, 2025

View reviewed changes

fix: remove check on stack in push_branch()

583c65f

zhouwfang requested review from ia0 and removed request for ia0 March 2, 2025 21:22

zhouwfang changed the title ~~Serialize side table~~ Serialize side table and integrate verify Mar 2, 2025

zhouwfang changed the title ~~Serialize side table and integrate verify~~ Serialize side table and integrate verify Mar 2, 2025

ia0 added 3 commits March 3, 2025 08:37

Merge branch 'dev/fast-interp' into serialize-side-table

40cd8b3

optimize Module::func()

0fbe6f3

review

293ea6a

ia0 previously approved these changes Mar 3, 2025

View reviewed changes

ia0 dismissed their stale review via c62d1cd March 3, 2025 09:59

disable hw-host in dev/fast-interp

911b5ae

ia0 force-pushed the serialize-side-table branch from c62d1cd to 911b5ae Compare March 3, 2025 10:55

ia0 approved these changes Mar 3, 2025

View reviewed changes

ia0 merged commit 188766a into google:dev/fast-interp Mar 3, 2025
19 checks passed

zhouwfang commented Mar 4, 2025

View reviewed changes

		let mut parser = unsafe { Parser::new(self.binary) };
		parser.parse_side_table().unwrap();

		// TODO(dev/fast-interp): Figure out why we can't simply source.stack - target.stack and
		// document it.

Serialize side table and integrate verify #758

Serialize side table and integrate verify #758

Uh oh!

Conversation

zhouwfang commented Feb 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ia0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhouwfang commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ia0 commented Feb 28, 2025

Uh oh!

zhouwfang Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouwfang Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ia0 Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

zhouwfang commented Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ia0 left a comment

Choose a reason for hiding this comment

Uh oh!

ia0 Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

ia0 Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

ia0 commented Mar 3, 2025

Uh oh!

ia0 commented Mar 3, 2025

Uh oh!

Uh oh!

zhouwfang Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouwfang commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Serialize side table and integrate `verify` #758

Serialize side table and integrate `verify` #758

zhouwfang commented Feb 23, 2025 •

edited

Loading

zhouwfang commented Feb 28, 2025 •

edited

Loading

zhouwfang Mar 2, 2025 •

edited

Loading

zhouwfang Mar 2, 2025 •

edited

Loading

zhouwfang commented Mar 2, 2025 •

edited

Loading

zhouwfang Mar 4, 2025 •

edited

Loading

zhouwfang commented Mar 4, 2025 •

edited

Loading