Skip to content

Fix JIT QM_ASSIGN may be optimized out when op1 is null #13610

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

arnaud-lb
Copy link
Member

@arnaud-lb arnaud-lb commented Mar 6, 2024

ZVAL_COPY_CONST and its variations avoid updating Z_TYPE_P(dst_addr) when it's known to have the right value already, based on dst_info. However, when emitting code for ZEND_QM_ASSIGN, we add MAY_BE_NULL to dst_info when the SSA type and stack types disagree. I'm assuming this is done to force a type store here:

|| if ((dst_info & (MAY_BE_ANY|MAY_BE_UNDEF|MAY_BE_GUARD)) != MAY_BE_DOUBLE) {
| SET_ZVAL_TYPE_INFO dst_addr, IS_DOUBLE
|| }
|| } else if (((dst_info & (MAY_BE_ANY|MAY_BE_UNDEF|MAY_BE_GUARD)) != (1<<Z_TYPE_P(zv))) || (dst_info & (MAY_BE_STRING|MAY_BE_ARRAY)) != 0) {
| SET_ZVAL_TYPE_INFO dst_addr, Z_TYPE_INFO_P(zv)

Both of these conditions will be true unless Z_TYPE_P(zv) is NULL, in which case the type store is erroneously eliminated.

In this change I set res_use_info (dst_info) to 0 to force a type store. MAY_BE_ANY would also work.

This fixes #13508, but I'm not 100% confident this is correct.

ZEND_ASSIGN may have the same issue and I will look at it later.

@arnaud-lb arnaud-lb changed the base branch from master to PHP-8.3 March 6, 2024 20:18
@arnaud-lb arnaud-lb changed the title [WIP] Fix JIT QM_ASSIGN optimized out when op1 is null and res_use_info is unknown Fix JIT QM_ASSIGN may be optimized out when op1 is null Mar 8, 2024
@arnaud-lb arnaud-lb marked this pull request as ready for review March 8, 2024 14:05
@dstogov
Copy link
Member

dstogov commented Mar 11, 2024

This seems already fixed in master in a better way.
Really we incorrectly narrowed res_type_info a few lines above, probably because rules for ASSIGN -> QM_ASSIGN optimization, but really QM_ASSIGN might be used in a first place.

diff --git a/ext/opcache/jit/zend_jit_trace.c b/ext/opcache/jit/zend_jit_trace.c
index f6da28375b7..c1ed5d80a66 100644
--- a/ext/opcache/jit/zend_jit_trace.c
+++ b/ext/opcache/jit/zend_jit_trace.c
@@ -5070,8 +5070,7 @@ static const void *zend_jit_trace(zend_jit_trace_rec *trace_buffer, uint32_t par
 						CHECK_OP1_TRACE_TYPE();
 						res_info = RES_INFO();
 						res_use_info = zend_jit_trace_type_to_info(
-							STACK_MEM_TYPE(stack, EX_VAR_TO_NUM(opline->result.var)))
-								& (MAY_BE_UNDEF|MAY_BE_NULL|MAY_BE_FALSE|MAY_BE_TRUE|MAY_BE_LONG|MAY_BE_DOUBLE);
+							STACK_MEM_TYPE(stack, EX_VAR_TO_NUM(opline->result.var)));
 						res_addr = RES_REG_ADDR();
 						if (Z_MODE(res_addr) != IS_REG &&
 								STACK_TYPE(stack, EX_VAR_TO_NUM(opline->result.var)) !=

Can you place take care about committing this.

@arnaud-lb arnaud-lb changed the base branch from PHP-8.3 to PHP-8.2 March 11, 2024 11:54
@arnaud-lb arnaud-lb merged commit 0ea8012 into php:PHP-8.2 Mar 11, 2024
arnaud-lb added a commit that referenced this pull request Mar 11, 2024
* PHP-8.2:
  [ci skip] NEWS
  Fix GH-13508: JITed QM_ASSIGN may be optimized out when op1 is null (#13610)
arnaud-lb added a commit that referenced this pull request Mar 11, 2024
* PHP-8.3:
  [ci skip] NEWS
  [ci skip] NEWS
  Fix GH-13508: JITed QM_ASSIGN may be optimized out when op1 is null (#13610)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jit bug & segfault with symfony lazyghosttrait
2 participants