Fix GH-22857: JIT wrong code for FETCH_OBJ_FUNC_ARG with property hooks #22897
+276
−0
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| --TEST-- | ||
| GH-22857: Function JIT emits wrong code for FETCH_OBJ_FUNC_ARG on a property hook (SIMPLE_GET fast path) | ||
| --INI-- | ||
| opcache.enable=1 | ||
| opcache.enable_cli=1 | ||
| opcache.jit_buffer_size=64M | ||
| opcache.jit=1205 | ||
| opcache.jit_hot_func=1 | ||
| --EXTENSIONS-- | ||
| opcache | ||
| --FILE-- | ||
| <?php | ||
| namespace Test; | ||
|
|
||
| interface HandlerInterface { public function noop(): void; } | ||
|
|
||
| final class DefaultHandler implements HandlerInterface { | ||
| private static ?self $i = null; | ||
| public static function getInstance(): self { return self::$i ??= new self(); } | ||
| public function noop(): void {} | ||
| } | ||
|
|
||
| /* Original issue: virtual property hook read via FETCH_OBJ_FUNC_ARG under | ||
| * function JIT. The getter frame is pushed by the SIMPLE_GET fast path in the | ||
| * shared FETCH_OBJ_R handler, but the JIT-compiled FUNC_ARG opcode had no | ||
| * hook-enter guard, so the argument slot was read before the getter ran. | ||
| * | ||
| * The assertion is deterministic: the getter sets a static flag as a side | ||
| * effect, so we check whether the getter actually ran when the property is | ||
| * passed through FETCH_OBJ_FUNC_ARG. This avoids the previous data-dependent | ||
| * failure mode (relying on file_get_contents() fataling on whatever garbage | ||
| * the unguarded JIT read happened to land on), which made the regression | ||
| * test flaky. */ | ||
| class Container { | ||
| private static bool $getterRan = false; | ||
|
|
||
| public protected(set) HandlerInterface $handler; | ||
|
|
||
| public string $path { | ||
| get => (self::$getterRan = true) | ||
| ? self::build($this->kind, $this->id) | ||
| : self::build($this->kind, $this->id); | ||
| } | ||
|
|
||
| protected mixed $prev = null; | ||
|
|
||
| public function __construct( | ||
| public protected(set) string $kind, | ||
| public protected(set) string $id, | ||
| ) { | ||
| $this->handler = DefaultHandler::getInstance(); | ||
| } | ||
|
|
||
| public static function build(string $k, string $i): string { | ||
| return "/nonexistent/gh22857_{$k}_{$i}.dat"; | ||
| } | ||
|
|
||
| public function step(): void { | ||
| /* Unqualified namespaced-fallback call (INIT_NS_FCALL_BY_NAME) keeps | ||
| * the FETCH_OBJ_FUNC_ARG opcode instead of letting the optimizer | ||
| * rewrite it to FETCH_OBJ_R. @ preserves the opcode shape that | ||
| * triggers the bug. */ | ||
| @file_get_contents($this->path); | ||
| if (!self::$getterRan) { | ||
| throw new \RuntimeException('getter did not run via FUNC_ARG'); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| $c = new Container('alpha', 'beta'); | ||
| $c->step(); | ||
| $c->step(); | ||
| $c->step(); | ||
|
|
||
| /* Sibling-slot variant: a preceding plain FETCH_OBJ_R primes the | ||
| * SIMPLE_GET bit on the property cache slot; compact_literals shares the | ||
| * slot between FETCH_OBJ_R and FETCH_OBJ_FUNC_ARG for the same property, | ||
| * so the following FETCH_OBJ_FUNC_ARG consumes that bit and hits the | ||
| * SIMPLE_GET fast path. Without the hook-enter guard it passes whatever | ||
| * sits in an adjacent property slot instead of running the getter. The | ||
| * flag is reset after the priming FETCH_OBJ_R so the assertion reflects | ||
| * only whether the getter ran during the FETCH_OBJ_FUNC_ARG read. */ | ||
| class Container2 { | ||
| private static bool $getterRan = false; | ||
|
|
||
| public protected(set) HandlerInterface $handler; | ||
|
|
||
| public string $path { | ||
| get => (self::$getterRan = true) | ||
| ? self::build($this->kind, $this->id) | ||
| : self::build($this->kind, $this->id); | ||
| } | ||
|
|
||
| protected mixed $prev = null; | ||
|
|
||
| public function __construct( | ||
| public protected(set) string $kind, | ||
| public protected(set) string $id, | ||
| ) { | ||
| $this->handler = DefaultHandler::getInstance(); | ||
| } | ||
|
|
||
| public static function build(string $k, string $i): string { | ||
| return "/nonexistent/gh22857b_{$k}_{$i}.dat"; | ||
| } | ||
|
|
||
| public function step(): void { | ||
| $this->prev = $this->path; // FETCH_OBJ_R primes SIMPLE_GET on shared slot | ||
| self::$getterRan = false; // reset; flag now reflects only the FUNC_ARG read below | ||
| @file_get_contents($this->path); // FETCH_OBJ_FUNC_ARG consumes the primed bit | ||
| if (!self::$getterRan) { | ||
| throw new \RuntimeException('sibling-slot variant: getter did not run via FUNC_ARG'); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| $c2 = new Container2('alpha', 'beta'); | ||
| $c2->step(); | ||
| $c2->step(); | ||
| $c2->step(); | ||
|
|
||
| echo "OK\n"; | ||
| ?> | ||
| --EXPECT-- | ||
| OK |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| --TEST-- | ||
| GH-22857 (reg-alloc): FETCH_OBJ_FUNC_ARG hook read must not lose register-held vars | ||
| --ENV-- | ||
| A=1 | ||
| --INI-- | ||
| opcache.enable=1 | ||
| opcache.enable_cli=1 | ||
| opcache.jit_buffer_size=64M | ||
| opcache.jit=1205 | ||
| opcache.jit_hot_func=1 | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| class C { | ||
| public $prop { | ||
| get { return 1; } | ||
| } | ||
| } | ||
|
|
||
| function f(int $a0, $obj) { | ||
| // $a lives only in a register: every use is in a supported opcode and in a | ||
| // non-entry basic block, so the register allocator never spills it to the | ||
| // VM stack. Exiting to the VM (the buggy hook-enter guard) would read a | ||
| // stale stack slot for $a. | ||
| $a = $a0; | ||
| $b = $a + 2; | ||
| $c = g($obj->prop, $a ? 1 : 2); | ||
| return $b + $c; | ||
| } | ||
|
|
||
| if (getenv('A')) { | ||
| function g($a, $b) { | ||
| var_dump($b); | ||
| return $a; | ||
| } | ||
| } | ||
|
|
||
| $c = new C(); | ||
| var_dump(f(1, $c)); | ||
| var_dump(f(1, $c)); | ||
|
|
||
| ?> | ||
| --EXPECT-- | ||
| int(1) | ||
| int(4) | ||
| int(1) | ||
| int(4) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be worth it to merge this
caseblock with the one below to deduplicate this part