Always load EX(opline) into the current frame in tracing JIT when observers are enabled #13776

bwoebi · 2024-03-21T14:12:14Z

I.e. disabling the optimization of the tracing jit where setting EX(opline) is elided.

arnaud-lb

This looks good to me in the sense that IP is loaded in all path leading to the two separate EXT_CALL zend_observer_fcall_begin, but let's wait for Dmitry's review.

arnaud-lb · 2024-03-21T15:14:02Z

ext/opcache/tests/jit/gh13772.phpt

+function Ack($m, $n) {
+	if ($m == 0) return $n+1;
+	if ($n == 0) return Ack($m-1, 1);
+	return Ack($m - 1, Ack($m, ($n - 1)));
+}
+
+var_dump(Ack(3, 3));


We could also cover the call_num_args > func->op_array.num_args case here with

function Ack($m, $n) { if ($m == 0) return $n+1; if ($n == 0) return Ack($m-1, 1, $n); return Ack($m - 1, Ack($m, ($n - 1), $n)); } var_dump(Ack(3, 3, 3));

dstogov · 2024-03-21T20:17:21Z

ext/opcache/jit/zend_jit_arm64.dasc

-				if (!trace || (trace->op == ZEND_JIT_TRACE_END
+				if (!trace || ZEND_OBSERVER_ENABLED || (trace->op == ZEND_JIT_TRACE_END


No. This is completely wrong. You don't need to calculate and load opline into register.

In tracing JIT, we often inline the function calls and the first opline of the inlined function is known.
It's must be recorded in the trace. Probably it should be in the next trace ZEND_JIT_TRACE_VM record.
So for inlined trace functions we should load the EX(opline) by SET_EX_OPLINE opline instead of SAVE_IP.

dstogov · 2024-03-25T07:09:29Z

ext/opcache/jit/zend_jit_x86.dasc

-			|	SAVE_IP
+			|	SET_EX_OPLINE opline, r0


Something looks wrong.

FP at this point keeps the execute_data of the called function, but you pass opline of the caller function.
Right? Or may be I miss something?

I think it's better to keep SAVE_IP for cases when the previous code calculates the value in IP (we just can't always know the value of opline at compile-time).

Few observer related tests on ARM are failed.

Yes, this is wrong. I tried it because you were suggesting So for inlined trace functions we should load the EX(opline) by SET_EX_OPLINE opline instead of SAVE_IP. I probably misunderstood what you meant.

However, I honestly don't understand the problem with 51c74a4. In general, this will go to LOAD_IP_ADDR (func->op_array.opcodes + num_args) which is later followed by SAVE_IP in the if (ZEND_OBSERVER_ENABLED) branch.

Which boils down to two instructions:

|| if (GCC_GLOBAL_REGS) { | LOAD_ADDR IP, addr <= mov reg, ((ptrdiff_t)addr) || } else { | ADDR_STORE aword EX->opline, addr, RX <= mov mem, ((ptrdiff_t)addr) || } || if (GCC_GLOBAL_REGS) { | mov aword EX->opline, IP || }

There are, to me, two choices, either make the code more complex by conditionally not doing the SAVE_IP and directly assigning ADDR_STORE aword EX->opline, (func->op_array.opcodes + num_args), r0. Or just do what I did in my original commit.

If you have a better alternative than 51c74a4, please feel welcome to propose one. At least I don't see one.

Yes, this is wrong. I tried it because you were suggesting So for inlined trace functions we should load the EX(opline) by SET_EX_OPLINE opline instead of SAVE_IP. I probably misunderstood what you meant.

I meant, it should be possible to extract the first opline of the function from the the trace and load it by SET_EX_OPLINE. Something like the following (I didn't check this):

if (trac && (trace+1).op == ZEND_JIT_TRACE_VM) { SET_EX_OPINE (trace+1).opline, r0 } else { SAVE_IP }

However, I honestly don't understand the problem with 51c74a4. In general, this will go to LOAD_IP_ADDR (func->op_array.opcodes + num_args) which is later followed by SAVE_IP in the if (ZEND_OBSERVER_ENABLED) branch.

This will execute code that wasn't designed for tracng JIT. I'm not sure if it's going to be right or not.

In any case, this code is going to do extra work (calculation the known address).

Tried that, but trace[1].op may be ZEND_JIT_TRACE_END immediately, if the code was fully inlined.

(gdb) p func->op_array.opcodes $7 = (zend_op *) 0x748cb840 (gdb) p *trace[1].opline $8 = {handler = 0x7bff75d0, op1 = {constant = 80, var = 80, num = 80, opline_num = 80, jmp_offset = 80}, op2 = {constant = 4294967184, var = 4294967184, num = 4294967184, opline_num = 4294967184, jmp_offset = 4294967184}, result = {constant = 112, var = 112, num = 112, opline_num = 112, jmp_offset = 112}, extended_value = 0, lineno = 4, opcode = 18 '\022', op1_type = 8 '\b', op2_type = 1 '\001', result_type = 18 '\022'} (gdb) p trace[1].op $9 = 8 '\b'

The normal location for EX(opline) on a function call is op_array->opcodes + MIN(op_array->num_args, call_num_args), which is basically what

uint32_t num_args; if ((func->op_array.fn_flags & ZEND_ACC_HAS_TYPE_HINTS) != 0) { if (trace) { num_args = 0; } else if (call_info) { num_args = skip_valid_arguments(op_array, ssa, call_info); } else { num_args = call_num_args; } } else { num_args = call_num_args; } if (zend_accel_in_shm(func->op_array.opcodes)) { | LOAD_IP_ADDR (func->op_array.opcodes + num_args)

that code computes, i.e. the part enabled by 51c74a4.

dstogov · 2024-04-04T10:05:27Z

ext/zend_test/observer.c

+	ZEND_ASSERT(!ZEND_USER_CODE(EX(func)->type) ||
+		(EX(opline) >= EX(func)->op_array.opcodes && EX(opline) < EX(func)->op_array.opcodes + EX(func)->op_array.last));
+


I would recommend to insert this kind of assertions into all other handlers.

make test TESTS="--asan -d zend_test.observer.enabled=1 -d zend_test.observer.observe_all=1 -d zend_test.observer.show_output=0 -d opcache.jit=0 -j16 Zend tests ext/opcache ext/zend_test" shows 71 failures.

Some of them are false positives, but many failures are caused by this assertion. They are mostly related to generators and fibers. This is even without JIT!

Added to end handler too. The assertion itself was missing EG(exception_op) handling, good catch.

dstogov · 2024-04-04T10:22:48Z

ext/opcache/jit/zend_jit_x86.dasc

-			|	SAVE_IP
+			if (trace && trace[1].op == ZEND_JIT_TRACE_VM) {
+				|	SET_EX_OPLINE trace[1].opline, r0
+			} else {
+				|	SAVE_IP
+			}


This still doesn't work. bench.php crashes.
I think, this should be changed into:

diff --git a/ext/opcache/jit/zend_jit_x86.dasc b/ext/opcache/jit/zend_jit_x86.dasc index 7c92d88351a..b4c9f87f03e 100644 --- a/ext/opcache/jit/zend_jit_x86.dasc +++ b/ext/opcache/jit/zend_jit_x86.dasc @@ -10221,7 +10221,12 @@ static int zend_jit_do_fcall(dasm_State **Dst, const zend_op *opline, const zend } if (ZEND_OBSERVER_ENABLED) { - | SAVE_IP + if (trace) { + ZEND_ASSERT(trace[1].op == ZEND_JIT_TRACE_VM || trace[1].op == ZEND_JIT_TRACE_END); + | SET_EX_OPLINE trace[1].opline, r0 + } else { + | SAVE_IP + } | mov FCARG1a, FP | EXT_CALL zend_observer_fcall_begin, r0 }

This is not quite right, I added && (trace->op != ZEND_JIT_TRACE_END || trace->stop != ZEND_JIT_TRACE_STOP_INTERPRETER) - I think that's better then. Otherwise trace[1] may be garbage.

dstogov

It seems like the observer API need to be fixed even without JIT (or may be the new assertion is not correct).

Please add the EX(opline) assertions into all relevant observer handlers and fix the related tests failures.

bwoebi · 2024-04-07T13:30:50Z

@dstogov now it's green :-) Thanks!

dstogov · 2024-04-08T07:10:36Z

ext/opcache/jit/zend_jit_x86.dasc

+			if (trace && (trace->op != ZEND_JIT_TRACE_END || trace->stop != ZEND_JIT_TRACE_STOP_INTERPRETER)) {
+				ZEND_ASSERT(trace[1].op == ZEND_JIT_TRACE_VM || trace[1].op == ZEND_JIT_TRACE_END);
+				|	SET_EX_OPLINE trace[1].opline, r0
+			} else {
+				|	SAVE_IP
+			}


Are you sure SAVE_IP should be called for tracing JIT?

From the first look, I would refer to check the direct condition - trace[1].op == ZEND_JIT_TRACE_VM || trace[1].op == ZEND_JIT_TRACE_END.
Why did you prefer the reverse one - trace->op != ZEND_JIT_TRACE_END || trace->stop != ZEND_JIT_TRACE_STOP_INTERPRETER?

Because it's the reverse condition of:

if (!trace || (trace->op == ZEND_JIT_TRACE_END && trace->stop == ZEND_JIT_TRACE_STOP_INTERPRETER)) {

(lines 10077) and which turns out to be the only case when trace[1] does not exist. But if that code is executed, e.g. line 10097 directly writes to IP, but not EX(opline):

| mov IP, aword [r0 + offsetof(zend_op_array, opcodes)]

so, not calling SAVE_IP would not store that.

dstogov · 2024-04-08T07:11:35Z

ext/zend_test/observer.c

+static inline void assert_observer_opline(zend_execute_data *execute_data) {
+	ZEND_ASSERT(!ZEND_USER_CODE(EX(func)->type) ||
+		(EX(opline) >= EX(func)->op_array.opcodes && EX(opline) < EX(func)->op_array.opcodes + EX(func)->op_array.last) ||
+		(EX(opline) >= EG(exception_op) && EX(opline) < EG(exception_op) + 3));


Interesting catch!
Should the begin/end observer handlers be called in case of exception?

Yes.
For begin observers: generators may be resumed with an exception and continue execution (after a catch). Not calling them would hide normal execution.
For end observers - we for sure want to observe thrown exceptions.

dstogov

The patch seems to fix all the related problems.
Please answer my last minor questions.

Also, why this is targeted to PHP_8_3 and not to PHP_8_2?

…re enabled Fixes php#13772.

bwoebi · 2024-04-08T10:37:30Z

@dstogov Because it was reported on PHP 8.3. But you are obviously right, it applies to PHP 8.2 too. Changed.

dstogov

Looks good.

bwoebi requested review from dstogov and iluuu1994 as code owners March 21, 2024 14:12

github-actions bot added Extension: opcache Extension: zend_test labels Mar 21, 2024

arnaud-lb reviewed Mar 21, 2024

View reviewed changes

dstogov requested changes Mar 21, 2024

View reviewed changes

bwoebi force-pushed the bob/fix-gh13772 branch from 51c74a4 to 4c8ff48 Compare March 24, 2024 22:48

dstogov requested changes Mar 25, 2024

View reviewed changes

bwoebi force-pushed the bob/fix-gh13772 branch from 4c8ff48 to a6fb71c Compare April 1, 2024 17:48

dstogov reviewed Apr 4, 2024

View reviewed changes

dstogov requested changes Apr 4, 2024

View reviewed changes

bwoebi force-pushed the bob/fix-gh13772 branch 4 times, most recently from b2825ac to 805a336 Compare April 7, 2024 12:55

bwoebi changed the title ~~Always load EX(opline) into the current frame in JIT when observers are enabled~~ Always load EX(opline) into the current frame in tracing JIT when observers are enabled Apr 7, 2024

dstogov reviewed Apr 8, 2024

View reviewed changes

Always load EX(opline) into the current frame in JIT when observers a…

1330e49

…re enabled Fixes php#13772.

bwoebi force-pushed the bob/fix-gh13772 branch from 805a336 to 1330e49 Compare April 8, 2024 10:36

bwoebi changed the base branch from PHP-8.3 to PHP-8.2 April 8, 2024 10:36

dstogov approved these changes Apr 8, 2024

View reviewed changes

bwoebi closed this in af098ac Apr 8, 2024

		if (!trace \|\| (trace->op == ZEND_JIT_TRACE_END
		if (!trace \|\| ZEND_OBSERVER_ENABLED \|\| (trace->op == ZEND_JIT_TRACE_END

		ZEND_ASSERT(!ZEND_USER_CODE(EX(func)->type) \|\|
		(EX(opline) >= EX(func)->op_array.opcodes && EX(opline) < EX(func)->op_array.opcodes + EX(func)->op_array.last));

Always load EX(opline) into the current frame in tracing JIT when observers are enabled #13776

Always load EX(opline) into the current frame in tracing JIT when observers are enabled #13776

Uh oh!

Conversation

bwoebi commented Mar 21, 2024

Uh oh!

arnaud-lb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dstogov Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

bwoebi commented Apr 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

bwoebi commented Apr 8, 2024

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dstogov Mar 21, 2024 •

edited

Loading

bwoebi Mar 25, 2024 •

edited

Loading