Skip to content
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

Also fix up return values when frame is present, for backref. #5504

Merged
merged 1 commit into from Dec 10, 2018

Conversation

Projects
None yet
2 participants
@headius
Copy link
Member

headius commented Dec 10, 2018

This logic was used to ensure that any heap-scoped variable loads
happened before the "pop" logic for those scopes. However another
type of variable, frame-locals like $~ and $_, did not get the
same treatment. This patch ensures that any frame or binding
requirement triggers the return value copy fixup.

Fixes #5503.

Also fix up return values when frame is present, for backref.
This logic was used to ensure that any heap-scoped variable loads
happened before the "pop" logic for those scopes. However another
type of variable, frame-locals like $~ and $_, did not get the
same treatment. This patch ensures that any frame or binding
requirement triggers the return value copy fixup.

Fixes #5503.

@headius headius added this to the JRuby 9.2.6.0 milestone Dec 10, 2018

@headius headius merged commit cfe635f into jruby:master Dec 10, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@headius headius deleted the headius:capture_backref_before_pop branch Dec 10, 2018

@subbuss

This comment has been minimized.

Copy link
Contributor

subbuss commented Dec 10, 2018

This might be conservative in the common case. It is not frame require that requires the return to be patch up .. but the fact that the return uses frame vars that requires the patch up. So, you could refine this if you wish to.

@subbuss

This comment has been minimized.

Copy link
Contributor

subbuss commented Dec 10, 2018

You could refine it in the fixupReturn -- seems like the simplest place to centralize that logic and leave the requireFrame check you added as is.

@headius

This comment has been minimized.

Copy link
Member Author

headius commented Dec 10, 2018

@subbuss Yeah that's kinda what I was thinking. Only returns that access the frame or binding need to be affected it. But I also was thinking...it's just one copy. Lack of visibility into which operands have "special" needs frustrates this a little.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.