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

add CvEVAL_COMPILED() flag and fix closure bug. #22097

Merged
merged 1 commit into from
Jun 17, 2024
Merged

Conversation

iabyn
Copy link
Contributor

@iabyn iabyn commented Mar 22, 2024

[NB: I'm not expecting to merge this until after 5.40. Too much chance that something subtle changes in closure behaviour.]

EVAL CVs are treated a bit weirdly: their CvROOT() and CvSTART() fields don't get populated; instead the current values are stored in the PL_eval_root and PL_eval_start variables while they are being executed.

This caused a bug in closures and nested evals when an inner eval was repeated twice. The first inner eval accessed an outer lexical, which caused a fake cache entry to be added to the outer eval's pad. The second inner eval finds this cached entry, but incorrectly concludes that the outer eval is in fact an anon sub prototype and issues a 'variable is not available' warning. This is due to this simplistic definition in pad.c:

#define CvCOMPILED(cv) CvROOT(cv)

This commit adds a new flag, CvEVAL_COMPILED(), to indicate a fully-compiled EVAL CV. This allows us to work around the limitation.

In an ideal world this would have been fixed instead by making EVAL CVs first-class citizens with CvROOT() etc, but plenty of stuff seems to assume otherwise. So I took the path of least resistance.

See https://www.perlmonks.org/?node_id=11158351

@tonycoz tonycoz added the defer-next-dev This PR should not be merged yet, but await the next development cycle label Apr 2, 2024
@jkeenan jkeenan removed the defer-next-dev This PR should not be merged yet, but await the next development cycle label Jun 10, 2024
@jkeenan
Copy link
Contributor

jkeenan commented Jun 10, 2024

For the purpose of getting smoke-testing data, today I checked out this pull request, rebased it on (post-perl-5.40.0) blead and pushed it to origin as branch smoke-me/jkeenan/davem/eval-compiled-gh-22097-20240610. @tonycoz has approved the branch but, since @iabyn has mentioned the possibility of subtle changes in closure behavior, additional eyeballs would be helpful.

EVAL CVs are treated a bit weirdly: their CvROOT() and CvSTART() fields
don't get populated; instead the current values are stored in the
PL_eval_root and PL_eval_start variables while they are being executed.

This caused a bug in closures and nested evals when an inner eval was
repeated twice. The first inner eval accessed an outer lexical, which
caused a fake cache entry to be added to the outer eval's pad. The
second inner eval finds this cached entry, but incorrectly concludes
that the outer eval is in fact an anon sub prototype and issues a
'variable is not available' warning. This is due to this simplistic
definition in pad.c:

    #define CvCOMPILED(cv) CvROOT(cv)

This commit adds a new flag, CvEVAL_COMPILED(), to indicate a
fully-compiled EVAL CV. This allows us to work around the limitation.

In an ideal world this would have been fixed instead by making EVAL CVs
first-class citizens with CvROOT() etc, but plenty of stuff seems to
assume otherwise. So I took the path of least resistance.

See https://www.perlmonks.org/?node_id=11158351
@iabyn iabyn force-pushed the davem/eval_compiled branch from b74c4e0 to d06d710 Compare June 17, 2024 09:55
@iabyn iabyn merged commit d06d710 into blead Jun 17, 2024
59 checks passed
@iabyn iabyn deleted the davem/eval_compiled branch June 17, 2024 11:15
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