Skip to content

Complete implementation for Verify #744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 17, 2025
Merged

Complete implementation for Verify #744

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2d27005
feat: complete impl for Verify mode
zhouwfang Feb 16, 2025
6b4947c
refactor: remove pub
zhouwfang Feb 16, 2025
40e7c38
review
ia0 Feb 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 23 additions & 19 deletions crates/interpreter/src/valid.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,13 +132,18 @@ impl<'m> BranchTableApi<'m> for &mut Vec<BranchTableEntry> {
}
}

struct MetadataView<'m> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good to have side table views specific to verification in valid.rs because exec.rs will need different views (it won't need the mutable index used only for verification).

metadata: Metadata<'m>,
branch_idx: usize,
}

#[derive(Default)]
struct Verify;
impl ValidMode for Verify {
/// Contains at most one _target_ branch. Source branches are eagerly patched to
/// their target branch using the branch table.
type Branches<'m> = Option<SideTableBranch<'m>>;
type BranchTable<'a, 'm> = Metadata<'m>;
type BranchTable<'a, 'm> = MetadataView<'m>;
type SideTable<'m> = SideTableView<'m>;
type Result = ();

Expand All @@ -152,11 +157,11 @@ impl ValidMode for Verify {
fn next_branch_table<'a, 'm>(
side_table: &'a mut Self::SideTable<'m>, type_idx: usize, parser_range: Range<usize>,
) -> Result<Self::BranchTable<'a, 'm>, Error> {
let branch_table = side_table.metadata(side_table.func_idx);
let metadata = side_table.metadata(side_table.func_idx);
side_table.func_idx += 1;
check(branch_table.type_idx() == type_idx)?;
check(branch_table.parser_range() == parser_range)?;
Ok(branch_table)
check(metadata.type_idx() == type_idx)?;
check(metadata.parser_range() == parser_range)?;
Ok(MetadataView { metadata, branch_idx: 0 })
}

fn side_table_result(side_table: Self::SideTable<'_>) -> Self::Result {
Expand All @@ -170,26 +175,29 @@ impl<'m> BranchesApi<'m> for Option<SideTableBranch<'m>> {
}
}

impl<'m> BranchTableApi<'m> for Metadata<'m> {
impl<'m> BranchTableApi<'m> for MetadataView<'m> {
fn stitch_branch(
&mut self, source: SideTableBranch<'m>, target: SideTableBranch<'m>,
) -> CheckResult {
check(source == target)
}

fn patch_branch(&self, mut source: SideTableBranch<'m>) -> Result<SideTableBranch<'m>, Error> {
let entry = self.branch_table()[source.branch_table].view();
source.branch_table = self.branch_idx;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit surprised by this. In theory, we should have source.branch_table + 1 == self.branch_idx, so this smells like an off-by-one error. Let's merge like this but keep it in mind. This might be a source of error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why you believe so?

Before this function, next_index() and allocate_branch() are called.

For Prepare, next_index() returns the length of the branch table, and allocate_branch() pushes an invalid entry to the branch table, which patch_branch returns.

For Verify, to be consistent with Prepare, I thought we should also use the next branch in patch_branch().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Verify, to be consistent with Prepare, I thought we should also use the next branch in patch_branch().

What does this mean?

patch_branch() should only do something in Verify (which is why it's the identity function in Prepare). In Verify it is supposed to convert the source branch to its target branch using the branch table. The branch index is given by the source branch. We shouldn't access branch_idx at all in this function. This field is only meaningful for next_index() and allocated_branch(), because it represents the current length of the branch table. In practice, patch_branch() is called right after the source branch is created, so it's the current last branch (thus its index is one less than the current length), but we shouldn't rely on this, in particular because it doesn't bring anything.

let entry = self.metadata.branch_table()[source.branch_table].view();
offset_front(source.parser, entry.delta_ip as isize);
source.branch_table += entry.delta_stp as usize;
source.result = entry.val_cnt as usize;
source.stack -= entry.pop_cnt as usize;
Ok(source)
}

fn allocate_branch(&mut self) {}
fn allocate_branch(&mut self) {
self.branch_idx += 1;
}

fn next_index(&self) -> usize {
0
self.branch_idx
}
}

Expand Down Expand Up @@ -704,11 +712,7 @@ impl<'a, 'm, M: ValidMode> Expr<'a, 'm, M> {
let result = self.label().type_.results.len();
let mut target = self.branch_target(result);
target.branch_table += 1;
M::BranchTable::stitch_branch(
self.branch_table.as_mut().unwrap(),
source,
target,
)?;
self.branch_table.as_mut().unwrap().stitch_branch(source, target)?;
}
_ => Err(invalid())?,
}
Expand Down Expand Up @@ -975,14 +979,14 @@ impl<'a, 'm, M: ValidMode> Expr<'a, 'm, M> {
let results_len = self.label().type_.results.len();
let mut target = self.branch_target(results_len);
for source in branches {
M::BranchTable::stitch_branch(self.branch_table.as_mut().unwrap(), source, target)?;
self.branch_table.as_mut().unwrap().stitch_branch(source, target)?;
}
let label = self.label();
if let LabelKind::If(source) = label.kind {
check(label.type_.params == label.type_.results)?;
// SAFETY: This function is only called after parsing an End instruction.
target.parser = offset_front(target.parser, -1);
M::BranchTable::stitch_branch(self.branch_table.as_mut().unwrap(), source, target)?;
self.branch_table.as_mut().unwrap().stitch_branch(source, target)?;
}
}
let results = self.label().type_.results;
Expand All @@ -999,15 +1003,15 @@ impl<'a, 'm, M: ValidMode> Expr<'a, 'm, M> {
let n = self.labels.len();
check(l < n)?;
let source = self.branch_source();
let source = M::BranchTable::patch_branch(self.branch_table.as_ref().unwrap(), source)?;
let source = self.branch_table.as_ref().unwrap().patch_branch(source)?;
let label = &mut self.labels[n - l - 1];
Ok(match label.kind {
LabelKind::Block | LabelKind::If(_) => {
M::Branches::push_branch(&mut label.branches, source)?;
label.branches.push_branch(source)?;
label.type_.results
}
LabelKind::Loop(target) => {
M::BranchTable::stitch_branch(self.branch_table.as_mut().unwrap(), source, target)?;
self.branch_table.as_mut().unwrap().stitch_branch(source, target)?;
label.type_.params
}
})
Expand Down
Loading