Fix ldc opcode in internal contexts #736
Conversation
Dentosal
added
bug
| Word::from_be_bytes(self.memory.read_bytes(code_size_ptr)?); | ||
| let old_code_size = padded_len_word(old_code_size) | ||
| .expect("Code size cannot overflow with padding"); |
There was a problem hiding this comment.
I would assume the size should already be padded when written in memory. Is it really correct to just compute padding here?
There was a problem hiding this comment.
Our code uses unpadded value in the CallFrame
There was a problem hiding this comment.
Yep. Shouldn't CallFrame use the padded value?
There was a problem hiding this comment.
I don't know=) The question is to the compiler team: do they use it somehow or not? Or is it used by anyone=)
Our specification says "Code size in bytes.", that sounds like exact size of the contract without padding.
There was a problem hiding this comment.
I'll ask the team, but a quick search from Sway codebase seems like they don't use this for anything. Time to remove? :D
There was a problem hiding this comment.
Doesnt "word-aligned" mean the same thing as padding here?
There was a problem hiding this comment.
Yep.
Moved to use padded size in 7335f9f.
… into dento/fix-ldc-frame-size-update
fuel-vm/src/interpreter/flow.rs
Outdated
| let frame_end = | ||
| (*self.registers.system_registers.fp) + CallFrame::serialized_size() as Word; |
There was a problem hiding this comment.
| let frame_end = | |
| (*self.registers.system_registers.fp) + CallFrame::serialized_size() as Word; | |
| let frame_end = | |
| (*self.registers.system_registers.fp) + total_size_in_stack as Word; |
There was a problem hiding this comment.
The name was wrong but the math correct. I've fixed this in 27ef70c
|
Could ou also update the Specification to specify this behaviour explicitly, please?=) |
|
| let code_start = | ||
| (*self.registers.system_registers.fp) + CallFrame::serialized_size() as Word; | ||
|
|
||
| *self.registers.system_registers.pc = code_start; |
There was a problem hiding this comment.
Before it was frame_end
Where frame_end is fp + total_size_in_stack
There was a problem hiding this comment.
No? Essentially it was before like this:
content_size = len - frame_len_with_padding
frame_end = fp + content_size # return valueThere was a problem hiding this comment.
Ohh, there is saturating_sub, I missed that=D
| @@ -2047,3 +2047,180 @@ fn coinbase_works() { | |||
|
|
|||
| assert_eq!(data.as_ref().unwrap(), &*expected); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn various_ldc_issues_poc() { | |||
There was a problem hiding this comment.
Is there any way to break this down in to more granular && specific tests? Tests like this are hard to maintain as it's not clear what behavior they are defining.
Introduced here: FuelLabs/fuel-vm#736 ### Before requesting review - [x] I have reviewed the code myself
LDC opcode was behaving incorrectly in internal contexts, i.e. when called from within a contract.
FuelLabs/fuel-specs#584
Checklist
Before requesting review