Message ID | 8c29d19f-6739-4fb1-835d-52c00d84c83b@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | [v2] rs6000: Error on CPUs and ABIs that don't support the ROP, protection insns [PR114759] | expand |
Hi Peter, on 2024/7/16 06:07, Peter Bergner wrote: > Hi Kewen, > > Here's the updated patch per your review comments, minus your suggestion > to disable the ROP mask which I mentioned isn't needed in my other reply. > > This passed bootstrap and regtesting with no regressions on powerpc64le-linux. > Ok for trunk? OK for trunk, thanks! BR, Kewen > > Peter > > > Changes from v1: > * Moved checks for invalid targets from rs6000_override_options_after_change > to rs6000_option_override_internal. > > > rs6000: Error on CPUs and ABIs that don't support the ROP, protection insns [PR114759] > > We currently silently ignore the -mrop-protect option for old CPUs we don't > support with the ROP hash insns, but we throw an error for unsupported ABIs. > This patch treats unsupported CPUs and ABIs similarly by throwing an error > both both. This matches clang behavior and allows us to simplify our tests > in the code that generates our prologue and epilogue code. > > 2024-07-15 Peter Bergner <bergner@linux.ibm.com> > > gcc/ > PR target/114759 > * config/rs6000/rs6000.cc (rs6000_option_override_internal): Disallow > CPUs and ABIs that do no support the ROP protection insns. > * config/rs6000/rs6000-logue.cc (rs6000_stack_info): Remove now > unneeded tests. > (rs6000_emit_prologue): Likewise. > Remove unneeded gcc_assert. > (rs6000_emit_epilogue): Likewise. > * config/rs6000/rs6000.md: Likewise. > > gcc/testsuite/ > PR target/114759 > * gcc.target/powerpc/pr114759-3.c: New test. > --- > gcc/config/rs6000/rs6000-logue.cc | 22 +++++-------------- > gcc/config/rs6000/rs6000.cc | 12 ++++++++++ > gcc/config/rs6000/rs6000.md | 4 ++-- > gcc/testsuite/gcc.target/powerpc/pr114759-3.c | 19 ++++++++++++++++ > 4 files changed, 39 insertions(+), 18 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/powerpc/pr114759-3.c > > diff --git a/gcc/config/rs6000/rs6000-logue.cc b/gcc/config/rs6000/rs6000-logue.cc > index bd363b625a4..fdb6414f486 100644 > --- a/gcc/config/rs6000/rs6000-logue.cc > +++ b/gcc/config/rs6000/rs6000-logue.cc > @@ -716,17 +716,11 @@ rs6000_stack_info (void) > info->calls_p = (!crtl->is_leaf || cfun->machine->ra_needs_full_frame); > info->rop_hash_size = 0; > > - if (TARGET_POWER8 > - && info->calls_p > - && DEFAULT_ABI == ABI_ELFv2 > - && rs6000_rop_protect) > + /* If we want ROP protection and this function makes a call, indicate > + we need to create a stack slot to save the hashed return address in. */ > + if (rs6000_rop_protect > + && info->calls_p) > info->rop_hash_size = 8; > - else if (rs6000_rop_protect && DEFAULT_ABI != ABI_ELFv2) > - { > - /* We can't check this in rs6000_option_override_internal since > - DEFAULT_ABI isn't established yet. */ > - error ("%qs requires the ELFv2 ABI", "-mrop-protect"); > - } > > /* Determine if we need to save the condition code registers. */ > if (save_reg_p (CR2_REGNO) > @@ -3277,9 +3271,8 @@ rs6000_emit_prologue (void) > /* NOTE: The hashst isn't needed if we're going to do a sibcall, > but there's no way to know that here. Harmless except for > performance, of course. */ > - if (TARGET_POWER8 && rs6000_rop_protect && info->rop_hash_size != 0) > + if (info->rop_hash_size) > { > - gcc_assert (DEFAULT_ABI == ABI_ELFv2); > rtx stack_ptr = gen_rtx_REG (Pmode, STACK_POINTER_REGNUM); > rtx addr = gen_rtx_PLUS (Pmode, stack_ptr, > GEN_INT (info->rop_hash_save_offset)); > @@ -5056,12 +5049,9 @@ rs6000_emit_epilogue (enum epilogue_type epilogue_type) > > /* The ROP hash check must occur after the stack pointer is restored > (since the hash involves r1), and is not performed for a sibcall. */ > - if (TARGET_POWER8 > - && rs6000_rop_protect > - && info->rop_hash_size != 0 > + if (info->rop_hash_size > && epilogue_type != EPILOGUE_TYPE_SIBCALL) > { > - gcc_assert (DEFAULT_ABI == ABI_ELFv2); > rtx stack_ptr = gen_rtx_REG (Pmode, STACK_POINTER_REGNUM); > rtx addr = gen_rtx_PLUS (Pmode, stack_ptr, > GEN_INT (info->rop_hash_save_offset)); > diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc > index fd6e013c346..1cee9c2011d 100644 > --- a/gcc/config/rs6000/rs6000.cc > +++ b/gcc/config/rs6000/rs6000.cc > @@ -4825,6 +4825,18 @@ rs6000_option_override_internal (bool global_init_p) > } > } > > + /* We only support ROP protection on certain targets. */ > + if (rs6000_rop_protect) > + { > + /* Disallow CPU targets we don't support. */ > + if (!TARGET_POWER8) > + error ("%<-mrop-protect%> requires %<-mcpu=power8%> or later"); > + > + /* Disallow ABI targets we don't support. */ > + if (DEFAULT_ABI != ABI_ELFv2) > + error ("%<-mrop-protect%> requires the ELFv2 ABI"); > + } > + > /* Initialize all of the registers. */ > rs6000_init_hard_regno_mode_ok (global_init_p); > > diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md > index 694076e311f..75fe51b8d43 100644 > --- a/gcc/config/rs6000/rs6000.md > +++ b/gcc/config/rs6000/rs6000.md > @@ -15810,7 +15810,7 @@ (define_insn "hashst" > [(set (match_operand:DI 0 "simple_offsettable_mem_operand" "=m") > (unspec_volatile:DI [(match_operand:DI 1 "int_reg_operand" "r")] > UNSPEC_HASHST))] > - "TARGET_POWER8 && rs6000_rop_protect" > + "rs6000_rop_protect" > { > static char templ[32]; > const char *p = rs6000_privileged ? "p" : ""; > @@ -15823,7 +15823,7 @@ (define_insn "hashchk" > [(unspec_volatile [(match_operand:DI 0 "int_reg_operand" "r") > (match_operand:DI 1 "simple_offsettable_mem_operand" "m")] > UNSPEC_HASHCHK)] > - "TARGET_POWER8 && rs6000_rop_protect" > + "rs6000_rop_protect" > { > static char templ[32]; > const char *p = rs6000_privileged ? "p" : ""; > diff --git a/gcc/testsuite/gcc.target/powerpc/pr114759-3.c b/gcc/testsuite/gcc.target/powerpc/pr114759-3.c > new file mode 100644 > index 00000000000..6770a9aec3b > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr114759-3.c > @@ -0,0 +1,19 @@ > +/* PR target/114759 */ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -mdejagnu-cpu=power7 -mrop-protect" } */ > + > +/* Verify we emit an error if we use -mrop-protect with an unsupported cpu. */ > + > +extern void foo (void); > + > +int > +bar (void) > +{ > + foo (); > + return 5; > +} > + > +/* The correct line number is in the preamble to the error message, not > + in the final line (which is all that dg-error inspects). Hence, we have > + to tell dg-error to ignore the line number. */ > +/* { dg-error "'-mrop-protect' requires '-mcpu=power8'" "PR114759" { target *-*-* } 0 } */
diff --git a/gcc/config/rs6000/rs6000-logue.cc b/gcc/config/rs6000/rs6000-logue.cc index bd363b625a4..fdb6414f486 100644 --- a/gcc/config/rs6000/rs6000-logue.cc +++ b/gcc/config/rs6000/rs6000-logue.cc @@ -716,17 +716,11 @@ rs6000_stack_info (void) info->calls_p = (!crtl->is_leaf || cfun->machine->ra_needs_full_frame); info->rop_hash_size = 0; - if (TARGET_POWER8 - && info->calls_p - && DEFAULT_ABI == ABI_ELFv2 - && rs6000_rop_protect) + /* If we want ROP protection and this function makes a call, indicate + we need to create a stack slot to save the hashed return address in. */ + if (rs6000_rop_protect + && info->calls_p) info->rop_hash_size = 8; - else if (rs6000_rop_protect && DEFAULT_ABI != ABI_ELFv2) - { - /* We can't check this in rs6000_option_override_internal since - DEFAULT_ABI isn't established yet. */ - error ("%qs requires the ELFv2 ABI", "-mrop-protect"); - } /* Determine if we need to save the condition code registers. */ if (save_reg_p (CR2_REGNO) @@ -3277,9 +3271,8 @@ rs6000_emit_prologue (void) /* NOTE: The hashst isn't needed if we're going to do a sibcall, but there's no way to know that here. Harmless except for performance, of course. */ - if (TARGET_POWER8 && rs6000_rop_protect && info->rop_hash_size != 0) + if (info->rop_hash_size) { - gcc_assert (DEFAULT_ABI == ABI_ELFv2); rtx stack_ptr = gen_rtx_REG (Pmode, STACK_POINTER_REGNUM); rtx addr = gen_rtx_PLUS (Pmode, stack_ptr, GEN_INT (info->rop_hash_save_offset)); @@ -5056,12 +5049,9 @@ rs6000_emit_epilogue (enum epilogue_type epilogue_type) /* The ROP hash check must occur after the stack pointer is restored (since the hash involves r1), and is not performed for a sibcall. */ - if (TARGET_POWER8 - && rs6000_rop_protect - && info->rop_hash_size != 0 + if (info->rop_hash_size && epilogue_type != EPILOGUE_TYPE_SIBCALL) { - gcc_assert (DEFAULT_ABI == ABI_ELFv2); rtx stack_ptr = gen_rtx_REG (Pmode, STACK_POINTER_REGNUM); rtx addr = gen_rtx_PLUS (Pmode, stack_ptr, GEN_INT (info->rop_hash_save_offset)); diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc index fd6e013c346..1cee9c2011d 100644 --- a/gcc/config/rs6000/rs6000.cc +++ b/gcc/config/rs6000/rs6000.cc @@ -4825,6 +4825,18 @@ rs6000_option_override_internal (bool global_init_p) } } + /* We only support ROP protection on certain targets. */ + if (rs6000_rop_protect) + { + /* Disallow CPU targets we don't support. */ + if (!TARGET_POWER8) + error ("%<-mrop-protect%> requires %<-mcpu=power8%> or later"); + + /* Disallow ABI targets we don't support. */ + if (DEFAULT_ABI != ABI_ELFv2) + error ("%<-mrop-protect%> requires the ELFv2 ABI"); + } + /* Initialize all of the registers. */ rs6000_init_hard_regno_mode_ok (global_init_p); diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md index 694076e311f..75fe51b8d43 100644 --- a/gcc/config/rs6000/rs6000.md +++ b/gcc/config/rs6000/rs6000.md @@ -15810,7 +15810,7 @@ (define_insn "hashst" [(set (match_operand:DI 0 "simple_offsettable_mem_operand" "=m") (unspec_volatile:DI [(match_operand:DI 1 "int_reg_operand" "r")] UNSPEC_HASHST))] - "TARGET_POWER8 && rs6000_rop_protect" + "rs6000_rop_protect" { static char templ[32]; const char *p = rs6000_privileged ? "p" : ""; @@ -15823,7 +15823,7 @@ (define_insn "hashchk" [(unspec_volatile [(match_operand:DI 0 "int_reg_operand" "r") (match_operand:DI 1 "simple_offsettable_mem_operand" "m")] UNSPEC_HASHCHK)] - "TARGET_POWER8 && rs6000_rop_protect" + "rs6000_rop_protect" { static char templ[32]; const char *p = rs6000_privileged ? "p" : ""; diff --git a/gcc/testsuite/gcc.target/powerpc/pr114759-3.c b/gcc/testsuite/gcc.target/powerpc/pr114759-3.c new file mode 100644 index 00000000000..6770a9aec3b --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr114759-3.c @@ -0,0 +1,19 @@ +/* PR target/114759 */ +/* { dg-do compile } */ +/* { dg-options "-O2 -mdejagnu-cpu=power7 -mrop-protect" } */ + +/* Verify we emit an error if we use -mrop-protect with an unsupported cpu. */ + +extern void foo (void); + +int +bar (void) +{ + foo (); + return 5; +} + +/* The correct line number is in the preamble to the error message, not + in the final line (which is all that dg-error inspects). Hence, we have + to tell dg-error to ignore the line number. */ +/* { dg-error "'-mrop-protect' requires '-mcpu=power8'" "PR114759" { target *-*-* } 0 } */