newFOROP: fix crash when optimizing 2-var for over builtin::indexed #23429
+203
−29
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mauke
force-pushed
the
fix-23405-segfault-for
branch
from
f5e4805 to
1731242
Compare
|
When I apply the changes in the two test files to blead, configure and build, then run the two test files, they FAIL with segfaults. When I then apply the code changes, the test files PASS. So from a TDD perspective, this p.r. is good. Someone else will have to look at the concept and the C-code. |
tonycoz
reviewed
Jul 16, 2025
tonycoz
reviewed
Jul 17, 2025
tonycoz
reviewed
Jul 17, 2025
mauke
added a commit
to mauke/perl5
that referenced
this pull request
Jul 17, 2025
When I saw (in Perl#23429) that use builtin qw(indexed); sub { for my ($x, $y) (indexed) {} } crashes, but sub { for my ($x, $y) (builtin::indexed) {} } works fine, I realized that optrees for calls to lexically imported subs look different from calls to package subs. This commit make sure both call variants are optimized to direct ops.
mauke
force-pushed
the
fix-23405-segfault-for
branch
from
1731242 to
8eda65e
Compare
|
@leonerd could you please review this pull request? Thanks. |
tonycoz
reviewed
Jul 21, 2025
OP_ENTERSUB isn't necessarily a LISTOP, apparently, so we can't just grab its op_last. Instead, copy/paste logic from elsewhere in op.c to find the cvop. Also, avoid crashing on "fake" pad entries that represent lexical subs from outer scopes by climbing up the scope chain until we reach a real pad entry. Fixes Perl#23405.
When I saw (in Perl#23429) that use builtin qw(indexed); sub { for my ($x, $y) (indexed) {} } crashes, but sub { for my ($x, $y) (builtin::indexed) {} } works fine, I realized that optrees for calls to lexically imported subs look different from calls to package subs. This commit make sure both call variants are optimized to direct ops.
mauke
force-pushed
the
fix-23405-segfault-for
branch
from
8eda65e to
8808c6a
Compare
... for locating the beginning of the argument list and the end (which is the entered sub itself) in the op tree.
mauke
force-pushed
the
fix-23405-segfault-for
branch
from
8808c6a to
d87975f
Compare
leonerd
approved these changes
Jul 21, 2025
| aop = cUNOPx(aop)->op_first; | ||
| } | ||
| /* move past pushmark */ | ||
| aop = OpSIBLING(aop); |
There was a problem hiding this comment.
I'd be tempted to add an assert for this, just to check
assert(OpTYPE(aop) == OP_PUSHMARK);There was a problem hiding this comment.
None of the other parts I cribbed this code from had an assert there and I didn't want to make the code "crashier" than before.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
OP_ENTERSUB isn't necessarily a LISTOP, apparently, so we can't just grab its op_last. Instead, copy/paste logic from elsewhere in op.c to find the cvop.
Also, avoid crashing on "fake" pad entries that represent lexical subs from outer scopes by climbing up the scope chain until we reach a real pad entry.
Fixes #23405.