Skip to content

Universal global regs 3 #113

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 16 commits into from
Closed

Universal global regs 3 #113

wants to merge 16 commits into from

Conversation

iluuu1994
Copy link
Owner

No description provided.

@dstogov
Copy link

dstogov commented Dec 4, 2023

I didn't completely understand what problem do you like to solve.

In case you like to eliminate VM code that moves CPU register used for opline to/from EX(opline), it's better to move some functions into zend_execute.c and make them responsible to that movements (e.g. zend_throw_...()) .

Completely disabling some CPU register for the whole binary looks too aggressive and useless.

@iluuu1994
Copy link
Owner Author

@dstogov Sorry, I should have included some more context. To clarify a bit, the idea comes from this comment: php#12758 (comment)

There is a known issue where EX(opline) may be outdated on OOM errors where the current opline->lineno is emitted. Worse, if the first opline is the one that OOMs, then EX(opline) may be completely uninitialized and cause a segfault. There were some recent attempts to fix some or all of them.

They involve adding SAVE_OPLINE calls to basically all handlers. With most handlers doing SAVE_OPLINE, global registers become less useful. I understand they still avoid passing opline to non-inlined helper functions. The motivation of this PR was to dodge SAVE_OPLINE by making the IP register reserved universally. Also note that GCC does use the fixed register as long as it can deduce that it is not accessed or written to during the execution of the compiled function. This does seem profitable according to this PoC, just not by as much as I had hoped.

image

That said, I do think the gain is too small to fragment extensions in another way (like nts/zts).

@iluuu1994 iluuu1994 closed this Feb 20, 2024
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.

2 participants