Created attachment 51105 [details] __memmove_ppc extracted from glibc When using ROP (-mrop-protect), it's expected that generated code reads the value from LR (mflr) and hash it later (hashst). This works well at -O0. However, at -O1 and higher, we're seeing cases where hashst appears before mflr. I'm attaching an example extracted from glibc. You can reproduce the issue with command: gcc -S -O1 -mrop-protect -mcpu=power10 memmove-ppc64.i -o - The generated asm contains the following: __memmove_ppc: .LFB6: .cfi_startproc .localentry __memmove_ppc,1 hashst 0,-40(1) std 28,-32(1) stdu 1,-80(1) .cfi_def_cfa_offset 80 .cfi_offset 28, -32 mr 28,3 subf 9,4,3 cmpld 0,9,5 bge 0,.L17 std 31,72(1) .cfi_offset 31, -8 add 4,4,5 add 31,3,5 cmpldi 0,5,15 ble 0,.L4 mflr 0 ...
Could you attach something that is a valid input to the compiler? Something that does not include the preprocessor directives... did you use -dM? Don't :-) (I can reproduce with this code, but there are many warnings :-) )
So, even if I use -fno-shrink-wrap and, although rs6000.c has anyway /* If we are inserting ROP-protect instructions, disable shrink wrap. */ if (rs6000_rop_protect) flag_shrink_wrap = 0; we *still* get shrink-wrapping done here?! But that cannot work with the ROP protection stuff.
Cc:ing Martin because this is options stuff.
Created attachment 51109 [details] __memmove_ppc extracted from glibc (without -g3) I removed -g3 from the command that generated memmove-ppc64.i. I think this won't generate warnings now.
Mine then.
Heh, another example of __attribute__((optimize("..."))) problem: __attribute__ ((__optimize__ ("-fno-tree-loop-distribute-patterns"))) __memmove_ppc ( void *dest, const void *src, size_t len) { ... } -mrop-protect is dropped when optimize attribute is parsed, for more information see: https://gcc.gnu.org/pipermail/gcc-patches/2021-August/577113.html
(In reply to Martin Liška from comment #6) > Heh, another example of __attribute__((optimize("..."))) problem: > > __attribute__ ((__optimize__ ("-fno-tree-loop-distribute-patterns"))) > __memmove_ppc ( void *dest, const void *src, size_t len) > { > ... > } > > -mrop-protect is dropped when optimize attribute is parsed, for more > information see: > https://gcc.gnu.org/pipermail/gcc-patches/2021-August/577113.html So what is the status with this? IIUC, looking at the thread, it seems like you and richi agreed that the optimize flags should be treated as if they were appended to the command line, but that doesn't seem how trunk works right now. So does GCC still need fixing or is the test case using the attribute optimize incorrectly or ???
FYI, here's a smaller test case that still shows the issue with today's trunk: extern void foo (void); long int __attribute__ ((__optimize__ ("-fno-tree-loop-distribute-patterns"))) __memmove_ppc (long int cond) { if (cond) foo (); return cond; }
> So what is the status with this? IIUC, looking at the thread, it seems like > you and richi agreed that the optimize flags should be treated as if they > were appended to the command line, but that doesn't seem how trunk works > right now. So does GCC still need fixing or is the test case using the > attribute optimize incorrectly or ??? The issue is still present and I can reproduce it. You are right, the flags are treated as if they were appended to the command line. We need a better place where the -mrop-protect is handled, let me take a look.
(In reply to Peter Bergner from comment #8) > FYI, here's a smaller test case that still shows the issue with today's > trunk: > > extern void foo (void); > long int > __attribute__ ((__optimize__ ("-fno-tree-loop-distribute-patterns"))) > __memmove_ppc (long int cond) > { > if (cond) > foo (); > return cond; > } All right, I have a patch candidate. Would it be possible to prepare a GCC test-suite test-case?
(In reply to Martin Liška from comment #10) > (In reply to Peter Bergner from comment #8) > > FYI, here's a smaller test case that still shows the issue with today's > > trunk: > > > > extern void foo (void); > > long int > > __attribute__ ((__optimize__ ("-fno-tree-loop-distribute-patterns"))) > > __memmove_ppc (long int cond) > > { > > if (cond) > > foo (); > > return cond; > > } > > All right, I have a patch candidate. Would it be possible to prepare a GCC > test-suite test-case? Great! I'm not sure we can detect the assembly directly, but I think a runnable test case should work. I'll work on that.
(In reply to Martin Liška from comment #10) > All right, I have a patch candidate. Would it be possible to prepare a GCC > test-suite test-case? Forgot to mention, if you want me to test your patch, I can do that on a POWER10 system to ensure the -mrop-protect stuff is working.
Created attachment 51668 [details] Patch candidate Please test the patch on a power10 machine, thanks.
(In reply to Martin Liška from comment #13) > Created attachment 51668 [details] > Patch candidate > > Please test the patch on a power10 machine, thanks. I'll kick it off now. Thanks!
(In reply to Peter Bergner from comment #14) > (In reply to Martin Liška from comment #13) > > Created attachment 51668 [details] > > Patch candidate > > > > Please test the patch on a power10 machine, thanks. > > I'll kick it off now. Thanks! This patch is okay for trunk if you (Peter) are happy with it, and it works ;-) Thank guys!
(In reply to Segher Boessenkool from comment #15) > (In reply to Peter Bergner from comment #14) > > (In reply to Martin Liška from comment #13) > > > Please test the patch on a power10 machine, thanks. > > > > I'll kick it off now. Thanks! > > This patch is okay for trunk if you (Peter) are happy with it, and it works The patch bootstrapped and regtested with no regressions. I have also conformed it fixes the issue, so I think this is good to commit. I'm having trouble with the runnable test case, so let's push the fix now and I'll submit the test case when I get it working? Thanks for the fix Martin!
Here's a compile time only test case that correctly FAILs using the unpatched compiler and passes using the patched compiler (requires a small change to target-supports.exp to add the rop_ok test): /* { 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," } } */
Namely this: diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp index 1c8b1ebb86e..0d9a3ba67ce 100644 --- a/gcc/testsuite/lib/target-supports.exp +++ b/gcc/testsuite/lib/target-supports.exp @@ -6625,6 +6625,13 @@ proc check_effective_target_powerpc_elfv2 { } { } } +# Return 1 if this is a PowerPC target supporting -mrop-protect + +proc check_effective_target_rop_ok { } { + return [check_effective_target_power10_ok] + && [check_effective_target_powerpc_elfv2] +} + # The VxWorks SPARC simulator accepts only EM_SPARC executables and # chokes on EM_SPARC32PLUS or EM_SPARCV9 executables. Return 1 if the # test environment appears to run executables on such a simulator.
I posted Martin's already approved patch to the gcc-patches mailing list along with a test case which need approval.
The master branch has been updated by Peter Bergner <bergner@gcc.gnu.org>: https://gcc.gnu.org/g:cff7879a381d3f5ed6556206896e6a6229800167 commit r12-5781-gcff7879a381d3f5ed6556206896e6a6229800167 Author: Martin Liska <mliska@suse.cz> Date: Fri Dec 3 11:58:25 2021 -0600 rs6000: Fix up flag_shrink_wrap handling in presence of -mrop-protect [PR101324] PR101324 shows a problem in disabling shrink-wrapping when using -mrop-protect when there is a attribute optimize/pragma. The fix envolves moving the handling of flag_shrink_wrap so it gets re-disbled when we change or add options. 2021-12-03 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-12-03 Peter Bergner <bergner@linux.ibm.com> gcc/testsuite/ PR target/101324 * gcc.target/powerpc/pr101324.c: New test.
Fixed on trunk.
So this is also broken in GCC11, so I'm testing the simple backport.
(In reply to Peter Bergner from comment #22) > So this is also broken in GCC11, so I'm testing the simple backport. Regression testing of the backport was clean. Just need approval for the backport.
(In reply to Peter Bergner from comment #21) > Fixed on trunk. I tested gcc trunk with glibc master and I confirm that it fix the problem with __memmove_ppc. I tested both running glibc check with a custom kernel with rop enable and doing a visual check of the disassembled function. The exact versions I used were: - binutils 2.36.1 build from binutils-2_36_1 (f35674005e60) - gcc (future 12) build from master (b7815aa9108) - glibc (future 2.35) build from master (268d812c19)
The releases/gcc-11 branch has been updated by Peter Bergner <bergner@gcc.gnu.org>: https://gcc.gnu.org/g:c56c398c39f6195c3d158f02514c33b7da73ebc2 commit r11-9560-gc56c398c39f6195c3d158f02514c33b7da73ebc2 Author: Martin Liska <mliska@suse.cz> Date: Fri Dec 3 11:58:25 2021 -0600 rs6000: Fix up flag_shrink_wrap handling in presence of -mrop-protect [PR101324] PR101324 shows a problem in disabling shrink-wrapping when using -mrop-protect when there is a attribute optimize/pragma. The fix envolves moving the handling of flag_shrink_wrap so it gets re-disbled when we change or add options. 2021-12-03 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-12-03 Peter Bergner <bergner@linux.ibm.com> gcc/testsuite/ PR target/101324 * gcc.target/powerpc/pr101324.c: New test. (cherry picked from commit cff7879a381d3f5ed6556206896e6a6229800167)
Fixed everywhere now.
FYI, as part of the work for PR114759, I have come to the conclusion that disabling shrink-wrapping in the presence of -mrop-protect is a big hammer and we shouldn't really need to do that. I plan on "fixing" that as part of my PR114759 work.