Skip to content
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

fix: store backpatch list as a temporary linked list within sidetable #119

Closed
wants to merge 1 commit into from
Closed

fix: store backpatch list as a temporary linked list within sidetable #119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
101 changes: 68 additions & 33 deletions src/validation/code.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,24 +115,32 @@ fn generate_unbackpatched_sidetable_entry(
) {
let stp_here = sidetable.len();

sidetable.push(SidetableEntry {
delta_pc: wasm.pc as isize,
delta_stp: stp_here as isize,
popcnt,
valcnt,
});

match label_info {
LabelInfo::Block { stps_to_backpatch } => stps_to_backpatch.push(stp_here),
LabelInfo::Loop { ip, stp } => {
//we already know where to jump to for loops
sidetable[stp_here].delta_pc = *ip as isize - wasm.pc as isize;
sidetable[stp_here].delta_stp = *stp as isize - stp_here as isize;
sidetable.push(SidetableEntry {
delta_pc: *ip as isize - wasm.pc as isize,
delta_stp: *stp as isize - stp_here as isize,
popcnt,
valcnt,
});
}
LabelInfo::If {
//use the delta_stp field temporarily as the "next" pointer of the linked list of unbackpatched entries of a particular label
//stps_to_backpatch field is the "head" pointer of this linked list, and is stored in the label
//-1 indicates the end of the linked list for the label
Comment on lines +128 to +130
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can negative values appear as stps_to_backpatch in normal code? Are the sets { -1 } and {valid values for stps_to_backpatch} disjoint?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm just a bit worried -1 can come and bite us back some time in the future. Having enums also does not guarantee size. If you can find a way to encode this as a valid enum, I'll be happy, but if not, make sure this is well documented, if not well tested

Copy link
Author

@cemonem cemonem Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Can negative values appear as stps_to_backpatch in normal code? Are the sets { -1 } and {valid values for stps_to_backpatch} disjoint?
I had thought of this issue and no it's not possible, unless there are more than 2^(isize-1)-1 jumping instructions within a module, that module has to be at least 2gb large then for 32 bit isize.

When thinking about it again now, I realized we could use 0 as NULL as well since if the sidetable is referred to, delta_stp is never 0, it is either positive or negative (in the cases it is negative, the jumping instruction refers to a loop instruction, sidetable of which needs no backpatching list, therefore this -1 is not a problem there). I could turn stps_to_backpatch into an usize too.

I'm just a bit worried -1 can come and bite us back some time in the future. Having enums also does not guarantee size. If you can find a way to encode this as a valid enum, I'll be happy, but if not, make sure this is well documented, if not well tested

We had a similar issue in elements pr as well. We could simply create an enum Optional<SomethingPtr>, under the hood this Optional<SomethingPtr> is an isize|usize with no tag, None is 0 and everything else is Some<SomethingPtr>. My rust-fu is not good to do this on the spot but we could look into it, I suspect it might be even already implemented like this for some fundamental types instead of a tagged union. This kind of thing would be used often in our interpreter I suspect.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I 100% think there is a type that has 0 as a variant and non-zero as another

NotZero or NotNull, something like that

LabelInfo::Block { stps_to_backpatch }
| LabelInfo::If {
stps_to_backpatch, ..
} => stps_to_backpatch.push(stp_here),
LabelInfo::Func { stps_to_backpatch } => stps_to_backpatch.push(stp_here),
}
| LabelInfo::Func { stps_to_backpatch } => {
sidetable.push(SidetableEntry {
delta_pc: wasm.pc as isize,
delta_stp: *stps_to_backpatch,
popcnt,
valcnt,
});
*stps_to_backpatch = stp_here as isize;
}
LabelInfo::Untyped => {
unreachable!("this label is for untyped wasm sequences")
}
Expand Down Expand Up @@ -198,7 +206,8 @@ fn read_instructions(
BLOCK => {
let block_ty = BlockType::read(wasm)?.as_func_type(fn_types)?;
let label_info = LabelInfo::Block {
stps_to_backpatch: Vec::new(),
//the linked list of unbackpatched sidetable entries that correspond to this label is empty so we initialize it as -1, standing in for null.
stps_to_backpatch: -1,
};
stack.assert_push_ctrl(label_info, block_ty)?;
}
Expand All @@ -215,6 +224,7 @@ fn read_instructions(

stack.assert_pop_val_type(ValType::NumType(NumType::I32))?;

//sidetable entry for the case the condition for If fails and we need to jump to the matching Else.
let stp_here = sidetable.len();
sidetable.push(SidetableEntry {
delta_pc: wasm.pc as isize,
Expand All @@ -224,8 +234,11 @@ fn read_instructions(
});

let label_info = LabelInfo::If {
//stp field holds the index of the entry above
stp: stp_here,
stps_to_backpatch: Vec::new(),
//the linked list of unbackpatched sidetable entries that correspond to this label is empty so we initialize it as -1, standing in for null.
//these sidetable entries correspond to jumps within the If and the corresponding Else blocks.
stps_to_backpatch: -1,
};
stack.assert_push_ctrl(label_info, block_ty)?;
}
Expand All @@ -237,24 +250,28 @@ fn read_instructions(
} = &mut label_info
{
if *stp == usize::MAX {
//this If was previously matched with an else already, it is already backpatched!
//this If was previously matched with an Else already, it is already backpatched!
return Err(Error::IfWithoutMatchingElse);
}

//sidetable entry for the unconditional jump at the ELSE instruction, when its corresponding If block executes (we shouldn't execute the else block then)
//similar to `generate_unbackpatched_sidetable_entry`, except the type validation is different
let stp_here = sidetable.len();
sidetable.push(SidetableEntry {
delta_pc: wasm.pc as isize,
delta_stp: stp_here as isize,
delta_stp: *stps_to_backpatch,
popcnt: 0,
valcnt: block_ty.returns.valtypes.len(),
});
stps_to_backpatch.push(stp_here);
*stps_to_backpatch = stp_here as isize;

//backpatch sidetable entry corresponding to case where the condition of the If block fails and we need to jump over it to execute this else block
sidetable[*stp].delta_pc = wasm.pc as isize - sidetable[*stp].delta_pc;
sidetable[*stp].delta_stp =
sidetable.len() as isize - sidetable[*stp].delta_stp;
*stp = usize::MAX; // mark the corresponding If as backpatched with usize::MAX

*stp = usize::MAX; // mark this If as backpatched

//type validation
for valtype in block_ty.returns.valtypes.iter().rev() {
stack.assert_pop_val_type(*valtype)?;
}
Expand Down Expand Up @@ -308,10 +325,17 @@ fn read_instructions(

match label_info {
LabelInfo::Block { stps_to_backpatch } => {
stps_to_backpatch.iter().for_each(|i| {
sidetable[*i].delta_pc = (wasm.pc as isize) - sidetable[*i].delta_pc;
sidetable[*i].delta_stp = (stp_here as isize) - sidetable[*i].delta_stp;
});
//follow the linked list indicated within the temporary entries of delta_stp
//and backpatch every entry.
let mut current_i = stps_to_backpatch;
while current_i != -1 {
let current = current_i as usize;
let next = sidetable[current].delta_stp;
sidetable[current].delta_pc =
(wasm.pc as isize) - sidetable[current].delta_pc;
sidetable[current].delta_stp = (stp_here as isize) - current_i;
current_i = next;
}
}
LabelInfo::If {
stp,
Expand All @@ -324,20 +348,31 @@ fn read_instructions(
sidetable[stp].delta_stp =
(stp_here as isize) - sidetable[stp].delta_stp;
}
stps_to_backpatch.iter().for_each(|i| {
sidetable[*i].delta_pc = (wasm.pc as isize) - sidetable[*i].delta_pc;
sidetable[*i].delta_stp = (stp_here as isize) - sidetable[*i].delta_stp;
});

//the rest is same as blocks.
let mut current_i = stps_to_backpatch;
while current_i != -1 {
let current = current_i as usize;
let next = sidetable[current].delta_stp;
sidetable[current].delta_pc =
(wasm.pc as isize) - sidetable[current].delta_pc;
sidetable[current].delta_stp = (stp_here as isize) - current_i;
current_i = next;
}
}
LabelInfo::Loop { .. } => (),
LabelInfo::Func { stps_to_backpatch } => {
// same as blocks, except jump just before the end instr, not after it
// the last end instruction will handle the return to callee during execution
stps_to_backpatch.iter().for_each(|i| {
sidetable[*i].delta_pc =
(wasm.pc as isize) - sidetable[*i].delta_pc - 1; // minus 1 is important!
sidetable[*i].delta_stp = (stp_here as isize) - sidetable[*i].delta_stp;
});
let mut current_i = stps_to_backpatch;
while current_i != -1 {
let current = current_i as usize;
let next = sidetable[current].delta_stp;
sidetable[current].delta_pc =
(wasm.pc as isize) - sidetable[current].delta_pc - 1; //minus 1 is important!
sidetable[current].delta_stp = (stp_here as isize) - current_i;
current_i = next;
}
}
LabelInfo::Untyped => unreachable!("this label is for untyped wasm sequences"),
}
Expand Down
11 changes: 7 additions & 4 deletions src/validation/validation_stack.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ impl ValidationStack {
stack: Vec::new(),
ctrl_stack: vec![CtrlStackEntry {
label_info: LabelInfo::Func {
stps_to_backpatch: Vec::new(),
stps_to_backpatch: -1,
},
block_ty,
height: 0,
Expand Down Expand Up @@ -345,18 +345,21 @@ impl CtrlStackEntry {
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum LabelInfo {
Block {
stps_to_backpatch: Vec<usize>,
// pointer to the temporary linked list of unbackpatched entries, embedded within the sidetable itself
stps_to_backpatch: isize,
},
Loop {
ip: usize,
stp: usize,
},
If {
stps_to_backpatch: Vec<usize>,
// pointer to the temporary linked list of unbackpatched entries, embedded within the sidetable itself
stps_to_backpatch: isize,
stp: usize,
},
Func {
stps_to_backpatch: Vec<usize>,
// pointer to the temporary linked list of unbackpatched entries, embedded within the sidetable itself
stps_to_backpatch: isize,
},
Untyped,
}
Expand Down
Loading