Fix GH-20838: JIT compiler produces wrong arithmetic results

[iliaal]

Summary

• When an opcode falls through to the handler path in JIT trace compilation and an operand has MAY_BE_GUARD, the guard is widened to MAY_BE_ANY but never emitted as a runtime type check
• The handler can produce a result type different from the TSSA prediction (e.g. IS_LONG instead of IS_DOUBLE for MUL with mixed string/numeric operands), but SET_STACK_TYPE unconditionally records the predicted concrete type as mem_type
• Side traces entering via the exit point then load the slot from memory assuming the predicted type, interpreting raw IS_LONG bytes as IEEE 754 double (producing values like 3.7054923438093E-322 instead of 75)
• Fix: don't trust the TSSA concrete result type when the handler path is taken and any operand had MAY_BE_GUARD

Fixes #20838

[dstogov]

@iliaal thanks for the deep analyses.
Unfortunate, even with your explanation, it took me a while to understand he problem.

• MUL may be JIT-ed as a call to VM handler (when one of operands is not numeric).
• in this case we don't emit required type guard code for the other MUL operand
• this leads to inconsistency in type inference for the result of MUL and potentially for all its successors

So your identified the problem completely right, but I;m not sure about the fix.

I think, the proper fix for the problem should be emitting the type guard code before calling the VM handler.
It seems PHP-8.3 worked this way. JIT-ing MUL, it checks CHECK_OP*_TRACE_TYPE before fall-back to VM handler.
I'm not completely sure...

Do you like to finalize the fix yourself?

[iliaal]

@dstogov Let me re-review the patch and suggest an update

[iliaal]

@dstogov You're right, the proper fix is emitting the type guards before the handler call. Updated the patch -- now CHECK_OP1_TRACE_TYPE() and CHECK_OP2_TRACE_TYPE() are called in the handler fallback path (before the MAY_BE_GUARD widening to MAY_BE_ANY), and the workaround in SET_STACK_TYPE is reverted.

If the guard passes, MAY_BE_GUARD is cleared and the subsequent widening is skipped, so the TSSA result type prediction stays valid. If the guard fails, we side-exit. Full JIT test suite passes with no regressions.

[dstogov]

I thought, about moving the while following if statements with zend_jit_fetch_reference() up, but probably your solution is even better. It should generate less code.

It makes sense to remove CHECK_OP*_TRACE_TYPE() from the else parts of the following ifs.
This shouldn't change any behavior.

Please re-target this PR to PHP-8.4.

[dstogov]

It should be verified, that hoisting CHECK_OP*_TRACE_TYPE() doesn't affect zend_jit_fetch_reference() path. I'll try to do this tomorrow...

[iliaal]

@dstogov Rebased onto PHP-8.4 and re-targeted the PR.

Regarding removing CHECK_OP*_TRACE_TYPE() from the else parts of the specialized opcode ifs -- I'd prefer to keep those as a separate follow-up once you've verified the zend_jit_fetch_reference() interaction. That way this fix stays minimal and easy to review. Happy to do the cleanup in a follow-up commit if you'd like.

[dstogov]

After all I think that the best fix is inserting "necessary" type guards on the "sensitive" paths.
This way we don't affect the usual numeric logic and don't insert useless guards when fall-back to VM handler.

diff --git a/ext/opcache/jit/zend_jit_trace.c b/ext/opcache/jit/zend_jit_trace.c
index 10be237d06b..82e173ba499 100644
--- a/ext/opcache/jit/zend_jit_trace.c
+++ b/ext/opcache/jit/zend_jit_trace.c
@@ -4556,6 +4556,12 @@ static zend_vm_opcode_handler_t zend_jit_trace(zend_jit_trace_rec *trace_buffer,
 						op2_info = OP2_INFO();
 						op2_addr = OP2_REG_ADDR();
 						if ((op1_info & MAY_BE_UNDEF) || (op2_info & MAY_BE_UNDEF)) {
+							if (op1_type == IS_LONG || op1_type == IS_DOUBLE) {
+								CHECK_OP1_TRACE_TYPE();
+							}
+							if (op2_type == IS_LONG || op2_type == IS_DOUBLE) {
+								CHECK_OP2_TRACE_TYPE();
+							}
 							break;
 						}
 						if (opline->opcode == ZEND_ADD &&
@@ -4564,6 +4570,12 @@ static zend_vm_opcode_handler_t zend_jit_trace(zend_jit_trace_rec *trace_buffer,
 							/* pass */
 						} else if (!(op1_info & (MAY_BE_LONG|MAY_BE_DOUBLE)) ||
 						    !(op2_info & (MAY_BE_LONG|MAY_BE_DOUBLE))) {
+							if (op1_type == IS_LONG || op1_type == IS_DOUBLE) {
+								CHECK_OP1_TRACE_TYPE();
+							}
+							if (op2_type == IS_LONG || op2_type == IS_DOUBLE) {
+								CHECK_OP2_TRACE_TYPE();
+							}
 							break;
 						}
 						if (orig_op1_type != IS_UNKNOWN

If you don't see problems or possible improvements - just commit this.

(I saw your other 2 PRs, but I can't dedicate them the required time right now. They are in my queue and I'll review them later).

[iliaal]

@dstogov Applied your diff as-is -- guards only on the sensitive bailout paths, conditioned on IS_LONG || IS_DOUBLE. Makes sense to narrow the scope and doesn't affect the end-result according to tests.

P.S. If looks good to you then feel free to merge latest version, I don't have direct commit/merge access at the moment

[dstogov]

Thanks!