Skip to content

Fix lineno for all constant expressions #8855

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 1 commit into from

Conversation

iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented Jun 23, 2022

Closes GH-8821

I'm not sure what the performance implications of the setjmp are for each iteration of zend_ast_evaluate, but if zend_ast_evaluate bails we could end up with a freed string in filename_override which would be bad.

@iluuu1994 iluuu1994 force-pushed the gh-8821 branch 2 times, most recently from 7eb0ac0 to a68e49f Compare June 23, 2022 18:27
@bwoebi
Copy link
Member

bwoebi commented Jun 23, 2022

I don't like the way this is fixed. In particular it overrides call site information.

Would it be possible to install a fake (execute_data) frame when evaluated instead? I.e. line 8 calls line 11? The fake function name for that frame could be something like "constant X" (or "constant someclass::X").

This also applies to #8124 (which initially looked good to me, but I overlooked the include).

@devnexen devnexen requested a review from dstogov June 23, 2022 19:29
@iluuu1994
Copy link
Member Author

@bwoebi You're right, that would be more desirable.

The fake function name for that frame could be something like "constant X" (or "constant someclass::X").

That would certainly be good, although we have to go through all the cases that use zend_ast_evaluate and configure the appropriate function name (or actually push the fake call frame right there to show all frames for nested constants). I'll give this a shot, or fall back to a simpler solution (like just adding a single frame with the name "Constant expression").

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 also don't like a special case covered by EG(filename_override)/EG(lineno_override), but I think creating a fake call frame is going to be too complex for this problem. So it's better to keep this simple solution.

@iluuu1994
Copy link
Member Author

I had another look. @bwoebi I added a frame to the trace so it doesn't get lost. Let me know if you think this is acceptable now. If now I'll at least fix the lineno on master that get lost in various places.

@iluuu1994
Copy link
Member Author

@bwoebi Have you ever had time to review this? I'm happy closing this if either of you object.

@bwoebi
Copy link
Member

bwoebi commented Jan 2, 2023

@iluuu1994 I don't object to it :-)

Copy link
Member

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

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

Yes, please merge it :-)

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

Successfully merging this pull request may close these issues.

Using enum as array key in class constant correctly results in error, but line number is inaccurate
3 participants