Closed p5pRT closed 12 years ago
Under +O1 compiler optimization\, the test passes fine:
$ ./perl -I./lib ext/XS-APItest/t/coplabel.t 1..1 ok 1
But when +O2 is used\, not so much:
$ ./perl -I./lib ext/XS-APItest/t/coplabel.t 1..1 fail # cop_fetch_label label at ext/XS-APItest/t/coplabel.t line 7.
I've filed this as a bug because the default -Doptimize value when Configure is run is '+O2 +Onolimit'.
I believe the issue is partly due to reordering of instructions. Here's the C code generated from APItest.xs:
XS_EUPXS(XS_XS__APItest_test_coplabel) { dVAR; dXSARGS; if (items != 0) croak_xs_usage(cv\, ""); { #line 2643 "APItest.xs" COP *cop; const char *label; STRLEN len; U32 utf8; #line 4476 "APItest.c" #line 2648 "APItest.xs" cop = &PL_compiling; Perl_cop_store_label(aTHX_ cop\, "foo"\, 3\, 0); label = Perl_cop_fetch_label(aTHX_ cop\, &len\, &utf8); if (strcmp(label\,"foo")) croak("fail # cop_fetch_label label"); if (len != 3) croak("fail # cop_fetch_label len"); if (utf8) croak("fail # cop_fetch_label utf8"); /* SMALL GERMAN UMLAUT A */ Perl_cop_store_label(aTHX_ cop\, "foC$"\, 4\, SVf_UTF8); label = Perl_cop_fetch_label(aTHX_ cop\, &len\, &utf8); if (strcmp(label\,"foC$")) croak("fail # cop_fetch_label label"); if (len != 4) croak("fail # cop_fetch_label len"); if (!utf8) croak("fail # cop_fetch_label utf8"); #line 4490 "APItest.c" } XSRETURN_EMPTY; }
Here's a snippet from a gdb debugging session under +O1:
2649 Perl_cop_store_label(aTHX_ cop\, "foo"\, 3\, 0); (gdb) n 2650 label = Perl_cop_fetch_label(aTHX_ cop\, &len\, &utf8); (gdb) n 2651 if (strcmp(label\,"foo")) croak("fail # cop_fetch_label label"); (gdb) n 2652 if (len != 3) croak("fail # cop_fetch_label len"); (gdb) n 2653 if (utf8) croak("fail # cop_fetch_label utf8"); (gdb) n 2655 Perl_cop_store_label(aTHX_ cop\, "foä"\, 4\, SVf_UTF8); (gdb) n 2656 label = Perl_cop_fetch_label(aTHX_ cop\, &len\, &utf8); (gdb) n 2657 if (strcmp(label\,"foä")) croak("fail # cop_fetch_label label"); (gdb) p label $1 = 0x600000000051ac3d "fo\303\244" (gdb) p len $2 = 4 (gdb) p utf8 $3 = 536870912 (gdb) n 2658 if (len != 4) croak("fail # cop_fetch_label len"); (gdb) n 2659 if (!utf8) croak("fail # cop_fetch_label utf8"); (gdb) n 4491 XSRETURN_EMPTY;
Note the contrast when +O2 is in force:
2650 label = Perl_cop_fetch_label(aTHX_ cop\, &len\, &utf8);
(gdb) n
2651 if (strcmp(label\,"foo")) croak("fail # cop_fetch_label
label");
(gdb) p label
$1 = \
Due to the opaqueness imposed by the +O2 optimizations\, it's hard to tell exactly what happened\, but somehow the optimizations are inducing a false exit path.
Reordering the tests within this function\, and changing strcmp() to memcmp()\, appears to resolve the issue (i.e. we can compile with the default of -Doptimize='+O2 +Onolimit' and this test passes).
Patch forthcoming.
Philip Monsen wrote:
Reordering the tests within this function\, and changing strcmp() to memcmp()\, appears to resolve the issue (i.e. we can compile with the default of -Doptimize='+O2 +Onolimit' and this test passes).
Presumably the same optimisation bug will strike in other places that we don't test. Can we resolve it instead by modifying the optimisation flags?
-zefram
The RT System itself - Status changed from 'new' to 'open'
On Tue\, Nov 15\, 2011 at 10:43 AM\, Zefram \zefram@​fysh\.org wrote:
Presumably the same optimisation bug will strike in other places that we don't test. Can we resolve it instead by modifying the optimisation flags?
I thought about that\, but it seemed like overkill considering that all other tests in the test suite pass\, and given that this default build option hasn't changed for a long time. +O2 alone is sufficient to trigger the issue\, and downgrading optimization from that level for a production build doesn't seem appropriate just to fix one test.
Someone more familiar with the core code being tested could probably revise the test_coplabel() code (and/or its pure-Perl test invocation code) to insulate more generally against this bug. My patch is just one way to do it.
--Phil
The attached patch corrects the issue. To illustrate the difference between this and what's currently in blead\, here's a snippet from a gdb session with the patch applied and +O2 in force:
2650 label = Perl_cop_fetch_label(aTHX_ cop\, &len\, &utf8); (gdb) n 2651 if (len != 3) croak("fail # test1 cop_fetch_label len"); (gdb) n 2652 if (memcmp(label\,"foo"\,len)) (gdb) n 2654 if (utf8) croak("fail # test1 cop_fetch_label utf8"); (gdb) n 2656 Perl_cop_store_label(aTHX_ cop\, "foä"\, 4\, SVf_UTF8); (gdb) n 2657 label = Perl_cop_fetch_label(aTHX_ cop\, &len\, &utf8); (gdb) n 2658 if (len != 4) croak("fail # test2 cop_fetch_label len"); (gdb) n 2659 if (memcmp(label\,"foä"\,len)) (gdb) n 2661 if (!utf8) croak("fail # test2 cop_fetch_label utf8"); (gdb) n Perl_pp_entersub () at pp_hot.c:2762
Note how the optimization no longer induces a bogus failure and early exit.
Philip Monsen wrote:
The attached patch corrects the issue.
The patch has a small fault\, that the new code with memcmp() isn't testing for the nul terminator on the strings. That's easily fixed by incrementing the block length parameter.
However\, looking at the change\, I am even less happy about patching it here than I was previously. The kind of operations that are evidently triggering the compiler bug are very ordinary\, and we do that sort of thing all over the place. I am not satisfied that the rest of the test suite can be relied upon to catch it\, if a bug of this kind is affecting core behaviour. This test is the canary that warns us that this compiler is broken with the options we give it; nailing the canary to its perch is not the appropriate response.
We need to turn off the broken optimisation. If that means dropping all the way from +O2 to +O1 then so be it. I prefer to get the right answer slowly.
-zefram
On Wed\, Nov 16\, 2011 at 7:41 AM\, Zefram \zefram@​fysh\.org wrote:
We need to turn off the broken optimisation. If that means dropping all the way from +O2 to +O1 then so be it. I prefer to get the right answer slowly.
Fair enough. I'll try using +O2 as a base and pulling specific optimizations down to what they would be at +O1 and see if I can find the culprit that way.
--Phil
On Wed\, Nov 16\, 2011 at 8:19 AM\, Philip Monsen \philip\.monsen@​pobox\.comwrote:
On Wed\, Nov 16\, 2011 at 7:41 AM\, Zefram \zefram@​fysh\.org wrote:
We need to turn off the broken optimisation. If that means dropping all the way from +O2 to +O1 then so be it. I prefer to get the right answer slowly.
Fair enough. I'll try using +O2 as a base and pulling specific optimizations down to what they would be at +O1 and see if I can find the culprit that way.
I added +Oinfo to see what optimizations are actually occurring. Looks like inlining may be the culprit (these are the only messages for the test_coplabel() function):
"APItest.c"\, line 2651\, procedure XS_XS__APItest_test_coplabel: info #20055-D: Inlined call to strcmp
"APItest.c"\, line 2657\, procedure XS_XS__APItest_test_coplabel: info #20055-D: Inlined call to strcmp
Default for +O2 is '+inline_level 2' and for +O1 is '+inline_level 1'. Inline levels can be specified in increments of 0.1 between 1 and 9. Will see if tweaking this down corrects the issue\, and if so\, the maximum setting for which all tests still pass.
--Phil
On Wed\, Nov 16\, 2011 at 9:30 AM\, Philip Monsen \philip\.monsen@​pobox\.comwrote:
I added +Oinfo to see what optimizations are actually occurring. Looks like inlining may be the culprit (these are the only messages for the test_coplabel() function):
"APItest.c"\, line 2651\, procedure XS_XS__APItest_test_coplabel: info #20055-D: Inlined call to strcmp
"APItest.c"\, line 2657\, procedure XS_XS__APItest_test_coplabel: info #20055-D: Inlined call to strcmp
Default for +O2 is '+inline_level 2' and for +O1 is '+inline_level 1'. Inline levels can be specified in increments of 0.1 between 1 and 9. Will see if tweaking this down corrects the issue\, and if so\, the maximum setting for which all tests still pass.
This wasn't quite the right tweak\, but the +Oinfo report was helpful. Looking across the whole build\, the only str* function being inlined was strcmp(). Various attempts to squash this inlining did not succeed\, to include:
+O2 +Onoinline +O2 +Onoinline=strcmp
However\, using '+O2 +Onolibcalls=strcmp' results in this test passing. Here's the gdb session (nice and clean):
2650 label = Perl_cop_fetch_label(aTHX_ cop\, &len\, &utf8); (gdb) n 2651 if (strcmp(label\,"foo")) croak("fail # cop_fetch_label label"); (gdb) n 2652 if (len != 3) croak("fail # cop_fetch_label len"); (gdb) n 2653 if (utf8) croak("fail # cop_fetch_label utf8"); (gdb) n 2655 Perl_cop_store_label(aTHX_ cop\, "foä"\, 4\, SVf_UTF8); (gdb) n 2656 label = Perl_cop_fetch_label(aTHX_ cop\, &len\, &utf8); (gdb) n 2657 if (strcmp(label\,"foä")) croak("fail # cop_fetch_label label"); (gdb) n 2658 if (len != 4) croak("fail # cop_fetch_label len"); (gdb) n 2659 if (!utf8) croak("fail # cop_fetch_label utf8"); (gdb) n Perl_pp_entersub () at pp_hot.c:2762
I'm going to rebuild once more with '+O2 +Onolimit +Olibcalls=strcmp'\, and assuming I get a clean test run\, I'll post a fresh patch\, this time to hints/hpux.sh
--Phil
On Wed\, Nov 16\, 2011 at 11:08 AM\, Philip Monsen \philip\.monsen@​pobox\.comwrote:
I'm going to rebuild once more with '+O2 +Onolimit +Onolibcalls=strcmp'\, and assuming I get a clean test run\, I'll post a fresh patch\, this time to hints/hpux.sh
Promised patch (to hints/hpux.sh) against blead attached. This one also corrects a couple of incidental things I ran across in that file while preparing the patch\, but most importantly\, fixes the default optimization flags for my narrow combination of platform + compiler version. I think the version coverage (i.e. versions beyond B3910B A.06.15) could/should be expanded if/when other folks could confirm that the bug is also triggered with such compiler versions.
I added some additional relaxations to the optimize string\, as the +Onolibcalls=strcmp by itself corrected the failing test but appeared to induce another failure (in t/op/state.t). The resulting collection of flags:
+O2 +Onolimit +Onoprocelim +Ostore_ordering +Onolibcalls=strcmp
also seems reasonable to me given the initial observed behavior\, and avoids having to punt back to +O1.
With this patch applied\, all tests in the test suite pass when the (updated) default optimization string is used. The earlier patch I submitted\, to the xs/XS-APItest/APItest.xs file\, should be disregarded.
--Phil
Philip Monsen wrote:
Promised patch (to hints/hpux.sh) against blead attached.
Thanks\, applied as 2ba3ed0.
-zefram
@cpansprout - Status changed from 'open' to 'resolved'
Migrated from rt.perl.org#103668 (status was 'resolved')
Searchable as RT103668$