Add val_cnt and pop_cnt in side table #649

zhouwfang · 2024-10-23T05:30:46Z

There was an earlier review in zhouwfang#2.

crates/interpreter/src/valid.rs

zhouwfang · 2024-10-23T05:38:03Z

The hw-host error says Internal:NotImplemented. What does it refer to?

ia0

Thanks! Going through I realized we probably want to store val_cnt in source branches already since that's an information available at that time. This is particularly relevant in br_label().

So I suggest:

Add result: usize to SideTableBranch which is always zero for target branches and represents val_cnt for source branches.
Make branch_source() take result: usize.
Refactor br_label() to match on the label kind before creating the source branch, returning (ResultType, Option<SideTableBranch>) which is the result of the function and the target branch (if it's a loop). Then create the source branch with the result length, match on the option to stitch or push, and return the result.

crates/interpreter/src/valid.rs

ia0 · 2024-10-23T09:00:52Z

crates/interpreter/src/valid.rs

@@ -856,8 +883,12 @@ impl<'a, 'm> Expr<'a, 'm> {
        branch


We probably need to add self.stack().len() to branch.stack, in addition of setting val_cnt.

prev_stack has been updated so in push_label. Why do we need to do this again to branch.stack?

I think I forgot to reply here last review. I don't understand the question.

You suggested adding branch.stack += self.stack().len(); in branch_source().

branch_source() calls branch_target() which sets SideTableBranch::stack to be self.immutable_label().prev_stack. In push_label(), prev_stack is prev_label.prev_stack + prev_label.stack.len(), which seems the same as your suggestion here. Why do we need to do it again?

I see. It's not the same stack because it's not the same label.

In push_label() we record prev_stack as the stack size up to (but excluding) the label we're pushing. This is equivalent to the stack size up to (but excluding) the current label (before we push) plus the stack size of that current label, hence: prev_stack = prev_label.prev_stack + prev_label.stack.len().

In branch_source() we record stack as the stack size at the current program point. This is equivalent to the stack size up to (but excluding) the current label (which we get from branch_target()) plus the stack size of the current label, hence: branch.stack += self.stack().len().

ia0 · 2024-10-23T09:09:55Z

The hw-host error says Internal:NotImplemented. What does it refer to?

Ideally we should #[cfg(feature = "debug")] eprintln!(""); before returning Unsupported such that we can easily debug with runner host --features=wasefire-interpreter/debug.

ia0

Ok, I think it should be mostly correct. It'll be easier to debug once we can run the test suite using the side table.

crates/interpreter/src/valid.rs

ia0 · 2024-10-24T10:12:11Z

crates/interpreter/src/valid.rs

+    fn pop_cnt(source: SideTableBranch, target: SideTableBranch) -> MResult<u32, Check> {
+        let source = source.stack as i32;
+        let target = target.stack as i32;
+        let Some(delta) = target.checked_sub(source) else {


I expect the subtraction to be the other way around.

Let me know if my understand it correctly: target refers to the label that appears earlier than the label for source, so the value stack has more elements at the label for source, which makes the direction of the subtraction clear.

After I switched source and target here, I got an error in the commit dbbc1d1. To debug locally (I added some debug prints), I got

in my Docker container. The tricky part is that I always get the same error (even after I use the version that passes the CI), so there seems something persistent in my container. Not sure what went wrong elsewhere in the PR, and I'm very interested in how to reproduce the CI error locally.

Try removing --target=x86_64-unknown-linux-gnu. Maybe your container has a different triple. What's the output of rustc -vV in your container?

Same error with that removed. The output of rustc -vV is

rustc 1.82.0-nightly (28a58f2fa 2024-07-31) binary: rustc commit-hash: 28a58f2fa7f0c46b8fab8237c02471a915924fe5 commit-date: 2024-07-31 host: x86_64-unknown-linux-gnu release: 1.82.0-nightly LLVM version: 19.1.0

Ok so this is not because it's the wrong triple target.

Actually reading the error again it seems obvious. You can't create UNIX sockets in your container.

There is no simple way to confirm (because we don't support dynamic board configuration yet). What you could do to confirm is run sed -i '/uart/d' Cargo.toml src/board.rs src/main.rs from crates/runner-host and retry. This will disable UART support.

Thanks. The error in 2) is indeed gone. But for the following error, I wonder how to get web.tar.gz:

For that, you just need to run make from crates/runner-host/crates/web-client or ./test.sh from crates/runner-host.

crates/interpreter/src/valid.rs

ia0

I tried to look into the tests and actually there are fundamental problems:

Since I added auto-skipping of unsupported behaviors, we may accidentally pass the test because everything is unsupported due to a bug. I need to fix the tests so that we can distinguish between really unsupported and unsupported by accident. Not completely sure yet how, probably just a counter of skipped tests due to unsupported, but I'd like to find something better.
We need to be very careful when we call branch_{source,target}() because it will save the stack. This matters particularly when we push a label, because we don't want to save the parameters of the label.

Sadly I don't have time at the moment to dig into this. I think the design is not there yet (for example, maybe we actually want to store the "val_cnt/result" in the target branch instead. Maybe it will help with capturing too much of the stack because we know val_cnt is included.

If you want to continue working on this until I get time to look into again, you can try to experiment on your own and run the test suite to test the invariants you come up with. The more asserts you can write that don't fail in the test suite, the better your understanding of the code will be.

ia0 · 2024-10-25T06:52:37Z

crates/interpreter/src/valid.rs

+    fn pop_cnt(source: SideTableBranch, target: SideTableBranch) -> MResult<u32, Check> {
+        let source = source.stack as i32;
+        let target = target.stack as i32;
+        let Some(delta) = target.checked_sub(source) else {


Try removing --target=x86_64-unknown-linux-gnu. Maybe your container has a different triple. What's the output of rustc -vV in your container?

ia0 · 2024-10-25T06:54:55Z

crates/interpreter/src/valid.rs

@@ -856,8 +883,12 @@ impl<'a, 'm> Expr<'a, 'm> {
        branch


I think I forgot to reply here last review. I don't understand the question.

zhouwfang · 2024-10-28T04:15:19Z

With the commit cc29418 and #658, all the tests pass now.

In the meanwhile, I'm printing the side tables for some test cases in the test suite as a sanity check.

ia0

I don't mind merging this without thorough review such that you can start using the side table in exec.rs which is the important part to be able to debug. I think this PR needs a redesign and it's hard to tell unless the side table is actually used during execution. I'll do a proper review once we have everything working end-to-end.

But you can also use the side table in exec.rs in this PR if you want. As you prefer.

ia0 · 2024-10-28T09:35:18Z

crates/interpreter/src/valid.rs

-        let pop_cnt = 0;
+        let val_cnt = u32::try_from(source.result).map_err(|_| {
+            #[cfg(feature = "debug")]
+            eprintln!("side-table conversion overflow {0}", source.result);


nit (we don't get a line number so this helps distinguish error messages)

Suggested change

eprintln!("side-table conversion overflow {0}", source.result);

eprintln!("side-table val_cnt overflow {}", source.result);

ia0 · 2024-10-28T09:38:05Z

crates/interpreter/src/valid.rs

-        // TODO(dev/fast-interp): Compute the fields below.
-        let val_cnt = 0;
-        let pop_cnt = 0;
+        let val_cnt = u32::try_from(source.result).map_err(|_| {


I think it's actually better to have val_cnt stored in the branch target instead, since that's the one defining it. This should simplify br_label().

ia0 · 2024-10-28T09:39:15Z

crates/interpreter/src/valid.rs

+        let target = target.stack;
+        let Some(delta) = source.checked_sub(target) else {
+            #[cfg(feature = "debug")]
+            eprintln!("side-table negative stack delta {source} < {target}");


I know I suggested the current version, but when I debugged last time, I think it's clearer to just output the operation, than the unexpected true property.

Suggested change

eprintln!("side-table negative stack delta {source} < {target}");

eprintln!("side-table negative stack delta {source} - {target}");

ia0 · 2024-10-28T09:39:41Z

crates/interpreter/src/valid.rs

+        };
+        u32::try_from(delta).map_err(|_| {
+            #[cfg(feature = "debug")]
+            eprintln!("side-table conversion overflow {delta}");


same here, we could write that it's for pop_cnt

zhouwfang · 2024-10-29T05:42:54Z

Thanks for the review. I plan to use val_cnt and pop_cnt in exec.rs in this PR.

zhouwfang requested a review from ia0 as a code owner October 23, 2024 05:30

zhouwfang commented Oct 23, 2024

View reviewed changes

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

zhouwfang commented Oct 23, 2024

View reviewed changes

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

ia0 reviewed Oct 23, 2024

View reviewed changes

zhouwfang requested a review from ia0 October 24, 2024 05:45

ia0 reviewed Oct 24, 2024

View reviewed changes

zhouwfang requested a review from ia0 October 25, 2024 02:46

ia0 reviewed Oct 25, 2024

View reviewed changes

zhouwfang and others added 9 commits October 25, 2024 11:03

fix: increase mask size to 64 bits

263bbb5

review

0ecefc9

feat!: add val_cnt and pop_cnt

af96a4f

refactor: rename

79c0de8

fix!: review

9bbf440

fix: use i32 in pop_cnt()

fdaf75c

refactor: review

eaa1833

fix: target-source in pop_cnt()

5d2fffb

fix: review

cc29418

zhouwfang force-pushed the add-val-and-pop-cnt branch from 5c1c205 to cc29418 Compare October 28, 2024 03:58

zhouwfang requested a review from ia0 October 28, 2024 04:15

ia0 reviewed Oct 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add val_cnt and pop_cnt in side table #649

Add val_cnt and pop_cnt in side table #649

zhouwfang commented Oct 23, 2024

zhouwfang commented Oct 23, 2024 •

edited

Loading

ia0 left a comment

ia0 Oct 23, 2024

zhouwfang Oct 24, 2024

ia0 Oct 25, 2024

zhouwfang Oct 25, 2024 •

edited

Loading

ia0 Oct 25, 2024

ia0 commented Oct 23, 2024

ia0 left a comment

ia0 Oct 24, 2024

zhouwfang Oct 25, 2024 •

edited

Loading

ia0 Oct 25, 2024

zhouwfang Oct 25, 2024

ia0 Oct 25, 2024

zhouwfang Oct 25, 2024

ia0 Oct 25, 2024

ia0 left a comment

ia0 Oct 25, 2024

ia0 Oct 25, 2024

zhouwfang commented Oct 28, 2024

ia0 left a comment

ia0 Oct 28, 2024

ia0 Oct 28, 2024

ia0 Oct 28, 2024

ia0 Oct 28, 2024

zhouwfang commented Oct 29, 2024

	eprintln!("side-table conversion overflow {0}", source.result);
	eprintln!("side-table val_cnt overflow {}", source.result);

	eprintln!("side-table negative stack delta {source} < {target}");
	eprintln!("side-table negative stack delta {source} - {target}");

Add val_cnt and pop_cnt in side table #649

Are you sure you want to change the base?

Add val_cnt and pop_cnt in side table #649

Conversation

zhouwfang commented Oct 23, 2024

zhouwfang commented Oct 23, 2024 • edited Loading

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

zhouwfang Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ia0 commented Oct 23, 2024

ia0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouwfang Oct 25, 2024 • edited Loading

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

zhouwfang commented Oct 28, 2024

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

zhouwfang commented Oct 29, 2024

zhouwfang commented Oct 23, 2024 •

edited

Loading

zhouwfang Oct 25, 2024 •

edited

Loading

zhouwfang Oct 25, 2024 •

edited

Loading