-
Notifications
You must be signed in to change notification settings - Fork 27
fix(stack): stack management #331
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
have got this locally, didn't commit it because it has nothing to do with the current commit /// Store data in memory with at current memory byte pointer.
pub fn memory_write(&mut self, ty: impl Type) -> Result<MemoryInfo> {
let size = ty.align();
let offset = if size == 32 {
tracing::trace!("Writing result at offset=0, size={}", size);
// Pad I32 value to 32 bytes
if self.sp() > 0 {
// Duplicate stack value to preserve it
self.dup(1)?;
// Pop value to get it, pad to 32 bytes
let value = vec![0; 28].into_iter().chain(vec![self.asm.sp as u8]).collect::<Vec<u8>>();
self._drop()?; // Remove original value
self.push(&value)?;
}
smallvec::smallvec![0]
} else {
let offset = self.mp.to_ls_bytes();
self.increment_mp(size)?;
offset
};
// Push offset and MSTORE
self.push(&offset)?;
self._mstore()?;
Ok(MemoryInfo { offset, size })
} |
you are trying to hardcode the PC in the method, which is not the ideal way to go, if you want to leave the PC on stack, register it to the jump table (use zink/codegen/src/jump/relocate.rs Line 26 in cd1bb5a
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we might not need to store the result of each call in memory but leave them on stack, because calls could be used as:
- external call (part of the dispatcher)
- internal call
instead, we can repeat the logic of storing result in memory for the main_return (in dispatcher), it increases the code size, but makes our stack management perfect
zink/codegen/src/codegen/dispatcher.rs
Line 63 in cd1bb5a
| fn emit_selector(&mut self, selector: &wasm::Function<'_>, last: bool) -> Result<()> { |
codegen/src/visitor/call.rs
Outdated
| // Register the label to jump back. | ||
| let return_pc = self.masm.pc() + 2; | ||
| let return_pc = self.masm.pc() + 3; | ||
| self.masm.push(&return_pc.to_ls_bytes())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as mentioned, this is a hard coded PC as well, which is not correct
|
todos (growing-list)
|
|
Hello, @g4titanx I would like to start contributing to this project, just to confirm, this PR only need the tests to be updated to be ready ?If yes, I will give it a try sounds a great task to start |
|
hello @AurelienFT, yes you can start working on this and for more clarity of what's to be done, you should check this #324 |
resolves #324
now, i've got two major issues plaguing this pr. first is
approval::test_approvalfailing with an empty return (ret=[])instead of[0, ..., 0, 1]anderc20::deploythrowing anInvalidJump error. for approval, logs show modified_callcorrectly pushes 1,main_returnaligns the stack to sp=1, andmemory_writeexecutesMSTOREwith offset=0, but the EVM returns nothing, indicating MSTORE fails to write the padded 32-byte[0, ..., 0, 1]most likely due to improper stack value handling or memory misalignment.the second one is InvalidJump in
erc20.rswhich originates from incorrect jump target relocation intarget.rs, likely miscalculating offsets for function jumps (e.g., Func(19)), causing the EVM to hit an invalidJUMPDEST. the modified version ofmemory_writepreserves stack values but haven’t resolved the padding issue, and jump relocation needs tighter bounds checking to prevent buffer overruns