public inbox for gcc-cvs@sourceware.org
help / color / mirror / Atom feed
* [gcc r14-9313] i386: For noreturn functions save at least the bp register if it is used [PR114116]
@ 2024-03-05 9:26 Jakub Jelinek
0 siblings, 0 replies; only message in thread
From: Jakub Jelinek @ 2024-03-05 9:26 UTC (permalink / raw)
To: gcc-cvs
https://gcc.gnu.org/g:8ee6d13e32279faf9ef4fd8eabfba0adfca0dfb9
commit r14-9313-g8ee6d13e32279faf9ef4fd8eabfba0adfca0dfb9
Author: Jakub Jelinek <jakub@redhat.com>
Date: Tue Mar 5 10:24:51 2024 +0100
i386: For noreturn functions save at least the bp register if it is used [PR114116]
As mentioned in the PR, on x86_64 currently a lot of ICEs end up
with crashes in the unwinder like:
during RTL pass: expand
pr114044-2.c: In function ‘foo’:
pr114044-2.c:5:3: internal compiler error: in expand_fn_using_insn, at internal-fn.cc:208
5 | __builtin_clzg (a);
| ^~~~~~~~~~~~~~~~~~
0x7d9246 expand_fn_using_insn
../../gcc/internal-fn.cc:208
pr114044-2.c:5:3: internal compiler error: Segmentation fault
0x1554262 crash_signal
../../gcc/toplev.cc:319
0x2b20320 x86_64_fallback_frame_state
./md-unwind-support.h:63
0x2b20320 uw_frame_state_for
../../../libgcc/unwind-dw2.c:1013
0x2b2165d _Unwind_Backtrace
../../../libgcc/unwind.inc:303
0x2acbd69 backtrace_full
../../libbacktrace/backtrace.c:127
0x2a32fa6 diagnostic_context::action_after_output(diagnostic_t)
../../gcc/diagnostic.cc:781
0x2a331bb diagnostic_action_after_output(diagnostic_context*, diagnostic_t)
../../gcc/diagnostic.h:1002
0x2a331bb diagnostic_context::report_diagnostic(diagnostic_info*)
../../gcc/diagnostic.cc:1633
0x2a33543 diagnostic_impl
../../gcc/diagnostic.cc:1767
0x2a33c26 internal_error(char const*, ...)
../../gcc/diagnostic.cc:2225
0xe232c8 fancy_abort(char const*, int, char const*)
../../gcc/diagnostic.cc:2336
0x7d9246 expand_fn_using_insn
../../gcc/internal-fn.cc:208
Segmentation fault (core dumped)
The problem are the PR38534 r14-8470 changes which avoid saving call-saved
registers in noreturn functions. If such functions ever touch the
bp register but because of the r14-8470 changes don't save it in the
prologue, the caller or any other function in the backtrace uses a frame
pointer and the noreturn function or anything it calls directly or
indirectly calls backtrace, then the unwinder crashes, because bp register
contains some unrelated value, but in the frames which do use frame pointer
CFA is based on the bp register.
In theory this could happen with any other call-saved register, e.g. code
written by hand in assembly with .cfi_* directives could use any other
call-saved register as register into which store the CFA or something
related to that, but in reality at least compiler generated code and usual
assembly probably just making sure bp doesn't contain garbage could be
enough for backtrace purposes. In the debugger of course it will not be
enough, the values of the arguments etc. can be lost (if DW_CFA_undefined
is emitted) or garbage.
So, I think for noreturn function we should at least save the bp register
if we use it. If user asks for it using no_callee_saved_registers
attribute, let's honor what is asked for (but then it is up to the user
to make sure e.g. backtrace isn't called from the function or anything it
calls). As discussed in the PR, whether to save bp or not shouldn't be
based on whether compiling with -g or -g0, because we don't want code
generation changes without/with debugging, it would also break
-fcompare-debug, and users can call backtrace(3), that doesn't use debug
info, just unwind info, even backtrace_symbols{,_fd}(3) don't use debug info
but just looks at dynamic symbol table.
The patch also adds check for no_caller_saved_registers
attribute in the implicit addition of not saving callee saved register
in noreturn functions, because on I think
__attribute__((no_caller_saved_registers, noreturn)) will otherwise
error that no_caller_saved_registers and no_callee_saved_registers
attributes are incompatible (but user didn't specify anything like that).
2024-03-05 Jakub Jelinek <jakub@redhat.com>
PR target/114116
* config/i386/i386.h (enum call_saved_registers_type): Add
TYPE_NO_CALLEE_SAVED_REGISTERS_EXCEPT_BP enumerator.
* config/i386/i386-options.cc (ix86_set_func_type): Remove
has_no_callee_saved_registers variable, add no_callee_saved_registers
instead, initialize it depending on whether it is
no_callee_saved_registers function or not. Don't set it if
no_caller_saved_registers attribute is present. Adjust users.
* config/i386/i386.cc (ix86_function_ok_for_sibcall): Handle
TYPE_NO_CALLEE_SAVED_REGISTERS_EXCEPT_BP like
TYPE_NO_CALLEE_SAVED_REGISTERS.
(ix86_save_reg): Handle TYPE_NO_CALLEE_SAVED_REGISTERS_EXCEPT_BP.
* gcc.target/i386/pr38534-1.c: Allow push/pop of bp.
* gcc.target/i386/pr38534-4.c: Likewise.
* gcc.target/i386/pr38534-2.c: Likewise.
* gcc.target/i386/pr38534-3.c: Likewise.
* gcc.target/i386/pr114097-1.c: Likewise.
* gcc.target/i386/stack-check-17.c: Expect no pop on ! ia32.
Diff:
---
gcc/config/i386/i386-options.cc | 33 ++++++++++++++++----------
gcc/config/i386/i386.cc | 10 ++++++--
gcc/config/i386/i386.h | 7 ++++--
gcc/testsuite/gcc.target/i386/pr114097-1.c | 4 ++--
gcc/testsuite/gcc.target/i386/pr38534-1.c | 4 ++--
gcc/testsuite/gcc.target/i386/pr38534-2.c | 4 ++--
gcc/testsuite/gcc.target/i386/pr38534-3.c | 4 ++--
gcc/testsuite/gcc.target/i386/pr38534-4.c | 4 ++--
gcc/testsuite/gcc.target/i386/stack-check-17.c | 4 ++--
9 files changed, 45 insertions(+), 29 deletions(-)
diff --git a/gcc/config/i386/i386-options.cc b/gcc/config/i386/i386-options.cc
index 1301f6b913e..2f8c85f66d4 100644
--- a/gcc/config/i386/i386-options.cc
+++ b/gcc/config/i386/i386-options.cc
@@ -3384,7 +3384,9 @@ ix86_set_func_type (tree fndecl)
{
/* No need to save and restore callee-saved registers for a noreturn
function with nothrow or compiled with -fno-exceptions unless when
- compiling with -O0 or -Og.
+ compiling with -O0 or -Og. So that backtrace works for those at least
+ in most cases, save the bp register if it is used, because it often
+ is used in callers to compute CFA.
NB: Can't use just TREE_THIS_VOLATILE to check if this is a noreturn
function. The local-pure-const pass turns an interrupt function
@@ -3394,15 +3396,20 @@ ix86_set_func_type (tree fndecl)
function is marked with TREE_THIS_VOLATILE in the IR output, which
leads to the incompatible attribute error in LTO1. Ignore the
interrupt function in this case. */
- bool has_no_callee_saved_registers
- = ((TREE_THIS_VOLATILE (fndecl)
- && !lookup_attribute ("interrupt",
- TYPE_ATTRIBUTES (TREE_TYPE (fndecl)))
- && optimize
- && !optimize_debug
- && (TREE_NOTHROW (fndecl) || !flag_exceptions))
- || lookup_attribute ("no_callee_saved_registers",
- TYPE_ATTRIBUTES (TREE_TYPE (fndecl))));
+ enum call_saved_registers_type no_callee_saved_registers
+ = TYPE_DEFAULT_CALL_SAVED_REGISTERS;
+ if (lookup_attribute ("no_callee_saved_registers",
+ TYPE_ATTRIBUTES (TREE_TYPE (fndecl))))
+ no_callee_saved_registers = TYPE_NO_CALLEE_SAVED_REGISTERS;
+ else if (TREE_THIS_VOLATILE (fndecl)
+ && optimize
+ && !optimize_debug
+ && (TREE_NOTHROW (fndecl) || !flag_exceptions)
+ && !lookup_attribute ("interrupt",
+ TYPE_ATTRIBUTES (TREE_TYPE (fndecl)))
+ && !lookup_attribute ("no_caller_saved_registers",
+ TYPE_ATTRIBUTES (TREE_TYPE (fndecl))))
+ no_callee_saved_registers = TYPE_NO_CALLEE_SAVED_REGISTERS_EXCEPT_BP;
if (cfun->machine->func_type == TYPE_UNKNOWN)
{
@@ -3413,7 +3420,7 @@ ix86_set_func_type (tree fndecl)
error_at (DECL_SOURCE_LOCATION (fndecl),
"interrupt and naked attributes are not compatible");
- if (has_no_callee_saved_registers)
+ if (no_callee_saved_registers)
error_at (DECL_SOURCE_LOCATION (fndecl),
"%qs and %qs attributes are not compatible",
"interrupt", "no_callee_saved_registers");
@@ -3442,7 +3449,7 @@ ix86_set_func_type (tree fndecl)
TYPE_ATTRIBUTES (TREE_TYPE (fndecl))))
cfun->machine->call_saved_registers
= TYPE_NO_CALLER_SAVED_REGISTERS;
- if (has_no_callee_saved_registers)
+ if (no_callee_saved_registers)
{
if (cfun->machine->call_saved_registers
== TYPE_NO_CALLER_SAVED_REGISTERS)
@@ -3451,7 +3458,7 @@ ix86_set_func_type (tree fndecl)
"no_caller_saved_registers",
"no_callee_saved_registers");
cfun->machine->call_saved_registers
- = TYPE_NO_CALLEE_SAVED_REGISTERS;
+ = no_callee_saved_registers;
}
}
}
diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
index fc5068539c1..4b6b665e599 100644
--- a/gcc/config/i386/i386.cc
+++ b/gcc/config/i386/i386.cc
@@ -984,8 +984,9 @@ ix86_function_ok_for_sibcall (tree decl, tree exp)
/* Sibling call isn't OK if callee has no callee-saved registers
and the calling function has callee-saved registers. */
- if ((cfun->machine->call_saved_registers
- != TYPE_NO_CALLEE_SAVED_REGISTERS)
+ if (cfun->machine->call_saved_registers != TYPE_NO_CALLEE_SAVED_REGISTERS
+ && (cfun->machine->call_saved_registers
+ != TYPE_NO_CALLEE_SAVED_REGISTERS_EXCEPT_BP)
&& lookup_attribute ("no_callee_saved_registers",
TYPE_ATTRIBUTES (type)))
return false;
@@ -6649,6 +6650,11 @@ ix86_save_reg (unsigned int regno, bool maybe_eh_return, bool ignore_outlined)
case TYPE_NO_CALLEE_SAVED_REGISTERS:
return false;
+
+ case TYPE_NO_CALLEE_SAVED_REGISTERS_EXCEPT_BP:
+ if (regno != HARD_FRAME_POINTER_REGNUM)
+ return false;
+ break;
}
if (regno == REAL_PIC_OFFSET_TABLE_REGNUM
diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h
index 35ce8b00d36..efd46a14313 100644
--- a/gcc/config/i386/i386.h
+++ b/gcc/config/i386/i386.h
@@ -2730,9 +2730,12 @@ enum call_saved_registers_type
/* The current function is a function specified with the "interrupt"
or "no_caller_saved_registers" attribute. */
TYPE_NO_CALLER_SAVED_REGISTERS,
+ /* The current function is a function specified with the
+ "no_callee_saved_registers" attribute. */
+ TYPE_NO_CALLEE_SAVED_REGISTERS,
/* The current function is a function specified with the "noreturn"
- or "no_callee_saved_registers" attribute. */
- TYPE_NO_CALLEE_SAVED_REGISTERS
+ attribute. */
+ TYPE_NO_CALLEE_SAVED_REGISTERS_EXCEPT_BP,
};
enum queued_insn_type
diff --git a/gcc/testsuite/gcc.target/i386/pr114097-1.c b/gcc/testsuite/gcc.target/i386/pr114097-1.c
index b14c7b6214d..feeb9165570 100644
--- a/gcc/testsuite/gcc.target/i386/pr114097-1.c
+++ b/gcc/testsuite/gcc.target/i386/pr114097-1.c
@@ -22,5 +22,5 @@ no_return_to_caller (void)
while (1);
}
-/* { dg-final { scan-assembler-not "push" } } */
-/* { dg-final { scan-assembler-not "pop" } } */
+/* { dg-final { scan-assembler-not "push\[^\n\r\]*(?:\[abcd\]x|\[sd\]i|sp|r\[0-9\]|\[xyz\]mm)" } } */
+/* { dg-final { scan-assembler-not "pop\[^\n\r\]*(?:\[abcd\]x|\[sd\]i|sp|r\[0-9\]|\[xyz\]mm)" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr38534-1.c b/gcc/testsuite/gcc.target/i386/pr38534-1.c
index 280f3b440a2..c73c8d23288 100644
--- a/gcc/testsuite/gcc.target/i386/pr38534-1.c
+++ b/gcc/testsuite/gcc.target/i386/pr38534-1.c
@@ -22,5 +22,5 @@ no_return_to_caller (void)
while (1);
}
-/* { dg-final { scan-assembler-not "push" } } */
-/* { dg-final { scan-assembler-not "pop" } } */
+/* { dg-final { scan-assembler-not "push\[^\n\r\]*(?:\[abcd\]x|\[sd\]i|sp|r\[0-9\]|\[xyz\]mm)" } } */
+/* { dg-final { scan-assembler-not "pop\[^\n\r\]*(?:\[abcd\]x|\[sd\]i|sp|r\[0-9\]|\[xyz\]mm)" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr38534-2.c b/gcc/testsuite/gcc.target/i386/pr38534-2.c
index 2e199896514..0dc8720dc89 100644
--- a/gcc/testsuite/gcc.target/i386/pr38534-2.c
+++ b/gcc/testsuite/gcc.target/i386/pr38534-2.c
@@ -12,7 +12,7 @@ foo (void)
fn ();
}
-/* { dg-final { scan-assembler-not "push" } } */
-/* { dg-final { scan-assembler-not "pop" } } */
+/* { dg-final { scan-assembler-not "push\[^\n\r\]*(?:\[abcd\]x|\[sd\]i|sp|r\[0-9\]|\[xyz\]mm)" } } */
+/* { dg-final { scan-assembler-not "pop\[^\n\r\]*(?:\[abcd\]x|\[sd\]i|sp|r\[0-9\]|\[xyz\]mm)" } } */
/* { dg-final { scan-assembler-not "jmp\[\\t \]+_?bar" } } */
/* { dg-final { scan-assembler "call\[\\t \]+_?bar" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr38534-3.c b/gcc/testsuite/gcc.target/i386/pr38534-3.c
index af6e1955f41..554c594feb7 100644
--- a/gcc/testsuite/gcc.target/i386/pr38534-3.c
+++ b/gcc/testsuite/gcc.target/i386/pr38534-3.c
@@ -13,7 +13,7 @@ foo (void)
fn ();
}
-/* { dg-final { scan-assembler-not "push" } } */
-/* { dg-final { scan-assembler-not "pop" } } */
+/* { dg-final { scan-assembler-not "push\[^\n\r\]*(?:\[abcd\]x|\[sd\]i|sp|r\[0-9\]|\[xyz\]mm)" } } */
+/* { dg-final { scan-assembler-not "pop\[^\n\r\]*(?:\[abcd\]x|\[sd\]i|sp|r\[0-9\]|\[xyz\]mm)" } } */
/* { dg-final { scan-assembler-not "jmp" } } */
/* { dg-final { scan-assembler "call\[\\t \]+" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr38534-4.c b/gcc/testsuite/gcc.target/i386/pr38534-4.c
index b2047893e89..8073aac01fc 100644
--- a/gcc/testsuite/gcc.target/i386/pr38534-4.c
+++ b/gcc/testsuite/gcc.target/i386/pr38534-4.c
@@ -12,7 +12,7 @@ foo (fn_t bar)
fn ();
}
-/* { dg-final { scan-assembler-not "push" } } */
-/* { dg-final { scan-assembler-not "pop" } } */
+/* { dg-final { scan-assembler-not "push\[^\n\r\]*(?:\[abcd\]x|\[sd\]i|sp|r\[0-9\]|\[xyz\]mm)" } } */
+/* { dg-final { scan-assembler-not "pop\[^\n\r\]*(?:\[abcd\]x|\[sd\]i|sp|r\[0-9\]|\[xyz\]mm)" } } */
/* { dg-final { scan-assembler-not "jmp" } } */
/* { dg-final { scan-assembler "call\[\\t \]+" } } */
diff --git a/gcc/testsuite/gcc.target/i386/stack-check-17.c b/gcc/testsuite/gcc.target/i386/stack-check-17.c
index 061484e1319..648572e5ebd 100644
--- a/gcc/testsuite/gcc.target/i386/stack-check-17.c
+++ b/gcc/testsuite/gcc.target/i386/stack-check-17.c
@@ -32,5 +32,5 @@ f3 (void)
register on ia32 for a noreturn function. */
/* { dg-final { scan-assembler-times "push\[ql\]" 1 { target { ! ia32 } } } } */
/* { dg-final { scan-assembler-times "push\[ql\]" 3 { target ia32 } } } */
-/* { dg-final { scan-assembler-times "pop" 1 } } */
-
+/* { dg-final { scan-assembler-not "pop" { target { ! ia32 } } } } */
+/* { dg-final { scan-assembler-times "pop" 1 { target ia32 } } } */
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2024-03-05 9:26 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-05 9:26 [gcc r14-9313] i386: For noreturn functions save at least the bp register if it is used [PR114116] Jakub Jelinek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).