Fix GH-15834: Segfault with hook "simple get" cache slot and minimal JIT #17909
Conversation
…al JIT The FETCH_OBJ_R VM handler has an optimization that directly enters into a hook if it is a simpler getter hook. This is not compatible with the minimal JIT because the minimal JIT will try to continue executing the opcodes after the FETCH_OBJ_R. To solve this, we check whether the opcode is still the expected one after the execution of the VM handler. If it is not, we know that we are going to execute a simple hook. In that case, exit to the VM.
| goto jit_failure; | ||
| } | ||
|
|
||
| /* If a simple hook is called, exit to the VM. */ |
There was a problem hiding this comment.
Technically this could be omitted for certain property fetches, i.e. the ones that are known not to be polymorphic & don't contain hooks. More concretely, private and final properties.
| ir_ref if_hook_enter = ir_IF(jit_CMP_IP(jit, IR_EQ, opline + 1)); | ||
| ir_IF_FALSE(if_hook_enter); | ||
| if (GCC_GLOBAL_REGS) { | ||
| ir_TAILCALL(IR_VOID, ir_LOAD_A(jit_IP(jit))); | ||
| } else { | ||
| ir_RETURN(ir_CONST_I32(1)); /* ZEND_VM_ENTER */ | ||
| } | ||
| ir_IF_TRUE(if_hook_enter); | ||
|
|
There was a problem hiding this comment.
Checking for a possible hook call after each property read - looks stupid...
Previously we had magic __get and __set, but we didn't optimize their calls through trampolines...
I understand, this is targeted for "-O1", but this also will affect any non-constant property access.
Please check how this affect the resulting code size on typical opcache.jit=1205 and 1254 (on Wordpress and/or Symfony Demo).
Shouldn't you also do the same for FETCH_OBJ_W and maybe others?
Tracing JIT should be also affected by this problem.
There was a problem hiding this comment.
Previously we had magic __get and __set, but we didn't optimize their calls through trampolines...
Indeed, but this optimization makes get hooks as fast as getter functions. While the vast majority of public properties will not have hooks, and thus still improve performance, it was still important to us that hooks are not significantly slower than a getter function.
Shouldn't you also do the same for FETCH_OBJ_W and maybe others?
This optimization only exists for property read.
Checking for a possible hook call after each property read - looks stupid...
Maybe we can avoid this by creating a custom VM handler that is only used here, that omits the "simple get" optimization.
There was a problem hiding this comment.
Please check how do this work with non-constant property reads in tracing JIT. (I mean $this->{$name} that leads to read property hook).
I guess, tracing JIT also may need to be fixed.
There was a problem hiding this comment.
I won't have time anymore to check this today. I kinda missed that this codegen path may still be taken a lot of times.
For call VM we can improve the codegen to check the return value of the VM handler directly instead of comparing IP, that should make more compact machine code.
A custom VM specialization may help, the question would be if the trade-off seems reasonable.
I'll think a bit more about possible other solutions.
The FETCH_OBJ_R VM handler has an optimization that directly enters into a hook if it is a simpler getter hook. This is not compatible with the minimal JIT because the minimal JIT will try to continue executing the opcodes after the FETCH_OBJ_R.
To solve this, we check whether the opcode is still the expected one after the execution of the VM handler. If it is not, we know that we are going to execute a simple hook. In that case, exit to the VM.
Normally, I wouldn't care too much about this issue, because it is in minimal JIT. However, someone had this cause a crash to them by accident: #17767