On 6/17/24 6:11 PM, Segher Boessenkool wrote:
> "ROP insns" are the instructions used in such exploits, not what you
> mean here :-)
>
> The instructions are called "hash*"C, so maybe call tbem "hash insns"
> or "ROP protect hash insns"?.
Ok, that bad verbiage was in the extra commentary not part of the git
log entry. That said, I'll reword that to the following:
Only disable shrink-wrapping when using -mrop-protect when we know we
-will be emitting the ROP instructions (ie, non-leaf functions).
+will be emitting the ROP protect hash instructions (ie, non-leaf functions).
>> * config/rs6000/rs6000.cc (rs6000_override_options_after_change): Move
>> the disabling of shrink-wrapping from here....
>> * config/rs6000/rs6000-logue.cc (rs6000_stack_info): ...to here.
>
> Hrm. Can you do it in some particular caller of rs6000_stack_info,
> instead? The rs6000_stack_info function itself is not suppposed to
> change any state whatsoever.
Sure, I can look at maybe moving that to the caller or maybe somewhere
better. I'll repost the patch once I find a better location.
> The comment should say *why*! The fact that we do is clear from the
> code itself already. But why do we want this?
>
>> --- a/gcc/config/rs6000/rs6000.cc
>> +++ b/gcc/config/rs6000/rs6000.cc
>> @@ -3427,10 +3427,6 @@ 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;
>> }
>
> (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.
>> --- /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. 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.
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.
Peter
