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 <[email protected]>
>
> 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 } */
