| Message ID | 6c8a8e20-674e-3954-a7e2-63c2eedac828@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000: Fix up flag_shrink_wrap handling in presence of -mrop-protect [PR101324] | expand |
On Wed, Oct 27, 2021 at 10:17:39PM -0500, Peter Bergner wrote: > Sorry for reposting, but I forgot to CC the gcc-patches mailing list. :-( Whoops, and I replied to your original message :-/ > 2021-10-27 Martin Liska <mliska@suse.cz> > > gcc/ > PR target/101324 > * config/rs6000/rs6000.c (rs6000_option_override_internal): Move the > disabling of shrink-wrapping when using -mrop-protect from here... > (rs6000_override_options_after_change): ...to here. > > 2021-10-27 Peter Bergner <bergner@linux.ibm.com> > > gcc/testsuite/ > PR target/101324 > * gcc.target/powerpc/pr101324.c: New test. > +/* Ensure hashst comes after mflr and hashchk comes after ld 0,16(1). */ > +/* { dg-final { scan-assembler "mflr 0.*hashst 0," } } */ > +/* { dg-final { scan-assembler "ld 0,16\\\(1\\\).*hashchk 0," } } */ First: don't use double quotes, or you get double backslashes (or more) as well. Use curlies instead: /* { dg-final { scan-assembler {ld 0,16\(1\).*hashchk 0,} } } */ But, more importantly, "." by default matches anything, newlines as well. You probably do not want that here, because your RE as written can match an "ld" in one function and a "hashchk" many functions later, many million lines later. You can for example do /* { dg-final { scan-assembler {(?p)ld 0,.*\n.*\mhashchk 0,} } } */ (?p) is "partial newline-sensitive matching": it makes "." not match newlines. This is often what you want. This RE also makes sure that "hashchk" is the full mnemonic (not the tail of one), and that it is on the line after that "ld". Similarly you would have /* { dg-final { scan-assembler {(?p)\mmflr 0,.*\n.*\mhashst 0,} } } */ I hope I didn't typo those things, I didn't test them out :-) Okay for trunk with similar robustification. Thanks! Segher
Sorry for taking so long to get back to this. On 10/29/21 4:45 PM, Segher Boessenkool wrote: > On Wed, Oct 27, 2021 at 10:17:39PM -0500, Peter Bergner wrote: >> +/* Ensure hashst comes after mflr and hashchk comes after ld 0,16(1). */ >> +/* { dg-final { scan-assembler "mflr 0.*hashst 0," } } */ >> +/* { dg-final { scan-assembler "ld 0,16\\\(1\\\).*hashchk 0," } } */ > > First: don't use double quotes, or you get double backslashes (or more) > as well. Use curlies instead: > > /* { dg-final { scan-assembler {ld 0,16\(1\).*hashchk 0,} } } */ > > But, more importantly, "." by default matches anything, newlines as > well. You probably do not want that here, because your RE as written > can match an "ld" in one function and a "hashchk" many functions later, > many million lines later. > > You can for example do > /* { dg-final { scan-assembler {(?p)ld 0,.*\n.*\mhashchk 0,} } } */ I had to change your suggestion to the following, which works: /* { dg-final { scan-assembler {(?p)\mmflr 0.*\n.*\n.*\mhashst 0,} } } */ /* { dg-final { scan-assembler {(?p)ld 0,.*\n.*\n.*\n.*\mhashchk 0,} } } */ Meaning from the current code generation, the hashst is 2 insns after the mflr insn and the hashck is 3 insns after the ld 0,16(1) insn. But is this fragile? Are we sure we won't schedule the hashst and hashchk to some other location breaking the test case? > (?p) is "partial newline-sensitive matching": it makes "." not match > newlines. This is often what you want. This RE also makes sure that > "hashchk" is the full mnemonic (not the tail of one), and that it is on > the line after that "ld". Well, this test case only has one function and is very small, so there should only be one "mflr 0" and one "ld 0,16(1)". Given the small size, I assumed if the hashst and hashchk were after the mflr/ld 0,16(1), then it's in the correct order and "fixed". But maybe that's just as fragile as assuming how many insns proceed the hashst/hashchk? If you prefer the updated change to your suggestion, let me know and I'll commit it that way. > I'll note the test case uses the "new" rop_ok effective-target function which > I submitted as a separate patch. This test case uses the new rop_ok effective-target which I said I submitted as a separate patch, but I can't seem to find the submission anywhere and it's not even in the archive. Did I forget to submit it? Probably. :-( Let me do that now, as this relies on that patch. Peter
On 10/29/21 4:45 PM, Segher Boessenkool wrote: > On Wed, Oct 27, 2021 at 10:17:39PM -0500, Peter Bergner wrote: >> 2021-10-27 Martin Liska <mliska@suse.cz> >> >> gcc/ >> PR target/101324 >> * config/rs6000/rs6000.c (rs6000_option_override_internal): Move the >> disabling of shrink-wrapping when using -mrop-protect from here... >> (rs6000_override_options_after_change): ...to here. >> >> 2021-10-27 Peter Bergner <bergner@linux.ibm.com> >> >> gcc/testsuite/ >> PR target/101324 >> * gcc.target/powerpc/pr101324.c: New test. > > Okay for trunk with similar robustification. Thanks! With the rop_ok change finally committed, I have finally pushed this change with your suggested test case changes. Thanks Martin for the fix and Segher for the reviews. Peter
On 12/3/21 2:39 PM, Peter Bergner wrote: > On 10/29/21 4:45 PM, Segher Boessenkool wrote: >> On Wed, Oct 27, 2021 at 10:17:39PM -0500, Peter Bergner wrote: >>> 2021-10-27 Martin Liska <mliska@suse.cz> >>> >>> gcc/ >>> PR target/101324 >>> * config/rs6000/rs6000.c (rs6000_option_override_internal): Move the >>> disabling of shrink-wrapping when using -mrop-protect from here... >>> (rs6000_override_options_after_change): ...to here. >>> >>> 2021-10-27 Peter Bergner <bergner@linux.ibm.com> >>> >>> gcc/testsuite/ >>> PR target/101324 >>> * gcc.target/powerpc/pr101324.c: New test. >> >> Okay for trunk with similar robustification. Thanks! > > With the rop_ok change finally committed, I have finally pushed this > change with your suggested test case changes. Thanks Martin for the > fix and Segher for the reviews. So I just checked and we have the same failure on GCC11 too. Ok for the GCC11 release branch after this has burned-in on trunk for a couple of days? Ditto for the rop_ok testsuite patch that goes with this one? Peter
On 12/3/21 3:27 PM, Peter Bergner wrote: > On 12/3/21 2:39 PM, Peter Bergner wrote: >> On 10/29/21 4:45 PM, Segher Boessenkool wrote: >>> On Wed, Oct 27, 2021 at 10:17:39PM -0500, Peter Bergner wrote: >>>> 2021-10-27 Martin Liska <mliska@suse.cz> >>>> >>>> gcc/ >>>> PR target/101324 >>>> * config/rs6000/rs6000.c (rs6000_option_override_internal): Move the >>>> disabling of shrink-wrapping when using -mrop-protect from here... >>>> (rs6000_override_options_after_change): ...to here. >>>> >>>> 2021-10-27 Peter Bergner <bergner@linux.ibm.com> >>>> >>>> gcc/testsuite/ >>>> PR target/101324 >>>> * gcc.target/powerpc/pr101324.c: New test. >>> >>> Okay for trunk with similar robustification. Thanks! >> >> With the rop_ok change finally committed, I have finally pushed this >> change with your suggested test case changes. Thanks Martin for the >> fix and Segher for the reviews. > > So I just checked and we have the same failure on GCC11 too. > Ok for the GCC11 release branch after this has burned-in on > trunk for a couple of days? > > Ditto for the rop_ok testsuite patch that goes with this one? FYI, both commits backported cleanly and showed no testsuite regressions on powerrpc64le-linux. Peter
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index bac959f4ef4..95e0d2cffdd 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -3484,6 +3484,10 @@ rs6000_override_options_after_change (void) } else if (!OPTION_SET_P (flag_cunroll_grow_size)) flag_cunroll_grow_size = flag_peel_loops || optimize >= 3; + + /* If we are inserting ROP-protect instructions, disable shrink wrap. */ + if (rs6000_rop_protect) + flag_shrink_wrap = 0; } #ifdef TARGET_USES_LINUX64_OPT @@ -4048,10 +4052,6 @@ rs6000_option_override_internal (bool global_init_p) && ((rs6000_isa_flags_explicit & OPTION_MASK_QUAD_MEMORY_ATOMIC) == 0)) rs6000_isa_flags |= OPTION_MASK_QUAD_MEMORY_ATOMIC; - /* If we are inserting ROP-protect instructions, disable shrink wrap. */ - if (rs6000_rop_protect) - flag_shrink_wrap = 0; - /* If we can shrink-wrap the TOC register save separately, then use -msave-toc-indirect unless explicitly disabled. */ if ((rs6000_isa_flags_explicit & OPTION_MASK_SAVE_TOC_INDIRECT) == 0 diff --git a/gcc/testsuite/gcc.target/powerpc/pr101324.c b/gcc/testsuite/gcc.target/powerpc/pr101324.c new file mode 100644 index 00000000000..d27cc2876f3 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr101324.c @@ -0,0 +1,17 @@ +/* { dg-require-effective-target rop_ok } */ +/* { dg-options "-O1 -mrop-protect -mdejagnu-cpu=power10" } */ + +extern void foo (void); + +long int +__attribute__ ((__optimize__ ("no-inline"))) +func (long int cond) +{ + if (cond) + foo (); + return cond; +} + +/* Ensure hashst comes after mflr and hashchk comes after ld 0,16(1). */ +/* { dg-final { scan-assembler "mflr 0.*hashst 0," } } */ +/* { dg-final { scan-assembler "ld 0,16\\\(1\\\).*hashchk 0," } } */