ZJIT: Fix crashes and a miscomp for param spilling #13802
Conversation
zjit/src/codegen.rs
Outdated
| @@ -216,6 +215,8 @@ fn gen_function(cb: &mut CodeBlock, iseq: IseqPtr, function: &Function) -> Optio | |||
| let new_sp = asm.sub(NATIVE_STACK_PTR, jit.c_stack_bytes.into()); | |||
| asm.mov(NATIVE_STACK_PTR, new_sp); | |||
| } | |||
|
|
|||
| gen_entry_params(&mut asm, iseq, function.block(BlockId(0))); | |||
There was a problem hiding this comment.
We deliberately put this in gen_entry() instead of here so that JIT-to-JIT calls don't have to read arguments off of EP. When we optimize frame push further by removing eager spilling, this would stop working.
Could we adjust operands in jit.opnds for params if they point to memory based off of NATIVE_STACK_PTR instead? If it doesn't work, let's leave a TODO comment to optimize argument access on JIT-to-JIT calls.
There was a problem hiding this comment.
Could we adjust operands in jit.opnds for params if they point to memory based off of NATIVE_STACK_PTR instead?
Or let the backend do it? https://github.com/ruby/ruby/pull/13789/files#r2190770455
There was a problem hiding this comment.
Wrote to the slots at entry point in 604c715.
Now that params can be in memory, this particular load_into() was panicking with "Invalid operands for LDUR" with test_spilled_method_args() on ARM. Since it's documented to be for register destinations let's validate it.
Previously, gen_param() access slots at `SP-x` for `x≥0` after subtracting from SP, so it was accessing slots from above the top of the stack. Also, the slots gen_entry_params() wrote to at entry point did not correspond to the slots access inside the JIT function. Redo the stack frame layout so that inside the function slots are at `SP+x`. Write to those slots in the entry point by anticipating the size of the frame. Fixes test_spilled_method_args().
XrXr
force-pushed
the
zjit-param-spill-fixes
branch
from
64534bf to
604c715
Compare
| @@ -2150,6 +2150,7 @@ impl Assembler { | |||
| } | |||
|
|
|||
| pub fn load_into(&mut self, dest: Opnd, opnd: Opnd) { | |||
| assert!(matches!(dest, Opnd::Reg(_) | Opnd::VReg{..}), "Destination of load_into must be a register"); | |||
There was a problem hiding this comment.
This feels a little too lax. Isn't there a chance the vreg is allocated to memory?
There was a problem hiding this comment.
There is a lot of code around register allocation that assume VReg means reg and I think there's no way a VReg becomes memory at the moment (because of no auto spills). I figure when they could become memory LIR would make some rewrites to completely erase these.
Also, maybe this is beside the point and we should remove these constrained movs (like Store) and use the general mov for everything. I guess these are still useful in spots where we don't run the split (legalization) backend pass...
Easiest to review each commit.
asm.load_into(Mem, ..)and avoid it in gen_entry_params()Still not completely debugged since there is still some issue with
it clobbering the self parameter, which is x0 on ARM.
It gets clobbered in and outside of gen_entry_params(), so it's not
easily fixable by just rejigging the order in there.