On Tue, Feb 27, 2024 at 9:42 AM Jakub Jelinek <ja...@redhat.com> wrote:
>
> Hi!
>
> 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).
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Just to add we've in the past opted to avoid tail-calling abort () and friends
exactly to help debugging. So treating noreturn functions specially with
respect to caller saves looks inconsistent in this regard - it makes debugging
harder since a lot of info in the calling frame is going to be lost?
I hope we at least avoid that at -O0, possibly also with -Og?
> 2024-02-27 Jakub Jelinek <ja...@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.
>
> --- gcc/config/i386/i386.h.jj 2024-01-27 22:59:53.121319813 +0100
> +++ gcc/config/i386/i386.h 2024-02-26 16:33:14.644745362 +0100
> @@ -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
> --- gcc/config/i386/i386-options.cc.jj 2024-02-26 16:29:07.471201524 +0100
> +++ gcc/config/i386/i386-options.cc 2024-02-26 16:42:01.730358609 +0100
> @@ -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;
> }
> }
> }
> --- gcc/config/i386/i386.cc.jj 2024-02-26 11:12:36.275453513 +0100
> +++ gcc/config/i386/i386.cc 2024-02-26 17:24:50.701602352 +0100
> @@ -984,8 +984,9 @@ ix86_function_ok_for_sibcall (tree decl,
>
> /* 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
>
> 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
> --- gcc/testsuite/gcc.target/i386/pr38534-1.c.jj 2024-02-01
> 16:00:37.443735635 +0100
> +++ gcc/testsuite/gcc.target/i386/pr38534-1.c 2024-02-27 08:59:08.421878166
> +0100
> @@ -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)" } } */
> --- gcc/testsuite/gcc.target/i386/pr38534-4.c.jj 2024-02-01
> 16:00:37.443735635 +0100
> +++ gcc/testsuite/gcc.target/i386/pr38534-4.c 2024-02-27 09:00:09.869021464
> +0100
> @@ -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 \]+" } } */
> --- gcc/testsuite/gcc.target/i386/pr38534-2.c.jj 2024-02-01
> 16:00:37.443735635 +0100
> +++ gcc/testsuite/gcc.target/i386/pr38534-2.c 2024-02-27 08:59:35.723497526
> +0100
> @@ -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" } } */
> --- gcc/testsuite/gcc.target/i386/pr38534-3.c.jj 2024-02-01
> 16:00:37.443735635 +0100
> +++ gcc/testsuite/gcc.target/i386/pr38534-3.c 2024-02-27 08:59:49.669303090
> +0100
> @@ -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 \]+" } } */
> --- gcc/testsuite/gcc.target/i386/pr114097-1.c.jj 2024-02-26
> 16:29:07.529200714 +0100
> +++ gcc/testsuite/gcc.target/i386/pr114097-1.c 2024-02-27 09:01:37.876794453
> +0100
> @@ -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)" } } */
> --- gcc/testsuite/gcc.target/i386/stack-check-17.c.jj 2024-01-27
> 22:59:53.186318915 +0100
> +++ gcc/testsuite/gcc.target/i386/stack-check-17.c 2024-02-27
> 09:06:10.805989259 +0100
> @@ -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 } } } */
>
> Jakub
>
