Hi!
On Mon, Jun 17, 2024 at 06:49:18PM -0500, Peter Bergner wrote:
> On 6/17/24 6:11 PM, Segher Boessenkool wrote:
> >> - /* If we are inserting ROP-protect instructions, disable shrink wrap.
> >> */
> >> - if (rs6000_rop_protect)
> >> - flag_shrink_wrap = 0;
> >> }
> >
> > (Yes, I know the original code didn't say either, but let's try to make
> > things better :-) )
>
> Yeah, I didn't write that, I only moved it, but I can try to come up with
> an explanation of why we need to disable it now. That said, my hope is to
> not have to disable shrink-wrapping even when we emit the ROP protect hash
> insns in the future, but that will take some extra work. If I can manage
> that, then this should all just go away. :-) Until then, we can stick
> with this patch's micro-optimization.
If you inline one function into another, there is no ROP protection on
their boundary anymore (since there is no such boundary anymore!) This
is not necessarily a problem, but you do want some noipa or similar
markup where without ROP protection you have no incentive to do that.
Shrink-wrapping allows more inlining, and more inlining allows more
shrink-wrapping, but there is no direct relation between shrink-wrapping
and our ROP protect stuff? We just need to make sure the hashst and
hashchk things are done at the very start and the very end of the
functions, but we need to make sure of that anyway!
So yeah, please investigate a bit more :-)
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.target/powerpc/pr114759-1.c
> >> @@ -0,0 +1,16 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect
> >> -fdump-rtl-pro_and_epilogue" } */
> >> +/* { dg-require-effective-target rop_ok } */
> >
> > Do you want rop_ok while you are *forcing* it to be okay anyway? Why?
>
> At the moment, yes, since the rop_ok test not only checks for the -mcpu=
> level,
> it also verifies that the ABI is ok.
Ah right! Add a short comment?
> Currently, rop_ok makes sure we have
> Power10 and ELFv2 ABI being used. So currently, if we were to run this test
> on BE, we'd get an UNSUPPORTED using the rop_ok check, but if we removed it,
> we'd see a FAIL.
Yup.
> As we discussed offline, the plan is to eventually enable emitting the ROP
> protect
> hash insns on other ABIs, but until then, I think we want to keep the rop_ok
> check
> so as to keep Bill's CI builder from flagging it as a FAIL.
:-)
Segher
