Skip to content

Commit f5e4805

Browse files
maukemauke
committed
newFOROP: fix crash when optimizing 2-var for over builtin::indexed
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 blank pad entries. (This is not a full fix: It avoids the crash, but doesn't recognize calls to imported indexed() in anonymous/lexical subs. Hence the TODO tests in t/perf/opcount.t.) Fixes #23405.
1 parent 9ef5300 commit f5e4805

File tree

3 files changed

+146
-12
lines changed

3 files changed

+146
-12
lines changed

‎op.c

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9666,7 +9666,12 @@ S_op_is_cv_xsub(pTHX_ OP *o, XSUBADDR_t xsub)
96669666

96679667
case OP_PADCV:
96689668
cv = (CV *)PAD_SVl(o->op_targ);
9669-
assert(cv && SvTYPE(cv) == SVt_PVCV);
9669+
assert(cv);
9670+
if (SvTYPE(cv) != SVt_PVCV) {
9671+
/* XXX If we're compiling an anonymous or 'my' sub, these pad
9672+
* entries aren't set up yet, so just bail out for now. */
9673+
return false;
9674+
}
96709675
break;
96719676

96729677
default:
@@ -9683,10 +9688,17 @@ S_op_is_cv_xsub(pTHX_ OP *o, XSUBADDR_t xsub)
96839688
static bool
96849689
S_op_is_call_to_cv_xsub(pTHX_ OP *o, XSUBADDR_t xsub)
96859690
{
9686-
if(o->op_type != OP_ENTERSUB)
9691+
if (o->op_type != OP_ENTERSUB)
96879692
return false;
96889693

9689-
OP *cvop = cLISTOPx(cUNOPo->op_first)->op_last;
9694+
OP *aop = cUNOPo->op_first;
9695+
if (!OpHAS_SIBLING(aop)) {
9696+
aop = cUNOPx(aop)->op_first;
9697+
}
9698+
aop = OpSIBLING(aop);
9699+
OP *cvop;
9700+
for (cvop = aop; OpHAS_SIBLING(cvop); cvop = OpSIBLING(cvop)) ;
9701+
96909702
return op_is_cv_xsub(cvop, xsub);
96919703
}
96929704

‎t/op/for-many.t

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -498,4 +498,17 @@ is($continue, 'xx', 'continue reached twice');
498498
is("@have", "Pointy end Up Flamey end Down", 'for my ($one, $two)');
499499
}
500500

501+
# GH #23405 - segfaults when compiling 2-var for loops
502+
{
503+
my $dummy = sub {};
504+
for my ($x, $y) (main->$dummy) {}
505+
pass '2-var for does not crash on method calls';
506+
507+
my sub dummy {}
508+
sub {
509+
for my ($x, $y) (dummy) {}
510+
}->();
511+
pass '2-var for does not crash on lexical sub calls';
512+
}
513+
501514
done_testing();

‎t/perf/opcount.t

Lines changed: 118 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -698,6 +698,21 @@ test_opcount(0, "multiconcat: local assign",
698698

699699
# builtin:: function calls should be replaced with efficient op implementations
700700
no warnings 'experimental::builtin';
701+
use builtin qw(
702+
blessed
703+
ceil
704+
false
705+
floor
706+
indexed
707+
is_bool
708+
is_tainted
709+
is_weak
710+
refaddr
711+
reftype
712+
true
713+
unweaken
714+
weaken
715+
);
701716

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

724+
test_opcount(0, "imported true/false are replaced with constants",
725+
sub { my $x = true(); my $y = false() },
726+
{
727+
entersub => 0,
728+
const => 2,
729+
});
730+
709731
test_opcount(0, "builtin::is_bool is replaced with direct opcode",
710732
sub { my $x; my $y; $y = builtin::is_bool($x); 1; },
711733
{
@@ -715,6 +737,15 @@ test_opcount(0, "builtin::is_bool is replaced with direct opcode",
715737
padsv_store => 1,
716738
});
717739

740+
test_opcount(0, "imported is_bool is replaced with direct opcode",
741+
sub { my $x; my $y; $y = is_bool($x); 1; },
742+
{
743+
entersub => 0,
744+
is_bool => 1,
745+
padsv => 3,
746+
padsv_store => 1,
747+
});
748+
718749
test_opcount(0, "builtin::is_bool gets constant-folded",
719750
sub { builtin::is_bool(123); },
720751
{
@@ -723,48 +754,98 @@ test_opcount(0, "builtin::is_bool gets constant-folded",
723754
const => 1,
724755
});
725756

757+
test_opcount(0, "imported is_bool gets constant-folded",
758+
sub { is_bool(123); },
759+
{
760+
entersub => 0,
761+
is_bool => 0,
762+
const => 1,
763+
});
764+
726765
test_opcount(0, "builtin::weaken is replaced with direct opcode",
727766
sub { my $x = []; builtin::weaken($x); },
728767
{
729768
entersub => 0,
730769
weaken => 1,
731770
});
732771

772+
test_opcount(0, "imported weaken is replaced with direct opcode",
773+
sub { my $x = []; weaken($x); },
774+
{
775+
entersub => 0,
776+
weaken => 1,
777+
});
778+
733779
test_opcount(0, "builtin::unweaken is replaced with direct opcode",
734780
sub { my $x = []; builtin::unweaken($x); },
735781
{
736782
entersub => 0,
737783
unweaken => 1,
738784
});
739785

786+
test_opcount(0, "imported unweaken is replaced with direct opcode",
787+
sub { my $x = []; unweaken($x); },
788+
{
789+
entersub => 0,
790+
unweaken => 1,
791+
});
792+
740793
test_opcount(0, "builtin::is_weak is replaced with direct opcode",
741794
sub { builtin::is_weak([]); },
742795
{
743796
entersub => 0,
744797
is_weak => 1,
745798
});
746799

800+
test_opcount(0, "imported is_weak is replaced with direct opcode",
801+
sub { is_weak([]); },
802+
{
803+
entersub => 0,
804+
is_weak => 1,
805+
});
806+
747807
test_opcount(0, "builtin::blessed is replaced with direct opcode",
748808
sub { builtin::blessed([]); },
749809
{
750810
entersub => 0,
751811
blessed => 1,
752812
});
753813

814+
test_opcount(0, "imported blessed is replaced with direct opcode",
815+
sub { blessed([]); },
816+
{
817+
entersub => 0,
818+
blessed => 1,
819+
});
820+
754821
test_opcount(0, "builtin::refaddr is replaced with direct opcode",
755822
sub { builtin::refaddr([]); },
756823
{
757824
entersub => 0,
758825
refaddr => 1,
759826
});
760827

828+
test_opcount(0, "imported refaddr is replaced with direct opcode",
829+
sub { refaddr([]); },
830+
{
831+
entersub => 0,
832+
refaddr => 1,
833+
});
834+
761835
test_opcount(0, "builtin::reftype is replaced with direct opcode",
762836
sub { builtin::reftype([]); },
763837
{
764838
entersub => 0,
765839
reftype => 1,
766840
});
767841

842+
test_opcount(0, "imported reftype is replaced with direct opcode",
843+
sub { reftype([]); },
844+
{
845+
entersub => 0,
846+
reftype => 1,
847+
});
848+
768849
my $one_point_five = 1.5; # Prevent const-folding.
769850
test_opcount(0, "builtin::ceil is replaced with direct opcode",
770851
sub { builtin::ceil($one_point_five); },
@@ -773,15 +854,22 @@ test_opcount(0, "builtin::ceil is replaced with direct opcode",
773854
ceil => 1,
774855
});
775856

776-
test_opcount(0, "builtin::floor is replaced with direct opcode",
777-
sub { builtin::floor($one_point_five); },
857+
test_opcount(0, "imported ceil is replaced with direct opcode",
858+
sub { ceil($one_point_five); },
859+
{
860+
entersub => 0,
861+
ceil => 1,
862+
});
863+
864+
test_opcount(0, "imported floor is replaced with direct opcode",
865+
sub { floor($one_point_five); },
778866
{
779867
entersub => 0,
780868
floor => 1,
781869
});
782870

783-
test_opcount(0, "builtin::is_tainted is replaced with direct opcode",
784-
sub { builtin::is_tainted($0); },
871+
test_opcount(0, "imported is_tainted is replaced with direct opcode",
872+
sub { is_tainted($0); },
785873
{
786874
entersub => 0,
787875
is_tainted => 1,
@@ -1014,20 +1102,41 @@ test_opcount(0, "Empty anonhash ref and direct lexical assignment",
10141102
test_opcount(0, "foreach 2 lexicals on builtin::indexed ARRAY",
10151103
sub { my @input = (); foreach my ($i, $x) (builtin::indexed @input) { } },
10161104
{
1017-
entersub => 0, # no call to builtin::indexed
1105+
entersub => 0, # no call to builtin::indexed
10181106
enteriter => 1,
1019-
iter => 1,
1020-
padav => 2,
1107+
iter => 1,
1108+
padav => 2,
10211109
});
10221110

10231111
test_opcount(0, "foreach 2 lexicals on builtin::indexed LIST",
10241112
sub { foreach my ($i, $x) (builtin::indexed qw( x y z )) { } },
10251113
{
1026-
entersub => 0, # no call to builtin::indexed
1114+
entersub => 0, # no call to builtin::indexed
10271115
enteriter => 1,
1028-
iter => 1,
1116+
iter => 1,
10291117
});
10301118

1119+
{
1120+
local $::TODO = 'unqualified (imported) indexed not optimized yet';
1121+
1122+
test_opcount(0, "foreach 2 lexicals on imported indexed ARRAY",
1123+
sub { my @input = (); foreach my ($i, $x) (indexed @input) { } },
1124+
{
1125+
entersub => 0, # no call to builtin::indexed
1126+
enteriter => 1,
1127+
iter => 1,
1128+
padav => 2,
1129+
});
1130+
1131+
test_opcount(0, "foreach 2 lexicals on imported indexed LIST",
1132+
sub { foreach my ($i, $x) (indexed qw( x y z )) { } },
1133+
{
1134+
entersub => 0, # no call to builtin::indexed
1135+
enteriter => 1,
1136+
iter => 1,
1137+
});
1138+
}
1139+
10311140
# substr with const zero offset and "" replacements
10321141
test_opcount(0, "substr with const zero offset and '' repl (void)",
10331142
sub { my $z; substr($z, 0, 2, "") },

0 commit comments

Comments
 (0)