Skip to content

Fix segfault in format_default_value due to unexpected enum/object #11947

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

Closed
wants to merge 2 commits into from

Conversation

iluuu1994
Copy link
Member

Evaluating constants at comptime can result in arrays that contain objects. This is problematic for printing the default value of constant ASTs containing objects, because we don't actually know what the constructor arguments were. Avoid this by not propagating array constants.

I will need to check whether this leads to a performance regression.

Fixes GH-11937

Evaluating constants at comptime can result in arrays that contain objects. This
is problematic for printing the default value of constant ASTs containing
objects, because we don't actually know what the constructor arguments were.
Avoid this by not propagating array constants.

Fixes phpGH-11937
@Girgias
Copy link
Member

Girgias commented Aug 13, 2023

Does this mean that arrays are not const evaluated any more, even if they only have scalar values?

@iluuu1994
Copy link
Member Author

@Girgias It means when using a constant or class constant within a constant expression, the value from the constant won't be embedded in the outer constant anymore if it is an array. Rather, the constant is looked up and copied at runtime when evaluating the constant expression. This was previously only the case without opcache when including a file that uses a constant that has already been defined, or with preloading (I didn't test this case though).

@Girgias
Copy link
Member

Girgias commented Aug 14, 2023

Ah, that doesn't sound too bad then. :)

@bwoebi
Copy link
Member

bwoebi commented Aug 14, 2023

I guess it's probably okay. If you want to avoid some cases you could recursively check arrays for < IS_OBJECT instead, but may not be worth it.

@iluuu1994
Copy link
Member Author

iluuu1994 commented Aug 16, 2023

What I missed is that constants can also be propagated to opcode operands, which can sometimes be done even with opcache for self class constants. This caused a 0.05% slowdown in the Symfony Demo benchmark. I just pushed the change @bwoebi suggested. I can still see a 0.02% slowdown. I suppose that's low enough to now matter. Nonetheless, an alternative approach might be to just use some pseudo-representation for objects when dumping constant ASTs. That's an edge case, and we should optimize for the common case. I'll try that instead and close this PR if it works out.

@iluuu1994 iluuu1994 closed this in f78d1d0 Aug 17, 2023
@iluuu1994
Copy link
Member Author

The alternative didn't work easily, so I went for this solution.

There was a leak reported because the object survived zend_objects_store_free_object_storage because the array containing the object may be stored in various constants, like function default arguments, op_array literals, attribute arguments, etc. zend_objects_store_free_object_storage intentionally adds a refcount to objects to assure that they are not released later on (c45f195). However, given that functions and classes are only released after zend_objects_store_free_object_storage this will report all such values as leaked. One solution might've been to delay zend_objects_store_free_object_storage until after functions and classes have been destroyed, but that was deemed too risky by Bob.

This solution seems more in line with the current expectations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants