| Message ID | b7a52cdf-1fcb-4bde-924a-08aee8e75755@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000: Error on CPUs and ABIs that don't support the ROP, protection insns [PR114759] | expand |
Hi Peter, on 2024/7/10 05:39, Peter Bergner wrote: > Hi Kewen, > > Here is that promised cleanup patch we discussed in the previous patch review. > I'll note this patch is dependent on the previous patch you approved. I have > not pushed that yet (in case you looked) since I'm waiting on Segher to approve > the updated patch for not disabling shrink-wrapping for leaf-functions in the > presence of -mrop-protect first. > > > 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 > for both. This matches clang behavior and allows us to simplify our ROP > tests in the code that generates our prologue and epilogue code. > > This passed bootstrap and regtesting on powerpc64le-linux and powerpc64-linux > with no regressions. Ok for trunk? > > I'll note I did not create a test case for unsupported ABIs, since I'll be > working on adding ROP support for powerpc-linux and powerpc64-linux next. > > Peter > > > > 2024-06-26 Peter Bergner <bergner@linux.ibm.com> > > gcc/ > PR target/114759 > * config/rs6000/rs6000.cc (rs6000_override_options_after_change): > 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.cc | 11 ++++++++++ > gcc/config/rs6000/rs6000-logue.cc | 22 +++++-------------- > gcc/config/rs6000/rs6000.md | 4 ++-- > gcc/testsuite/gcc.target/powerpc/pr114759-3.c | 19 ++++++++++++++++ > 4 files changed, 38 insertions(+), 18 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/powerpc/pr114759-3.c > > diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc > index fd6e013c346..e9642fd5310 100644 > --- a/gcc/config/rs6000/rs6000.cc > +++ b/gcc/config/rs6000/rs6000.cc > @@ -3427,6 +3427,17 @@ 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 (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"); > + } I wonder if there is some reason to put this hunk here. IMHO we want the hunk in rs6000_option_override_internal instead since no optimization options can affect cpu type and DEFAULT_ABI? And we probably want to unset rs6000_rop_protect to align with the handlings on other options? The others look good to me, thanks! BR, Kewen > } > > #ifdef TARGET_USES_LINUX64_OPT > 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.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 } */
On 7/10/24 1:01 AM, Kewen.Lin wrote: >> + 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"); >> + } > > I wonder if there is some reason to put this hunk here. IMHO we want the hunk > in rs6000_option_override_internal instead since no optimization options can > affect cpu type and DEFAULT_ABI? So the original code that used to disable shrink-wrapping that looked like: /* If we are inserting ROP-protect instructions, disable shrink wrap. */ if (rs6000_rop_protect) flag_shrink_wrap = 0; ...used to be in rs6000_option_override_internal, but was moved to rs6000_override_options_after_change as part of PR101324 (commit cff7879a381d). I guess I just placed this code here since it was the correct location for that old usage (it's changed in the patch I'm waiting for Segher to re-review), but I will look at moving it to rs6000_option_override_internal. I think I thought we could use a target attribute to enable -mrop-protect, but looking closer, we don't actually allow that option there. > And we probably want to unset rs6000_rop_protect to align with the handlings > on other options? I'm not sure I know what you mean? Why would we unset rs6000_rop_protect? Either we've concluded the current target options allow ROP code gen or not and for the cases where we don't/can't allow ROP, we want to give the user and error to match clang's behavior and how we already handle unsupported ABIs. So what is it you're trying to describe here? Peter
on 2024/7/11 11:36, Peter Bergner wrote: > On 7/10/24 1:01 AM, Kewen.Lin wrote: >>> + 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"); >>> + } >> >> I wonder if there is some reason to put this hunk here. IMHO we want the hunk >> in rs6000_option_override_internal instead since no optimization options can >> affect cpu type and DEFAULT_ABI? > > So the original code that used to disable shrink-wrapping that looked like: > > /* If we are inserting ROP-protect instructions, disable shrink wrap. */ > if (rs6000_rop_protect) > flag_shrink_wrap = 0; > > ...used to be in rs6000_option_override_internal, but was moved to > rs6000_override_options_after_change as part of PR101324 (commit cff7879a381d). Yeah, I noticed that, but it's for optimization option flag_shrink_wrap. If we just disable shrink-wrap in rs6000_option_override_internal, some function adopts optimize attribute to enable shrink-wrap, it would be re-enabled, which is unexpected. But target options (this patch cares about) don't have the issue, so I think function rs6000_option_override_internal is a better fit. > I guess I just placed this code here since it was the correct location for > that old usage (it's changed in the patch I'm waiting for Segher to re-review), > but I will look at moving it to rs6000_option_override_internal. I think I > thought we could use a target attribute to enable -mrop-protect, but looking > closer, we don't actually allow that option there. > > > >> And we probably want to unset rs6000_rop_protect to align with the handlings >> on other options? > > I'm not sure I know what you mean? Why would we unset rs6000_rop_protect? > Either we've concluded the current target options allow ROP code gen or not > and for the cases where we don't/can't allow ROP, we want to give the user > and error to match clang's behavior and how we already handle unsupported > ABIs. So what is it you're trying to describe here? Sorry for the confusion, I meant for most target options when we emit some error message meanwhile we also unset it, such as: if (TARGET_CRYPTO && !TARGET_ALTIVEC) { if (rs6000_isa_flags_explicit & OPTION_MASK_CRYPTO) error ("%qs requires %qs", "-mcrypto", "-maltivec"); rs6000_isa_flags &= ~OPTION_MASK_CRYPTO; } if (!TARGET_FPRND && TARGET_VSX) { if (rs6000_isa_flags_explicit & OPTION_MASK_FPRND) /* TARGET_VSX = 1 implies Power 7 and newer */ error ("%qs requires %qs", "-mvsx", "-mfprnd"); rs6000_isa_flags &= ~OPTION_MASK_FPRND; } ... if (TARGET_DFP && !TARGET_HARD_FLOAT) { if (rs6000_isa_flags_explicit & OPTION_MASK_DFP) error ("%qs requires %qs", "-mhard-dfp", "-mhard-float"); rs6000_isa_flags &= ~OPTION_MASK_DFP; } ... I guess this point here is to avoid the caught unexpected thing to propagate in the following processing like causing re-error or hit some unexpected assertion etc., although in most cases this concern won't happen, it seems not harmful to unset it (align with the others). BR, Kewen
On 7/11/24 1:24 AM, Kewen.Lin wrote: > Sorry for the confusion, I meant for most target options when we emit some error > message meanwhile we also unset it, such as: > > if (TARGET_CRYPTO && !TARGET_ALTIVEC) > { > if (rs6000_isa_flags_explicit & OPTION_MASK_CRYPTO) > error ("%qs requires %qs", "-mcrypto", "-maltivec"); > rs6000_isa_flags &= ~OPTION_MASK_CRYPTO; > } That is not what is happening here though. The code here to disable crypto is for the case where crypto is enabled implicitly, via say -mcpu=power8, but the user has also explicitly disabled Altivec which crypto depends on. In that case, we do not emit an error or warning and we silently disable crypto. This is similar to -mcpu=power8 -mno-altivec where we silently disable VSX. The other cases you showed are of similar scenarios. In my ROP code, we know ROP was explicitly enabled (it is never turned on implicitly with any -mcpu= option) and the target cpu and/or ABI does not support it, so there's nothing more to do, other than to emit an error. There is no matching implicit use case where we silently disable ROP as there was in your case above. Therefore, I think the code as I showed it is correct as is...other than the code snipit location, which I have moved and am testing now. Peter
on 2024/7/16 04:30, Peter Bergner wrote: > On 7/11/24 1:24 AM, Kewen.Lin wrote: >> Sorry for the confusion, I meant for most target options when we emit some error >> message meanwhile we also unset it, such as: >> >> if (TARGET_CRYPTO && !TARGET_ALTIVEC) >> { >> if (rs6000_isa_flags_explicit & OPTION_MASK_CRYPTO) >> error ("%qs requires %qs", "-mcrypto", "-maltivec"); >> rs6000_isa_flags &= ~OPTION_MASK_CRYPTO; >> } > > That is not what is happening here though. The code here to disable > crypto is for the case where crypto is enabled implicitly, via say > -mcpu=power8, but the user has also explicitly disabled Altivec which > crypto depends on. In that case, we do not emit an error or warning and But if it's just for the case implicit enabling, the unmask should be in an else arm and not for both implicit and explicit, the code still does unmasking for explicit enabling. Since it's very unlikely to cause some unexpected behaviors in following processing even in future, it's your call. :) BR, Kewen > we silently disable crypto. This is similar to -mcpu=power8 -mno-altivec > where we silently disable VSX. The other cases you showed are of similar > scenarios. > > In my ROP code, we know ROP was explicitly enabled (it is never turned on > implicitly with any -mcpu= option) and the target cpu and/or ABI does > not support it, so there's nothing more to do, other than to emit an error. > There is no matching implicit use case where we silently disable ROP as > there was in your case above. Therefore, I think the code as I showed it > is correct as is...other than the code snipit location, which I have moved > and am testing now. > > Peter > > >
On 7/15/24 9:19 PM, Kewen.Lin wrote: > on 2024/7/16 04:30, Peter Bergner wrote: >> On 7/11/24 1:24 AM, Kewen.Lin wrote: >>> Sorry for the confusion, I meant for most target options when we emit some error >>> message meanwhile we also unset it, such as: >>> >>> if (TARGET_CRYPTO && !TARGET_ALTIVEC) >>> { >>> if (rs6000_isa_flags_explicit & OPTION_MASK_CRYPTO) >>> error ("%qs requires %qs", "-mcrypto", "-maltivec"); >>> rs6000_isa_flags &= ~OPTION_MASK_CRYPTO; >>> } >> >> That is not what is happening here though. The code here to disable >> crypto is for the case where crypto is enabled implicitly, via say >> -mcpu=power8, but the user has also explicitly disabled Altivec which >> crypto depends on. In that case, we do not emit an error or warning and > > But if it's just for the case implicit enabling, the unmask should be > in an else arm and not for both implicit and explicit, the code still does > unmasking for explicit enabling. Since it's very unlikely to cause some > unexpected behaviors in following processing even in future, it's your call. :) I think that is just Mike's coding style. Yes, the clearing of the flag could be in an else block, but clearing it even in the error case is harmless, since we're going to emit an error and exit anyway. Peter
diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc index fd6e013c346..e9642fd5310 100644 --- a/gcc/config/rs6000/rs6000.cc +++ b/gcc/config/rs6000/rs6000.cc @@ -3427,6 +3427,17 @@ 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 (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"); + } } #ifdef TARGET_USES_LINUX64_OPT 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.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 } */