From 6cd9372495577d08cda3dc6a6980581850c3e4cf Mon Sep 17 00:00:00 2001 From: Alessio Giacobbe Date: Mon, 31 Aug 2026 15:59:47 +0200 Subject: [PATCH 1/2] fix(codegen): keep Zend operand read order around hoisted side effects Lowering a later call argument or concat operand that materializes captured statements (an assignment, a call result) appended them to the enclosing statement, executing the side effect before earlier operands were read: two($j, $j = 5) with $j = 1 produced "5,5" (Zend "1,5") and $m . "," . ($m = 9) produced "9,9" (Zend "1,9"). Call arguments: Zend SENDs strictly left to right, so when a later argument hoists statements, every earlier by-value plain-variable argument is snapshotted into a temporary at its own argument position. By-reference parameters, unpacked arguments, $this and $GLOBALS are left alone. Concat chains: Zend reads a CV operand when its CONCAT opcode executes, so in the left-associated chain the first two items are read together at the first op (after both items' side effects: $s . ($s = 'b') . $s is "bbb") and each later item after the side effects of everything up to itself. The flattened braced-list lowering now snapshots a plain-variable item exactly at that read position, deferring the first item's snapshot until the second item has been lowered. Plain arithmetic is intentionally unchanged: Zend's ADD reads the CV at op time, so $k + ($k = 5) is 10 in both worlds, and the existing codegen already matches. --- phpunit/code/eval-order-side-effects.php | 24 +++++ .../src/EvalOrderSideEffectsCodegenTest.php | 88 +++++++++++++++++++ src/Generator/CallArgumentGenerator.php | 29 +++++- src/Parser/BinaryOpTrait.php | 80 +++++++++++++++-- .../operator/eval-order-side-effects.phpt | 68 ++++++++++++++ 5 files changed, 279 insertions(+), 10 deletions(-) create mode 100644 phpunit/code/eval-order-side-effects.php create mode 100644 phpunit/src/EvalOrderSideEffectsCodegenTest.php create mode 100644 tests/compiler/operator/eval-order-side-effects.phpt diff --git a/phpunit/code/eval-order-side-effects.php b/phpunit/code/eval-order-side-effects.php new file mode 100644 index 00000000..ad2e9f9a --- /dev/null +++ b/phpunit/code/eval-order-side-effects.php @@ -0,0 +1,24 @@ +compileFixture(); + $body = $this->extractFunctionBody($code, 'php_callargorder()'); + + self::assertMatchesRegularExpression( + '/(tmp_var_\d+) = j;\s*\n\s*(tmp_var_\d+) = j = 5LL;/', + $body, + 'the old value of $j must be captured before $j = 5 executes', + ); + self::assertDoesNotMatchRegularExpression( + '/php_pair\(php::toIntArgExact\(j,/', + $body, + '$j must not be read directly after the hoisted assignment', + ); + } + + public function testConcatOperandReadIsSnapshottedBeforeLaterAssignment(): void + { + $code = $this->compileFixture(); + $body = $this->extractFunctionBody($code, 'php_concatorder()'); + + self::assertMatchesRegularExpression( + '/(tmp_var_\d+) = m;\s*\n\s*(tmp_var_\d+) = m = 9LL;/', + $body, + 'the old value of $m must be captured before $m = 9 executes', + ); + self::assertDoesNotMatchRegularExpression( + '/php::concat\(\{php::toString\(m\)/', + $body, + '$m must not be read directly after the hoisted assignment', + ); + } + + public function testPlainArithmeticKeepsZendCvReadSemantics(): void + { + $code = $this->compileFixture(); + $body = $this->extractFunctionBody($code, 'php_plainarithmeticunchanged()'); + + // Zend reads the CV when the ADD executes, i.e. after the nested + // assignment; the direct read of k matches that and must stay. + self::assertMatchesRegularExpression( + '/(tmp_var_\d+) = k = 5LL;\s*\n[^\n]*\(\(k\) \+ \(\1\)\)/', + $body, + ); + self::assertStringNotContainsString('= k;', $body); + } + + private function extractFunctionBody(string $code, string $marker): string + { + $start = strpos($code, $marker); + self::assertIsInt($start, "missing function: {$marker}"); + $end = strpos($code, "\n}", $start); + self::assertIsInt($end); + return substr($code, $start, $end - $start); + } + + private function compileFixture(): string + { + global $translator; + + $compiler = CompilerTest::create(TYPEPHP_ROOT_PATH); + $translator = $compiler; + $source = TYPEPHP_ROOT_PATH . '/phpunit/code/eval-order-side-effects.php'; + $compiler->addFiles([$source]); + $compiler->prepareFile($source); + $generated = $compiler->convertFile($source); + $code = file_get_contents($generated); + + self::assertIsString($code); + return $code; + } +} diff --git a/src/Generator/CallArgumentGenerator.php b/src/Generator/CallArgumentGenerator.php index 03c70f44..0f1c836f 100644 --- a/src/Generator/CallArgumentGenerator.php +++ b/src/Generator/CallArgumentGenerator.php @@ -130,10 +130,37 @@ protected function parseNativeCallArgs( $variadicVar = null; $callableName = $functionDef->displayName ?: $functionDef->getNamespacedName(); + // PHP evaluates arguments left to right. A later argument that hoists + // captured statements while being lowered (an assignment, a call) + // would execute those side effects before an earlier plain-variable + // argument is read: `two($j, $j = 5)` must pass the old value of $j. + // Record the last such argument so every earlier by-value variable + // read can be snapshotted at its own argument position. + $lastHoistingSourceIndex = -1; + foreach ($sourceArgs as $sourceIndex => [, , $arg]) { + if ($arg instanceof Node\Arg && $this->shouldMaterializeOrderedOperand($arg->value)) { + $lastHoistingSourceIndex = $sourceIndex; + } + } + // Evaluate every supplied argument in PHP source order. The resulting // expressions/temporaries may then be rearranged safely for the native // C++ ABI without changing observable call order. - foreach ($sourceArgs as [$argIndex, $variadicName, $arg]) { + foreach ($sourceArgs as $sourceIndex => [$argIndex, $variadicName, $arg]) { + if ($sourceIndex < $lastHoistingSourceIndex + && $arg instanceof Node\Arg + && !$arg->unpack + && $this->isSnapshotableVariableRead($arg->value) + ) { + $paramInfo = $argIndex === $variadicArgIndex + ? $functionDef->argInfoList[$variadicArgIndex] + : $this->getArgInfo($arg, $nativeFunc, $argIndex); + if ($paramInfo !== null && !$paramInfo->byRef) { + $snapshot = $this->parseOrderedOperand($arg->value, false, true); + $arg = clone $arg; + $arg->value = new Expr\Variable($snapshot, $arg->value->getAttributes()); + } + } if ($argIndex !== $variadicArgIndex) { $argInfo = $this->getArgInfo($arg, $nativeFunc, $argIndex); $resolvedArgs[$argIndex] = $this->getTypeConvertedArg( diff --git a/src/Parser/BinaryOpTrait.php b/src/Parser/BinaryOpTrait.php index 8ba4e9aa..2005bef0 100644 --- a/src/Parser/BinaryOpTrait.php +++ b/src/Parser/BinaryOpTrait.php @@ -759,8 +759,38 @@ protected function parseFlattenedConcat(NodeAbstract $expr, array $prefixExpress $useTwoOperandOverload = $prefixExpressions === [] && $this->canUseTwoOperandConcatOverload($items); + // Zend lowers the left-associated chain i0.i1.i2... into one CONCAT + // opcode per node and reads a CV operand when its opcode executes: + // i0 and i1 are both read at the first op (after the side effects of + // both), and every later item ik at the k-th op (after the side + // effects of i0..ik, before those of later items). The flattened + // braced list hoists all captured side effects ahead of the whole + // expression, so a plain-variable item that Zend reads before a later + // item's side effects (`$m . ',' . ($m = 9)` must yield "1,9") is + // snapshotted into a temporary at its Zend read position. + $lastHoistingIndex = -1; + foreach ($items as $index => $item) { + if ($this->shouldMaterializeOrderedOperand($item) + || $this->isNativeObjectClass($this->detectClassOfExpr($item)) + ) { + $lastHoistingIndex = $index; + } + } + + // The first item is read together with the second at the first op, + // i.e. after the second item's side effects. Its snapshot is deferred + // until the second item has been lowered. + $deferFirstItemSnapshot = $lastHoistingIndex >= 2 + && isset($items[1]) + && $this->isSnapshotableVariableRead($items[0]) + && !($this->isScalarString($items[1]) && $items[1]->value === ''); + $argList = $prefixExpressions; - foreach ($items as $item) { + foreach ($items as $index => $item) { + if ($deferFirstItemSnapshot && $index === 0) { + continue; + } + // Keep one operand so concat still performs PHP string coercion. // Prefix expressions are operands too (for example, the left-hand // value of `.=`), so an empty RHS literal can be omitted there. @@ -768,20 +798,34 @@ protected function parseFlattenedConcat(NodeAbstract $expr, array $prefixExpress continue; } + $entryPosition = count($argList); $itemClass = $this->detectClassOfExpr($item); if ($this->isNativeObjectClass($itemClass)) { $toString = new Expr\MethodCall($item, new Node\Identifier('toString')); $argList[] = $this->parseOrderedOperand($toString, false); - continue; + } else { + $type = $this->detectTypeOfExpr($item); + // C++17 evaluates the braced-list elements in order. The + // temporary is still required because lowering a later operand + // may append captured beforeStmtLines ahead of the entire + // concat expression; without it, those statements could + // overtake an earlier Call. + $snapshotEarlierRead = $index >= 1 + && $index < $lastHoistingIndex + && $this->isSnapshotableVariableRead($item); + $parsed = $this->parseOrderedOperand($item, false, $snapshotEarlierRead); + $argList[] = $this->prepareConcatOperand($parsed, $type); } - $type = $this->detectTypeOfExpr($item); - // C++17 evaluates the braced-list elements in order. The temporary - // is still required because lowering a later operand may append - // captured beforeStmtLines ahead of the entire concat expression; - // without it, those statements could overtake an earlier Call. - $parsed = $this->parseOrderedOperand($item, false); - $argList[] = $this->prepareConcatOperand($parsed, $type); + if ($deferFirstItemSnapshot && $index === 1) { + // Snapshot the first item now, after the second item's side + // effects, and keep its leading position in the operand list. + $firstType = $this->detectTypeOfExpr($items[0]); + $firstParsed = $this->parseOrderedOperand($items[0], false, true); + array_splice($argList, $entryPosition, 0, [ + $this->prepareConcatOperand($firstParsed, $firstType), + ]); + } } if ($useTwoOperandOverload && count($argList) === 2) { @@ -791,6 +835,24 @@ protected function parseFlattenedConcat(NodeAbstract $expr, array $prefixExpress return Symbol::concat() . '({' . implode(', ', $argList) . '})'; } + /** + * Whether an operand is a plain local variable read whose value can be + * snapshotted into a temporary to preserve left-to-right evaluation when + * a later operand hoists side-effecting statements. `$this` cannot be + * reassigned and $GLOBALS has dedicated lowering; both are left alone. + */ + protected function isSnapshotableVariableRead(NodeAbstract $expr): bool + { + if (!$this->isVarExpr($expr) || !is_string($expr->name)) { + return false; + } + if ($expr->name === 'this' || $expr->name === 'GLOBALS') { + return false; + } + $var = (string) $this->parseIdentifier($expr); + return $this->hasVar($var) && !$this->isStdContainer($var); + } + protected function canUseTwoOperandConcatOverload(array $items): bool { if (count($items) !== 2) { diff --git a/tests/compiler/operator/eval-order-side-effects.phpt b/tests/compiler/operator/eval-order-side-effects.phpt new file mode 100644 index 00000000..1361d40f --- /dev/null +++ b/tests/compiler/operator/eval-order-side-effects.phpt @@ -0,0 +1,68 @@ +--TEST-- +Call arguments and concat operands follow Zend's operand read order around side effects +--FILE-- + +--EXPECT-- +string(3) "1,5" +int(5) +string(3) "2,6" +int(6) +string(3) "1,9" +int(9) +string(3) "bbb" +string(4) "xxxy" +string(3) "pbb" +string(4) "pa,b" +string(5) "preaz" +int(10) From f685a176c0f8dd27cc8946738e054a5945999a07 Mon Sep 17 00:00:00 2001 From: Alessio Giacobbe Date: Tue, 1 Sep 2026 11:55:53 +0200 Subject: [PATCH 2/2] test(codegen): platform-neutral integer-literal suffixes in eval-order assertions --- phpunit/src/EvalOrderSideEffectsCodegenTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/phpunit/src/EvalOrderSideEffectsCodegenTest.php b/phpunit/src/EvalOrderSideEffectsCodegenTest.php index 8b95dd9c..206c4002 100644 --- a/phpunit/src/EvalOrderSideEffectsCodegenTest.php +++ b/phpunit/src/EvalOrderSideEffectsCodegenTest.php @@ -19,7 +19,7 @@ public function testCallArgumentReadIsSnapshottedBeforeLaterAssignment(): void $body = $this->extractFunctionBody($code, 'php_callargorder()'); self::assertMatchesRegularExpression( - '/(tmp_var_\d+) = j;\s*\n\s*(tmp_var_\d+) = j = 5LL;/', + '/(tmp_var_\d+) = j;\s*\n\s*(tmp_var_\d+) = j = 5L{1,2};/', $body, 'the old value of $j must be captured before $j = 5 executes', ); @@ -36,7 +36,7 @@ public function testConcatOperandReadIsSnapshottedBeforeLaterAssignment(): void $body = $this->extractFunctionBody($code, 'php_concatorder()'); self::assertMatchesRegularExpression( - '/(tmp_var_\d+) = m;\s*\n\s*(tmp_var_\d+) = m = 9LL;/', + '/(tmp_var_\d+) = m;\s*\n\s*(tmp_var_\d+) = m = 9L{1,2};/', $body, 'the old value of $m must be captured before $m = 9 executes', ); @@ -55,7 +55,7 @@ public function testPlainArithmeticKeepsZendCvReadSemantics(): void // Zend reads the CV when the ADD executes, i.e. after the nested // assignment; the direct read of k matches that and must stay. self::assertMatchesRegularExpression( - '/(tmp_var_\d+) = k = 5LL;\s*\n[^\n]*\(\(k\) \+ \(\1\)\)/', + '/(tmp_var_\d+) = k = 5L{1,2};\s*\n[^\n]*\(\(k\) \+ \(\1\)\)/', $body, ); self::assertStringNotContainsString('= k;', $body);