Skip to content

Conversation

alexandre-daubois
Copy link
Member

@alexandre-daubois alexandre-daubois commented Aug 18, 2025

@alexandre-daubois alexandre-daubois force-pushed the array-key-exists-null branch 3 times, most recently from 472c3c6 to baeda1f Compare August 20, 2025 08:30
@alexandre-daubois alexandre-daubois force-pushed the array-key-exists-null branch 2 times, most recently from d5fd9e5 to a602268 Compare August 20, 2025 15:02
@alexandre-daubois
Copy link
Member Author

Still a few tests to fix but we're getting closer

@alexandre-daubois alexandre-daubois force-pushed the array-key-exists-null branch 2 times, most recently from 6f50c89 to e508fcc Compare August 21, 2025 06:54
@alexandre-daubois alexandre-daubois force-pushed the array-key-exists-null branch 4 times, most recently from 8f22921 to dc5049b Compare August 21, 2025 08:54
@alexandre-daubois alexandre-daubois force-pushed the array-key-exists-null branch 2 times, most recently from c47e2b5 to 58878be Compare August 21, 2025 11:50
@alexandre-daubois alexandre-daubois force-pushed the array-key-exists-null branch 5 times, most recently from 7c574af to a1564b1 Compare August 22, 2025 14:40
@alexandre-daubois alexandre-daubois force-pushed the array-key-exists-null branch 2 times, most recently from 736aac0 to 1bf2dbd Compare August 22, 2025 16:06
@alexandre-daubois alexandre-daubois marked this pull request as draft August 22, 2025 16:16
@alexandre-daubois alexandre-daubois force-pushed the array-key-exists-null branch 3 times, most recently from ed3ac5b to 8324606 Compare August 22, 2025 16:58
@alexandre-daubois alexandre-daubois marked this pull request as ready for review August 23, 2025 08:28
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This is okay on my side, but I would like for @nielsdos to confirm that whatever the issue was is now resolved. :)

@Girgias
Copy link
Member

Girgias commented Sep 3, 2025

@nielsdos @alexandre-daubois is there still something blocking here?

@alexandre-daubois
Copy link
Member Author

Ah yes I missed your comment, PR updated. All good on my side

@Girgias Girgias requested a review from a team September 3, 2025 09:21
@Girgias
Copy link
Member

Girgias commented Sep 3, 2025

Circle CI failures look legit

Copy link
Member

@edorian edorian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RM approval 👍, no technical review performed

@nielsdos
Copy link
Member

nielsdos commented Sep 3, 2025

It can still be simplified:

diff --git a/ext/opcache/jit/zend_jit_helpers.c b/ext/opcache/jit/zend_jit_helpers.c
index 261f06068c8..ce48d102234 100644
--- a/ext/opcache/jit/zend_jit_helpers.c
+++ b/ext/opcache/jit/zend_jit_helpers.c
@@ -501,33 +501,17 @@ static void ZEND_FASTCALL zend_jit_fetch_dim_r_helper(zend_array *ht, zval *dim,
 			}
 			ZEND_FALLTHROUGH;
 		case IS_NULL:
-			/* The array may be destroyed while throwing the notice.
-			 * Temporarily increase the refcount to detect this situation. */
-			if (!(GC_FLAGS(ht) & IS_ARRAY_IMMUTABLE)) {
-				GC_ADDREF(ht);
+			retval = zend_hash_find(ht, ZSTR_EMPTY_ALLOC());
+			if (!retval) {
+				ZVAL_NULL(result);
+			} else {
+				ZVAL_COPY_DEREF(result, retval);
 			}
-			execute_data = EG(current_execute_data);
-			opline = EX(opline);
 			zend_error(E_DEPRECATED, "Using null as an array offset is deprecated, use an empty string instead");
-			if (!(GC_FLAGS(ht) & IS_ARRAY_IMMUTABLE) && !GC_DELREF(ht)) {
-				zend_array_destroy(ht);
-				if (opline->result_type & (IS_VAR | IS_TMP_VAR)) {
-					if (EG(exception)) {
-						ZVAL_UNDEF(EX_VAR(opline->result.var));
-					} else {
-						ZVAL_NULL(EX_VAR(opline->result.var));
-					}
-				}
-				return;
-			}
-			if (EG(exception)) {
-				if (opline->result_type & (IS_VAR | IS_TMP_VAR)) {
-					ZVAL_UNDEF(EX_VAR(opline->result.var));
-				}
-				return;
+			if (!retval) {
+				zend_error(E_WARNING, "Undefined array key \"\"");
 			}
-			offset_key = ZSTR_EMPTY_ALLOC();
-			goto str_index;
+			return;
 		case IS_DOUBLE:
 			hval = zend_dval_to_lval(Z_DVAL_P(dim));
 			if (!zend_is_long_compatible(Z_DVAL_P(dim), hval)) {

the write cases are more annoying but this read case works on my machine.

@alexandre-daubois
Copy link
Member Author

alexandre-daubois commented Sep 4, 2025

Oh right, your last comment in our thread got lost in my notifications, sorry. PR updated and green. Thanks!

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.

7 participants