fix(codegen): keep Zend operand read order around hoisted side effects - #52
Conversation
matyhtf
left a comment
matyhtf
left a comment
There was a problem hiding this comment.
The current operand classifier misses side effects wrapped in non-binary expressions, so the argument-order bug is still observable.
For example:
function pairValue(mixed $a, mixed $b): string
{
return $a . ',' . $b;
}
$i = 1;
var_dump(pairValue($i, (int) ($i = 5)));
$k = 1;
var_dump(pairValue($k, !($k = 0)));Zend PHP outputs:
string(3) "1,5"
string(3) "1,1"
This branch outputs:
string(3) "5,5"
string(3) "0,1"
shouldMaterializeOrderedOperand() only descends through Expr\BinaryOp. An assignment wrapped by Expr\Cast or BooleanNot is therefore classified as non-hoisting, even though lowering it appends captured statements. The earlier by-value argument is not snapshotted and the nested assignment overtakes its read.
Please make the side-effect/hoisting detection structurally recursive through expression wrappers such as casts, unary expressions, boolean-not, bitwise-not, and error suppression, or derive the decision from the captured statements produced by lowering. A generic AST walk must stop at nested Closure/ArrowFunction bodies because their bodies are not evaluated at creation time.
Add PHPT coverage for at least the cast and boolean-not examples above. The existing tests pass but do not cover wrapped side effects.
f685a17 to
2d80364
Compare
|
Fixed and rebased on current master. On your suggested alternative (deriving from captured statements): I investigated it and it can't work here — the classifier runs before lowering (earlier arguments must be snapshotted before the later operand is emitted), and lowering doesn't always hoist: Both your repros now match Zend ( |
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.
Zend sends call arguments strictly left to right and reads a concat
operand's CV when its opcode executes: an assignment nested in a later
argument runs after every earlier by-value argument has been sent, so
pairValue($i, (int) ($i = 5)) passes the old value ("1,5"), and the
same holds when the assignment is wrapped in !, unary +/-, ~, or @.
shouldMaterializeOrderedOperand() only descended through BinaryOp
nodes, so an assignment inside any other expression wrapper was not
classified as side-effecting: no earlier operand was snapshotted and
the assignment could even stay inline in the C++ argument list
(php::toInt(i = 5LL)), mutating the variable at an unsequenced point.
Deriving the decision from the captured statements lowering produces
would miss exactly these inline cases, so the classifier now recurses
structurally through every sub-expression: any wrapper of a
side-effecting node is itself side-effecting. Closure and arrow
function bodies do not run at creation time and stop the walk; throw,
yield, yield from, and backtick expressions join the side-effecting
leaves.
Zend's ADD opcode still reads its CV at op time, so $k + ($k = 5)
keeps its existing codegen (10 in both worlds, no snapshot).
A wrapper expression around a side effect (-strlen($s), a cast, error suppression) is now materialized for evaluation order, but the temp-type fallback typed it by the detected PHP type. The lowered C++ can still be dynamic — an unqualified namespaced call lowers to php::call(...), a Variant — so a php::Int temporary failed to compile in the self-build. Follow the call/binary-op policy: native scalar types only in native-types mode, otherwise a dynamic temporary.
fa79e8d to
d48d60d
Compare
2 participants
When a later call argument or concat operand hoisted a side effect, it executed before earlier operands were read:
$j = 1; two($j, $j = 5)printed"5,5"where Zend SENDs left-to-right and prints"1,5";$m . "," . ($m = 9)printed"9,9"instead of"1,9".Earlier plain-variable operands are now snapshotted at their exact Zend read positions when a later operand materializes statements. Concat read positions were probed against Zend empirically (the VM reads the first two chain items together after both items' side effects), and plain arithmetic (
$k + ($k = 5)) is deliberately unchanged — Zend itself reads the CV at the op, so the existing codegen already matches; a no-regression test pins that.Verified against Zend 8.4.13; codegen tests + phpt included.
Part of the split of #39.