Skip to content

newFOROP: fix crash when optimizing 2-var for over builtin::indexed #23429

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: blead
Choose a base branch
from
Open

newFOROP: fix crash when optimizing 2-var for over builtin::indexed #23429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48ccf3e
newFOROP: fix crash when optimizing 2-var for over builtin::indexed
mauke Jul 11, 2025
f5932f8
perf/opcount.t: ensure imported builtins are optimized
mauke Jul 17, 2025
8eda65e
perldelta for the 2-var 'for' crash (#23405)
mauke Jul 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions op.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9671,7 +9671,7 @@ S_op_is_cv_xsub(pTHX_ OP *o, XSUBADDR_t xsub)
}

case OP_PADCV:
cv = (CV *)PAD_SVl(o->op_targ);
cv = find_lexical_cv(o->op_targ);
assert(cv && SvTYPE(cv) == SVt_PVCV);
break;

Expand All @@ -9689,10 +9689,17 @@ S_op_is_cv_xsub(pTHX_ OP *o, XSUBADDR_t xsub)
static bool
S_op_is_call_to_cv_xsub(pTHX_ OP *o, XSUBADDR_t xsub)
{
if(o->op_type != OP_ENTERSUB)
if (o->op_type != OP_ENTERSUB)
return false;

OP *cvop = cLISTOPx(cUNOPo->op_first)->op_last;
OP *aop = cUNOPo->op_first;
if (!OpHAS_SIBLING(aop)) {
aop = cUNOPx(aop)->op_first;
}
aop = OpSIBLING(aop);
OP *cvop;
for (cvop = aop; OpHAS_SIBLING(cvop); cvop = OpSIBLING(cvop)) ;

return op_is_cv_xsub(cvop, xsub);
}

Expand Down
24 changes: 23 additions & 1 deletion pod/perldelta.pod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,29 @@ manager will later use a regex to expand these into links.

=item *

XXX
Certain constructs involving a two-variable C<for> loop would crash the perl
compiler in v5.42.0:

# Two-variable for loop over a list returned from a method call:
for my ($x, $y) (Some::Class->foo()) { ... }
for my ($x, $y) ($object->foo()) { ... }

and

# Two-variable for loop over a list returned from a call to a
# lexical(ly imported) subroutine, all inside a lexically scoped
# or anonymous subroutine:
my sub foo { ... }
my $fn = sub {
for my ($x, $y) (foo()) { ... }
};

use builtin qw(indexed); # lexical import!
my sub bar {
for my ($x, $y) (indexed(...)) { ... }
}

These have been fixed. [GH #23405]

=item *

Expand Down
13 changes: 13 additions & 0 deletions t/op/for-many.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,4 +498,17 @@ is($continue, 'xx', 'continue reached twice');
is("@have", "Pointy end Up Flamey end Down", 'for my ($one, $two)');
}

# GH #23405 - segfaults when compiling 2-var for loops
{
my $dummy = sub {};
for my ($x, $y) (main->$dummy) {}
pass '2-var for does not crash on method calls';

my sub dummy {}
sub {
for my ($x, $y) (dummy) {}
}->();
pass '2-var for does not crash on lexical sub calls';
}

