Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Root current opline result after GC #19787

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
arnaud-lb wants to merge 1 commit into php:master
base: master
Choose a base branch
Loading
from arnaud-lb:gh13687
Draft

Conversation

Copy link
Member

@arnaud-lb arnaud-lb commented Sep 10, 2025
edited
Loading

Possible fix for GH-13687.

GH-13687 TL;DR:

  • Normally, the GC adds live TMPVARs to the roots for the reasons explained in

    php-src/Zend/zend_gc.c

    Lines 2213 to 2216 in 05eda43

    /* TMPVAR operands are destroyed using zval_ptr_dtor_nogc(), because they usually cannot contain
    * cycles. However, there are some rare exceptions where this is possible, in which case we rely
    * on the producing code to root the value. If a GC run occurs between the rooting and consumption
    * of the value, we would end up leaking it. To avoid this, root all live TMPVAR values here. */
  • However this misses variables produced by opN and consumed by opN+1, as these don't appear in live vars

This implements two main changes:

  • At the end of a GC run, root the last executed opline's result
  • Delay GC runs until a safe point is reached: The GC is now executed in interrupt handlers

The latter point is needed so that we can have some expectations about the VM state (e.g. the last opline's result is initialized), and it's a good thing regardless of this bug, IMHO (e.g. #17246).

There are a few things to consider:

  • Interrupts are executed with EX(opline) set to the next opline to execute, not the last one executed. So I root the inputs of EX(opline) instead: If EX(opline) consumes the result of the previously executed opline, it will be one of the inputs.
  • Except internal function call interrupts. Those are handled in the middle of the DO_[FI]CALL op while the internal frame is still on the stack, so I root the result of the parent frame, which is the result of the call.
  • When walking the stack frame, internal frames indicate that the parent frame is in the middle of the DO_[FI]CALL op as well. Arguments are not consumed yet and must be rooted.

Copy link
Member Author

arnaud-lb commented Sep 11, 2025
edited
Loading

In JIT:

  • Using exits to handle interrupts is useful to make sure that we have a consistent view of the stack in GC. It also keeps the code size small.
  • However, the exit may be eventually blacklisted as vm_interrupt will be set more often
  • We could introduce a new exit kind that doesn't blacklist
  • Alternatively we could exit manually, provided that we handle deoptimization
  • WDYT @dstogov?

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

I think delaying GC until interrupt handler has its advantages, but this is a significant change that may cause regressions.
I can't dedicate significant time to this task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimWolla TimWolla TimWolla left review comments

@dstogov dstogov dstogov left review comments

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /