Skip to content

Commit adbdebe

Browse files
committed
Avoid z/OS function pointer comparison undefined behavior
Prior to this commit, code in peep.c that checks to see if a PL_check[] function had been overridden would always return true on z/OS. This would turn off optimization for multideref, causing tests to fail that expected it to be on. One solution would be to just not have multideref on that box, and to skip the failing tests. I'd rather not turn off optimizations unless absolutely necessary. The check for being overridden was to simply compare two function pointers for equality. From information relayed to me on an IBM discord z/OS chat channel, function pointer equality comparisons will never compare equal across different translation units. To get a valid comparison, you must use pointers from the same translation unit. I had first looked at the documentation. It required more background knowledge of the jargon used, and the z/OS design than I was willing to spend the time learning. But there may be a way around this, but if so it's complicated. That's when I turned to the chat channel. The PL_check[] table of the function pointers is declared extern, and when peep.c is compiled, it doesn't have access to that table; it gets linked to later. So its different translation units, and so the pointers never will be equal. However within the table itself, all the pointers to function X will have the same address, as it is going to be in the same translation unit. What this commit does is to add three extra elements to PL_check[], initialized with the function pointers that peep.c wants to compare against. Those pointers are unknown to any other code, so will never be changed away from pointing to these function pointers. And the commit changes peep.c to use the appropriate array entry, which will contain a valid value, instead of using the function pointer directly. This solution works for z/OS and all other current implementations. It, unfortunately, isn't a general solution. We would have to do a similar game if there were other function pointers that were compared across translation units. But this appears to be the only current case this happens. As long as the function pointers to be compared are known in the same translation unit, it works. This commit builds upon some ideas from Dave Mitchell and Richard Leach. People on the z/OS chat independently came up with the same general solution.
1 parent c00b151 commit adbdebe

File tree

3 files changed

+49
-3
lines changed

3 files changed

+49
-3
lines changed

opcode.h

Lines changed: 19 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

peep.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2205,14 +2205,14 @@ S_maybe_multideref(pTHX_ OP *start, OP *orig_o, UV orig_action, U8 hints)
22052205
/* if a custom array/hash access checker is in scope,
22062206
* abandon optimisation attempt */
22072207
if ( (o->op_type == OP_AELEM || o->op_type == OP_HELEM)
2208-
&& PL_check[o->op_type] != Perl_ck_null)
2208+
&& PL_check[o->op_type] != PL_check[PERL_CK_NULL])
22092209
return;
22102210
/* similarly for customised exists and delete */
22112211
if ( (o->op_type == OP_EXISTS)
2212-
&& PL_check[o->op_type] != Perl_ck_exists)
2212+
&& PL_check[o->op_type] != PL_check[PERL_CK_EXISTS])
22132213
return;
22142214
if ( (o->op_type == OP_DELETE)
2215-
&& PL_check[o->op_type] != Perl_ck_delete)
2215+
&& PL_check[o->op_type] != PL_check[PERL_CK_DELETE])
22162216
return;
22172217

22182218
if ( o->op_type != OP_AELEM

regen/opcode.pl

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1114,13 +1114,40 @@ sub generate_opcode_h_pl_check {
11141114
INIT({
11151115
END
11161116

1117+
11171118
for (@ops) {
11181119
print "\t", tab(3, "Perl_$check{$_},"), "\t/* $_ */\n";
11191120
}
11201121

1122+
print <<~'END';
1123+
1124+
/* The final entries are function pointers not attached to an opcode.
1125+
* These are to be used to compare with function pointers in the earlier
1126+
* part of the array, since in some platforms (notably z/OS), it is
1127+
* undefined behavior to compare function pointers for equality, even
1128+
* though calling them will invoke the same function. IBM personnel say
1129+
* that the comparisons do work when the pointers are compiled in the same
1130+
* translation unit. Hence, ck_null in all positions in the array will
1131+
* have the same value. See GH #23399 */
1132+
END
1133+
my @perl_internal_extras = qw(ck_null ck_exists ck_delete);
1134+
for (@perl_internal_extras) {
1135+
print "\t", tab(3, "Perl_$_,"), "\n";
1136+
}
1137+
11211138
print <<~'END';
11221139
});
1140+
1141+
/* Indexes into PL_check for the comparison function pointers */
1142+
#ifdef PERL_IN_PEEP_C
11231143
END
1144+
1145+
for (my $i = 0; $i < @perl_internal_extras; $i++) {
1146+
my $index = @ops + $i;
1147+
my $define = uc $perl_internal_extras[$i];
1148+
print " #define PERL_$define $index\n";
1149+
}
1150+
print "#endif\n";
11241151
}
11251152

11261153
sub generate_opcode_h_pl_opargs {

0 commit comments

Comments
 (0)