done_testing();
129 changes: 124 additions & 5 deletions t/perf/opcount.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,21 @@ test_opcount(0, "multiconcat: local assign",

# builtin:: function calls should be replaced with efficient op implementations
no warnings 'experimental::builtin';
use builtin qw(
blessed
ceil
false
floor
Comment on lines +701 to +705
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of the changes in this file seem like they belong in a separate commit - they have nothing to do with fixing the builtin::indexed optimization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The builtin::indexed optimization works fine on builtin::indexed(...), but may crash on use builtin qw(indexed); ... indexed(...) because the call optrees look different. I wanted to make sure both cases are optimized correctly (for all builtins).

This change is now a separate commit.

indexed
is_bool
is_tainted
is_weak
refaddr
reftype
true
unweaken
weaken
);

test_opcount(0, "builtin::true/false are replaced with constants",
sub { my $x = builtin::true(); my $y = builtin::false() },
Expand All @@ -706,6 +721,13 @@ test_opcount(0, "builtin::true/false are replaced with constants",
const => 2,
});

test_opcount(0, "imported true/false are replaced with constants",
sub { my $x = true(); my $y = false() },
{
entersub => 0,
const => 2,
});

test_opcount(0, "builtin::is_bool is replaced with direct opcode",
sub { my $x; my $y; $y = builtin::is_bool($x); 1; },
{
Expand All @@ -715,6 +737,15 @@ test_opcount(0, "builtin::is_bool is replaced with direct opcode",
padsv_store => 1,
});

test_opcount(0, "imported is_bool is replaced with direct opcode",
sub { my $x; my $y; $y = is_bool($x); 1; },
{
entersub => 0,
is_bool => 1,
padsv => 3,
padsv_store => 1,
});

test_opcount(0, "builtin::is_bool gets constant-folded",
sub { builtin::is_bool(123); },
{
Expand All @@ -723,48 +754,98 @@ test_opcount(0, "builtin::is_bool gets constant-folded",
const => 1,
});

test_opcount(0, "imported is_bool gets constant-folded",
sub { is_bool(123); },
{
entersub => 0,
is_bool => 0,
const => 1,
});

test_opcount(0, "builtin::weaken is replaced with direct opcode",
sub { my $x = []; builtin::weaken($x); },
{
entersub => 0,
weaken => 1,
});

test_opcount(0, "imported weaken is replaced with direct opcode",
sub { my $x = []; weaken($x); },
{
entersub => 0,
weaken => 1,
});

test_opcount(0, "builtin::unweaken is replaced with direct opcode",
sub { my $x = []; builtin::unweaken($x); },
{
entersub => 0,
unweaken => 1,
});

test_opcount(0, "imported unweaken is replaced with direct opcode",
sub { my $x = []; unweaken($x); },
{
entersub => 0,
unweaken => 1,
});

test_opcount(0, "builtin::is_weak is replaced with direct opcode",
sub { builtin::is_weak([]); },
{
entersub => 0,
is_weak => 1,
});

test_opcount(0, "imported is_weak is replaced with direct opcode",
sub { is_weak([]); },
{
entersub => 0,
is_weak => 1,
});

test_opcount(0, "builtin::blessed is replaced with direct opcode",
sub { builtin::blessed([]); },
{
entersub => 0,
blessed => 1,
});

test_opcount(0, "imported blessed is replaced with direct opcode",
sub { blessed([]); },
{
entersub => 0,
blessed => 1,
});

test_opcount(0, "builtin::refaddr is replaced with direct opcode",
sub { builtin::refaddr([]); },
{
entersub => 0,
refaddr => 1,
});

test_opcount(0, "imported refaddr is replaced with direct opcode",
sub { refaddr([]); },
{
entersub => 0,
refaddr => 1,
});

test_opcount(0, "builtin::reftype is replaced with direct opcode",
sub { builtin::reftype([]); },
{
entersub => 0,
reftype => 1,
});

test_opcount(0, "imported reftype is replaced with direct opcode",
sub { reftype([]); },
{
entersub => 0,
reftype => 1,
});

my $one_point_five = 1.5; # Prevent const-folding.
test_opcount(0, "builtin::ceil is replaced with direct opcode",
sub { builtin::ceil($one_point_five); },
Expand All @@ -773,20 +854,41 @@ test_opcount(0, "builtin::ceil is replaced with direct opcode",
ceil => 1,
});

test_opcount(0, "imported ceil is replaced with direct opcode",
sub { ceil($one_point_five); },
{
entersub => 0,
ceil => 1,
});

test_opcount(0, "builtin::floor is replaced with direct opcode",
sub { builtin::floor($one_point_five); },
{
entersub => 0,
floor => 1,
});

test_opcount(0, "imported floor is replaced with direct opcode",
sub { floor($one_point_five); },
{
entersub => 0,
floor => 1,
});

test_opcount(0, "builtin::is_tainted is replaced with direct opcode",
sub { builtin::is_tainted($0); },
{
entersub => 0,
is_tainted => 1,
});

test_opcount(0, "imported is_tainted is replaced with direct opcode",
sub { is_tainted($0); },
{
entersub => 0,
is_tainted => 1,
});

# void sassign + padsv combinations are replaced by padsv_store
test_opcount(0, "sassign + padsv replaced by padsv_store",
sub { my $y; my $z = $y = 3; 1; },
Expand Down Expand Up @@ -1014,18 +1116,35 @@ test_opcount(0, "Empty anonhash ref and direct lexical assignment",
test_opcount(0, "foreach 2 lexicals on builtin::indexed ARRAY",
sub { my @input = (); foreach my ($i, $x) (builtin::indexed @input) { } },
{
entersub => 0, # no call to builtin::indexed
entersub => 0, # no call to builtin::indexed
enteriter => 1,
iter => 1,
padav => 2,
iter => 1,
padav => 2,
});

test_opcount(0, "foreach 2 lexicals on imported indexed ARRAY",
sub { my @input = (); foreach my ($i, $x) (indexed @input) { } },
{
entersub => 0, # no call to builtin::indexed
enteriter => 1,
iter => 1,
padav => 2,
});

test_opcount(0, "foreach 2 lexicals on builtin::indexed LIST",
sub { foreach my ($i, $x) (builtin::indexed qw( x y z )) { } },
{
entersub => 0, # no call to builtin::indexed
entersub => 0, # no call to builtin::indexed
enteriter => 1,
iter => 1,
});

test_opcount(0, "foreach 2 lexicals on imported indexed LIST",
sub { foreach my ($i, $x) (indexed qw( x y z )) { } },
{
entersub => 0, # no call to builtin::indexed
enteriter => 1,
iter => 1,
iter => 1,
});

# substr with const zero offset and "" replacements
Expand Down
Loading