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 <berg...@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");
+ }
}
#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 } */
--
2.43.5
