Fix phi use chain management when renaming variable

If there is a previous use of the new variable in the phi, we need to NULL out the use chain of the new source we're adding. Test case is reduced from an assertion failure in the Symfony Demo.
php · Nov 9, 2020 · d971b67 · d971b67
1 parent 96b7251
commit d971b67
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 8 deletions.
diff --git a/ext/opcache/Optimizer/ssa_integrity.c b/ext/opcache/Optimizer/ssa_integrity.c
@@ -287,8 +287,9 @@ int ssa_verify_integrity(zend_op_array *op_array, zend_ssa *ssa, const char *ext
 
 	/* Phis */
 	FOREACH_PHI(phi) {
-		int source;
-		FOREACH_PHI_SOURCE(phi, source) {
+		unsigned num_sources = NUM_PHI_SOURCES(phi);
+		for (i = 0; i < num_sources; i++) {
+			int source = phi->sources[i];
 			if (source < 0) {
 				FAIL(VARFMT " negative source\n", VAR(phi->ssa_var));
 			}
@@ -298,7 +299,16 @@ int ssa_verify_integrity(zend_op_array *op_array, zend_ssa *ssa, const char *ext
 			if (ssa->vars[source].var != ssa->vars[phi->ssa_var].var) {
 				FAIL(VARFMT " source of phi for " VARFMT "\n", VAR(source), VAR(phi->ssa_var));
 			}
-		} FOREACH_PHI_SOURCE_END();
+			if (phi->use_chains[i]) {
+				int j;
+				for (j = i + 1; j < num_sources; j++) {
+					if (phi->sources[j] == source && phi->use_chains[j]) {
+						FAIL("use chain for source " VARFMT " of phi " VARFMT
+							" at %d despite earlier use\n", VAR(source), VAR(phi->ssa_var), j);
+					}
+				}
+			}
+		}
 		if (ssa->vars[phi->ssa_var].definition_phi != phi) {
 			FAIL(VARFMT " does not define this phi\n", VAR(phi->ssa_var));
 		}

diff --git a/ext/opcache/Optimizer/zend_ssa.c b/ext/opcache/Optimizer/zend_ssa.c
@@ -1298,11 +1298,7 @@ static inline void zend_ssa_remove_phi_source(zend_ssa *ssa, zend_ssa_phi *phi,
 	for (j = 0; j < predecessors_count; j++) {
 		if (phi->sources[j] == var_num) {
 			if (j < pred_offset) {
-				if (next_phi == NULL) {
-					next_phi = phi->use_chains[pred_offset];
-				} else {
-					ZEND_ASSERT(phi->use_chains[pred_offset] == NULL);
-				}
+				ZEND_ASSERT(next_phi == NULL);
 			} else if (j >= pred_offset) {
 				phi->use_chains[j] = next_phi;
 			}
@@ -1591,6 +1587,8 @@ void zend_ssa_rename_var_uses(zend_ssa *ssa, int old, int new, zend_bool update_
 						new_var->phi_use_chain = phi;
 					}
 					after_first_new_source = 1;
+				} else {
+					phi->use_chains[j] = NULL;
 				}
 			}
 		}

diff --git a/ext/opcache/tests/phi_use_chain.phpt b/ext/opcache/tests/phi_use_chain.phpt
@@ -0,0 +1,19 @@
+--TEST--
+Check that phi use chains are correctly maintained when removing blocks
+--FILE--
+<?php
+
+function test(array $adapters) {
+    foreach ($adapters as $adapter) {
+        if (\in_array('cli-server', ['cli', 'phpdbg'], true) && $adapter instanceof stdClass && !\filter_var('1', \FILTER_VALIDATE_BOOLEAN)) {
+            continue;
+        }
+
+        $adapters[] = $adapter;
+    }
+}
+
+?>
+===DONE===
+--EXPECT--
+===DONE===