interpreter: Fix out-of-range execution ProgramCounter #303
Conversation
crates/polkavm/src/tests.rs
Outdated
| instance.set_reg(Reg::RA, crate::RETURN_TO_HOST); | ||
| instance.set_next_program_counter(ProgramCounter(0)); | ||
| match_interrupt!(instance.run().unwrap(), InterruptKind::Trap); | ||
| assert_eq!(instance.program_counter(), Some(ProgramCounter(9))); |
There was a problem hiding this comment.
Don't hardcode this offset. Parse the instructions to get the offset (e.g. scroll up and take a look at the previous simple_test where it does something like that).
crates/polkavm/src/tests.rs
Outdated
| @@ -3797,6 +3814,8 @@ run_tests! { | |||
| trapping_preserves_all_registers_normal_trap | |||
| trapping_preserves_all_registers_segfault | |||
|
|
|||
| test_out_of_range_execution | |||
There was a problem hiding this comment.
test_out_of_range_execution -> out_of_range_execution
crates/polkavm/src/interpreter.rs
Outdated
| let (pc, branch_taken) = if callback(s1, s2) { | ||
| (ProgramCounter(target_true), true) | ||
| } else { | ||
| (ProgramCounter(target_false), false) | ||
| }; | ||
|
|
||
| if let Some(target) = self.inner.resolve_jump::<DEBUG>(pc) { | ||
| Some(target) | ||
| } else { | ||
| Some(target_false) | ||
| self.inner.unresolved_program_counter = Some(pc); | ||
| if branch_taken { | ||
| Some(TARGET_INVALID_BRANCH) | ||
| } else { | ||
| Some(TARGET_OUT_OF_RANGE) | ||
| } |
There was a problem hiding this comment.
Hm, I don't think we want to do this here.
The whole point of the two-stage lazy "compilation" thing we have here in the interpreter is that we can do work like resolving jumps only once when we "compile" a given instruction, and then for any subsequent execution that work can be skipped. That's why by default the branches initially trigger the unresolved_* handlers, which resolve the jumps and replace the handler in-place with one that doesn't need to do it again.
There was a problem hiding this comment.
Fixed in the latest iteration.
aman4150
force-pushed
the
interpreter_oor_exec
branch
2 times, most recently
from
e110acd to
e2b67d2
Compare
Currently, the program counter always returns 0 when the last instruction is branch instruction and it evaluates to false. The correct behavior should be returning the faulting address. This is also weird because the program counter returns correct value if the last instruction is something else. Also, on re-compiler we do return correct faulting address. This patch fixes interpreter behavior and makes it consistent with re-compiler. Signed-off-by: Aman <[email protected]>
aman4150
force-pushed
the
interpreter_oor_exec
branch
from
e2b67d2 to
819d36c
Compare
| - name: Install clippy (zygote toolchain) | ||
| run: cd crates/polkavm-zygote && rustup component add clippy |
There was a problem hiding this comment.
I think, we already do this for ubuntu-* setups.
There was a problem hiding this comment.
This is clippy for zygote. Zygote is Linux-only, and this is action for macOS.
| let s1 = self.get64(s1); | ||
| let s2 = self.get64(s2); | ||
| if callback(s1, s2) { | ||
| Some(target_true) | ||
|
|
||
| let (packed_target, branch_taken) = if callback(s1, s2) { | ||
| (target_true, true) | ||
| } else { | ||
| (target_false, false) | ||
| }; | ||
|
|
||
| let (is_jump_target_valid, target) = InterpretedInstance::unpack_target(NonZeroU32::new(packed_target).unwrap()); | ||
|
|
||
| if !is_jump_target_valid { | ||
| self.inner.unresolved_program_counter = Some(ProgramCounter(target)); | ||
| if branch_taken { | ||
| Some(TARGET_INVALID_BRANCH) | ||
| } else { | ||
| Some(TARGET_OUT_OF_RANGE) | ||
| } | ||
| } else { | ||
| Some(target_false) | ||
| Some(target) | ||
| } |
There was a problem hiding this comment.
This is still not exactly what we want. :P
In general we'd want this handler to stay exactly the same way as it was, and instead have separate handler(s) for this special case. Again, we want to minimize the execution cost - the vast majority of branches will not need this, so they shouldn't pay the cost of handling this.
There was a problem hiding this comment.
The extra penalty is testing the MSB now. Is that a lot?
I don't think there is any other way to implement this (without major refactoring, I think).
There was a problem hiding this comment.
You shouldn't need major refactoring; just use a separate handler for cases where the jump target is not valid, no?
There was a problem hiding this comment.
Just for my understanding, you are saying we should go ahead with defining all 17 handlers for unresolved jump flavor?
I still don't follow how testing MSB is adding complexity though. As you already said, most of the code would not trigger this out-of-range logic, the branch predictor would quickly learn to execute non-out-of-range code path, and there is no penalty to be paid.
There was a problem hiding this comment.
there is no penalty to be paid
There's always a penalty, even if this won't trigger a branch miss, and branches are very common.
Just for my understanding, you are saying we should go ahead with defining all 17 handlers for unresolved jump flavor?
No, you don't need to explicitly define new handlers. Check which type of handler needs to be used in handle_unresolved_branch, then add a new const generic param to the handlers which will decide whether it's a branch with an invalid jump target (so then you don't have to do this check at runtime inside the handler).
Currently, the program counter always returns 0 when the last instruction is branch instruction and it evaluates to false. The correct behavior should be returning the faulting address.
This is also weird because the program counter returns correct value if the last instruction is something else.
Also, on re-compiler we do return correct faulting address.
This patch fixes interpreter behavior and makes it consistent with re-compiler.