dwarf2unwind : Force the CFA after remember/restore pairs [44107/48097].

Message ID A7866750-DC99-4434-B10C-FCC41EC439B3@sandoe.co.uk
State New
Headers show
Series
  • dwarf2unwind : Force the CFA after remember/restore pairs [44107/48097].
Related show

Commit Message

Iain Sandoe Feb. 26, 2021, 9:21 p.m.
Hi

This address one of the more long-standing and serious regressions
for Darwin.  GCC emits unwind code by default on the assumption that
the unwinder will be (or have the same capability) as the one in the
current libgcc_s.  For Darwin platforms, this is not the case - some
of them are based on the libgcc_s from GCC-4.2.1 and some are using
the unwinder provided by libunwind (part of the LLVM project).  The
latter implementation has gradually adopted a section that deals with
GNU unwind.

The most serious problem for some of the platform versions is in
handling DW_CFA_remember/restore_state pairs.  The DWARF description
talks about these in terms of saving/restoring register rows; this is
what GCC originally did (and is what the unwinders do for the Darwin
versions based on libgcc_s).

However, in r118068, this was changed so that not only the registers
but also the current frame address expression were saved.  The unwind
code-gen assumes that the unwinder will do this; some of Darwin's unwinders
do not, leading to lockups etc.  To date, the only solution has been to replace
the system libgcc_s with a newer one which is not a usable solution for many
end-users (since that means overwritting the one provided with the system
installation).

The fix here provides a target hook that allows the target to specify
that the CFA expression should be reinstated after a DW_CFA_restore.  This
fixes the open PR (and also the closed WONTFIX of 44107).

(As a matter of record, it also fixes reported Java issues if
 backported to GCC-5).

tested on *-darwin* and x86_64-linux-gnu,
OK for master / backports to open branches?
thanks
Iain

gcc/ChangeLog:

	PR target/44107
	PR target/48097
	* config/darwin-protos.h (darwin_should_restore_cfa_state): New.
	* config/darwin.c (darwin_should_restore_cfa_state): New.
	* config/darwin.h (TARGET_ASM_SHOULD_RESTORE_CFA_STATE): New.
	* doc/tm.texi: Regenerated.
	* doc/tm.texi.in: Document TARGET_ASM_SHOULD_RESTORE_CFA_STATE.
	* dwarf2cfi.c (connect_traces): If the target requests, restore
	the CFA expression after a DW_CFA_restore.
	* target.def (TARGET_ASM_SHOULD_RESTORE_CFA_STATE): New hook.
---
 gcc/config/darwin-protos.h |  1 +
 gcc/config/darwin.c        | 10 ++++++++++
 gcc/config/darwin.h        |  5 +++++
 gcc/doc/tm.texi            |  4 ++++
 gcc/doc/tm.texi.in         |  2 ++
 gcc/dwarf2cfi.c            |  6 ++++++
 gcc/target.def             | 14 ++++++++++++++
 7 files changed, 42 insertions(+)

-- 
2.24.1

Comments

Richard Biener via Gcc-patches March 1, 2021, 8:35 a.m. | #1
On Fri, Feb 26, 2021 at 11:04 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
>

> Hi

>

> This address one of the more long-standing and serious regressions

> for Darwin.  GCC emits unwind code by default on the assumption that

> the unwinder will be (or have the same capability) as the one in the

> current libgcc_s.  For Darwin platforms, this is not the case - some

> of them are based on the libgcc_s from GCC-4.2.1 and some are using

> the unwinder provided by libunwind (part of the LLVM project).  The

> latter implementation has gradually adopted a section that deals with

> GNU unwind.

>

> The most serious problem for some of the platform versions is in

> handling DW_CFA_remember/restore_state pairs.  The DWARF description

> talks about these in terms of saving/restoring register rows; this is

> what GCC originally did (and is what the unwinders do for the Darwin

> versions based on libgcc_s).

>

> However, in r118068, this was changed so that not only the registers

> but also the current frame address expression were saved.  The unwind

> code-gen assumes that the unwinder will do this; some of Darwin's unwinders

> do not, leading to lockups etc.  To date, the only solution has been to replace

> the system libgcc_s with a newer one which is not a usable solution for many

> end-users (since that means overwritting the one provided with the system

> installation).

>

> The fix here provides a target hook that allows the target to specify

> that the CFA expression should be reinstated after a DW_CFA_restore.  This

> fixes the open PR (and also the closed WONTFIX of 44107).

>

> (As a matter of record, it also fixes reported Java issues if

>  backported to GCC-5).

>

> tested on *-darwin* and x86_64-linux-gnu,

> OK for master / backports to open branches?


OK for master and branches after a while.

Thanks,
Richard.

> thanks

> Iain

>

> gcc/ChangeLog:

>

>         PR target/44107

>         PR target/48097

>         * config/darwin-protos.h (darwin_should_restore_cfa_state): New.

>         * config/darwin.c (darwin_should_restore_cfa_state): New.

>         * config/darwin.h (TARGET_ASM_SHOULD_RESTORE_CFA_STATE): New.

>         * doc/tm.texi: Regenerated.

>         * doc/tm.texi.in: Document TARGET_ASM_SHOULD_RESTORE_CFA_STATE.

>         * dwarf2cfi.c (connect_traces): If the target requests, restore

>         the CFA expression after a DW_CFA_restore.

>         * target.def (TARGET_ASM_SHOULD_RESTORE_CFA_STATE): New hook.

> ---

>  gcc/config/darwin-protos.h |  1 +

>  gcc/config/darwin.c        | 10 ++++++++++

>  gcc/config/darwin.h        |  5 +++++

>  gcc/doc/tm.texi            |  4 ++++

>  gcc/doc/tm.texi.in         |  2 ++

>  gcc/dwarf2cfi.c            |  6 ++++++

>  gcc/target.def             | 14 ++++++++++++++

>  7 files changed, 42 insertions(+)

>

> diff --git a/gcc/config/darwin-protos.h b/gcc/config/darwin-protos.h

> index 2120eb62c56..f5ef82456aa 100644

> --- a/gcc/config/darwin-protos.h

> +++ b/gcc/config/darwin-protos.h

> @@ -70,6 +70,7 @@ extern void darwin_non_lazy_pcrel (FILE *, rtx);

>  extern void darwin_emit_unwind_label (FILE *, tree, int, int);

>  extern void darwin_emit_except_table_label (FILE *);

>  extern rtx darwin_make_eh_symbol_indirect (rtx, bool);

> +extern bool darwin_should_restore_cfa_state (void);

>

>  extern void darwin_pragma_ignore (struct cpp_reader *);

>  extern void darwin_pragma_options (struct cpp_reader *);

> diff --git a/gcc/config/darwin.c b/gcc/config/darwin.c

> index 119f3195b63..e2e60bbf1b2 100644

> --- a/gcc/config/darwin.c

> +++ b/gcc/config/darwin.c

> @@ -2236,6 +2236,16 @@ darwin_make_eh_symbol_indirect (rtx orig, bool ARG_UNUSED (pubvis))

>                                                         /*stub_p=*/false));

>  }

>

> +/* The unwinders in earlier Darwin versions are based on an old version

> +   of libgcc_s and need current frame address stateto be reset after a

> +   DW_CFA_restore_state recovers the register values.  */

> +

> +bool

> +darwin_should_restore_cfa_state (void)

> +{

> +  return generating_for_darwin_version <= 10;

> +}

> +

>  /* Return, and mark as used, the name of the stub for the mcount function.

>     Currently, this is only called by X86 code in the expansion of the

>     FUNCTION_PROFILER macro, when stubs are enabled.  */

> diff --git a/gcc/config/darwin.h b/gcc/config/darwin.h

> index 5a9fb43f3c6..d2b2c141c8e 100644

> --- a/gcc/config/darwin.h

> +++ b/gcc/config/darwin.h

> @@ -614,6 +614,11 @@ extern GTY(()) int darwin_ms_struct;

>  /* Make an EH (personality or LDSA) symbol indirect as needed.  */

>  #define TARGET_ASM_MAKE_EH_SYMBOL_INDIRECT darwin_make_eh_symbol_indirect

>

> +/* Some of Darwin's unwinders need current frame address state to be reset

> +   after a DW_CFA_restore_state recovers the register values.  */

> +#undef TARGET_ASM_SHOULD_RESTORE_CFA_STATE

> +#define TARGET_ASM_SHOULD_RESTORE_CFA_STATE darwin_should_restore_cfa_state

> +

>  /* Our profiling scheme doesn't LP labels and counter words.  */

>

>  #define NO_PROFILE_COUNTERS    1

> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi

> index 062785af1e2..a25ca6c78b5 100644

> --- a/gcc/doc/tm.texi

> +++ b/gcc/doc/tm.texi

> @@ -9551,6 +9551,10 @@ If necessary, modify personality and LSDA references to handle indirection.  The

>  True if the @code{TARGET_ASM_UNWIND_EMIT} hook should be called before the assembly for @var{insn} has been emitted, false if the hook should be called afterward.

>  @end deftypevr

>

> +@deftypefn {Target Hook} bool TARGET_ASM_SHOULD_RESTORE_CFA_STATE (void)

> +For DWARF-based unwind frames, two CFI instructions provide for save and restore of register state.  GCC maintains the current frame address (CFA) separately from the register bank but the unwinder in libgcc preserves this state along with the registers (and this is expected by the code that writes the unwind frames).  This hook allows the target to specify that the CFA data is not saved/restored along with the registers by the target unwinder so that suitable additional instructions should be emitted to restore it.

> +@end deftypefn

> +

>  @node Exception Region Output

>  @subsection Assembler Commands for Exception Regions

>

> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in

> index 3b19e6f4281..bf724dc093c 100644

> --- a/gcc/doc/tm.texi.in

> +++ b/gcc/doc/tm.texi.in

> @@ -6462,6 +6462,8 @@ the jump-table.

>

>  @hook TARGET_ASM_UNWIND_EMIT_BEFORE_INSN

>

> +@hook TARGET_ASM_SHOULD_RESTORE_CFA_STATE

> +

>  @node Exception Region Output

>  @subsection Assembler Commands for Exception Regions

>

> diff --git a/gcc/dwarf2cfi.c b/gcc/dwarf2cfi.c

> index c2533b091b2..2fa9f325360 100644

> --- a/gcc/dwarf2cfi.c

> +++ b/gcc/dwarf2cfi.c

> @@ -2848,6 +2848,12 @@ connect_traces (void)

>               cfi->dw_cfi_opc = DW_CFA_restore_state;

>               add_cfi (cfi);

>

> +             /* If the target unwinder does not save the CFA as part of the

> +                register state, we need to restore it separately.  */

> +             if (targetm.asm_out.should_restore_cfa_state ()

> +                 && (cfi = def_cfa_0 (&old_row->cfa, &ti->beg_row->cfa)))

> +               add_cfi (cfi);

> +

>               old_row = prev_ti->beg_row;

>             }

>           /* Otherwise, we'll simply change state from the previous end.  */

> diff --git a/gcc/target.def b/gcc/target.def

> index be7fcde961a..ab00657330a 100644

> --- a/gcc/target.def

> +++ b/gcc/target.def

> @@ -211,6 +211,20 @@ DEFHOOKPOD

>   be called afterward.",

>   bool, true)

>

> +/* Return true if the target needs extra instructions to restore the current

> +   frame address after a DW_CFA_restore_state opcode.  */

> +DEFHOOK

> +(should_restore_cfa_state,

> + "For DWARF-based unwind frames, two CFI instructions provide for save and\

> + restore of register state.  GCC maintains the current frame address (CFA)\

> + separately from the register bank but the unwinder in libgcc preserves this\

> + state along with the registers (and this is expected by the code that writes\

> + the unwind frames).  This hook allows the target to specify that the CFA data\

> + is not saved/restored along with the registers by the target unwinder so that\

> + suitable additional instructions should be emitted to restore it.",

> + bool, (void),

> + hook_bool_void_false)

> +

>  /* Generate an internal label.

>     For now this is just a wrapper for ASM_GENERATE_INTERNAL_LABEL.  */

>  DEFHOOK_UNDOC

> --

> 2.24.1

>

>

Patch

diff --git a/gcc/config/darwin-protos.h b/gcc/config/darwin-protos.h
index 2120eb62c56..f5ef82456aa 100644
--- a/gcc/config/darwin-protos.h
+++ b/gcc/config/darwin-protos.h
@@ -70,6 +70,7 @@  extern void darwin_non_lazy_pcrel (FILE *, rtx);
 extern void darwin_emit_unwind_label (FILE *, tree, int, int);
 extern void darwin_emit_except_table_label (FILE *);
 extern rtx darwin_make_eh_symbol_indirect (rtx, bool);
+extern bool darwin_should_restore_cfa_state (void);
 
 extern void darwin_pragma_ignore (struct cpp_reader *);
 extern void darwin_pragma_options (struct cpp_reader *);
diff --git a/gcc/config/darwin.c b/gcc/config/darwin.c
index 119f3195b63..e2e60bbf1b2 100644
--- a/gcc/config/darwin.c
+++ b/gcc/config/darwin.c
@@ -2236,6 +2236,16 @@  darwin_make_eh_symbol_indirect (rtx orig, bool ARG_UNUSED (pubvis))
 							/*stub_p=*/false));
 }
 
+/* The unwinders in earlier Darwin versions are based on an old version
+   of libgcc_s and need current frame address stateto be reset after a
+   DW_CFA_restore_state recovers the register values.  */
+
+bool
+darwin_should_restore_cfa_state (void)
+{
+  return generating_for_darwin_version <= 10;
+}
+
 /* Return, and mark as used, the name of the stub for the mcount function.
    Currently, this is only called by X86 code in the expansion of the
    FUNCTION_PROFILER macro, when stubs are enabled.  */
diff --git a/gcc/config/darwin.h b/gcc/config/darwin.h
index 5a9fb43f3c6..d2b2c141c8e 100644
--- a/gcc/config/darwin.h
+++ b/gcc/config/darwin.h
@@ -614,6 +614,11 @@  extern GTY(()) int darwin_ms_struct;
 /* Make an EH (personality or LDSA) symbol indirect as needed.  */
 #define TARGET_ASM_MAKE_EH_SYMBOL_INDIRECT darwin_make_eh_symbol_indirect
 
+/* Some of Darwin's unwinders need current frame address state to be reset
+   after a DW_CFA_restore_state recovers the register values.  */
+#undef TARGET_ASM_SHOULD_RESTORE_CFA_STATE
+#define TARGET_ASM_SHOULD_RESTORE_CFA_STATE darwin_should_restore_cfa_state
+
 /* Our profiling scheme doesn't LP labels and counter words.  */
 
 #define NO_PROFILE_COUNTERS	1
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 062785af1e2..a25ca6c78b5 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -9551,6 +9551,10 @@  If necessary, modify personality and LSDA references to handle indirection.  The
 True if the @code{TARGET_ASM_UNWIND_EMIT} hook should be called before the assembly for @var{insn} has been emitted, false if the hook should be called afterward.
 @end deftypevr
 
+@deftypefn {Target Hook} bool TARGET_ASM_SHOULD_RESTORE_CFA_STATE (void)
+For DWARF-based unwind frames, two CFI instructions provide for save and restore of register state.  GCC maintains the current frame address (CFA) separately from the register bank but the unwinder in libgcc preserves this state along with the registers (and this is expected by the code that writes the unwind frames).  This hook allows the target to specify that the CFA data is not saved/restored along with the registers by the target unwinder so that suitable additional instructions should be emitted to restore it.
+@end deftypefn
+
 @node Exception Region Output
 @subsection Assembler Commands for Exception Regions
 
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 3b19e6f4281..bf724dc093c 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -6462,6 +6462,8 @@  the jump-table.
 
 @hook TARGET_ASM_UNWIND_EMIT_BEFORE_INSN
 
+@hook TARGET_ASM_SHOULD_RESTORE_CFA_STATE
+
 @node Exception Region Output
 @subsection Assembler Commands for Exception Regions
 
diff --git a/gcc/dwarf2cfi.c b/gcc/dwarf2cfi.c
index c2533b091b2..2fa9f325360 100644
--- a/gcc/dwarf2cfi.c
+++ b/gcc/dwarf2cfi.c
@@ -2848,6 +2848,12 @@  connect_traces (void)
 	      cfi->dw_cfi_opc = DW_CFA_restore_state;
 	      add_cfi (cfi);
 
+	      /* If the target unwinder does not save the CFA as part of the
+		 register state, we need to restore it separately.  */
+	      if (targetm.asm_out.should_restore_cfa_state ()
+		  && (cfi = def_cfa_0 (&old_row->cfa, &ti->beg_row->cfa)))
+		add_cfi (cfi);
+
 	      old_row = prev_ti->beg_row;
 	    }
 	  /* Otherwise, we'll simply change state from the previous end.  */
diff --git a/gcc/target.def b/gcc/target.def
index be7fcde961a..ab00657330a 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -211,6 +211,20 @@  DEFHOOKPOD
  be called afterward.",
  bool, true)
 
+/* Return true if the target needs extra instructions to restore the current
+   frame address after a DW_CFA_restore_state opcode.  */
+DEFHOOK
+(should_restore_cfa_state,
+ "For DWARF-based unwind frames, two CFI instructions provide for save and\
+ restore of register state.  GCC maintains the current frame address (CFA)\
+ separately from the register bank but the unwinder in libgcc preserves this\
+ state along with the registers (and this is expected by the code that writes\
+ the unwind frames).  This hook allows the target to specify that the CFA data\
+ is not saved/restored along with the registers by the target unwinder so that\
+ suitable additional instructions should be emitted to restore it.",
+ bool, (void),
+ hook_bool_void_false)
+ 
 /* Generate an internal label.
    For now this is just a wrapper for ASM_GENERATE_INTERNAL_LABEL.  */
 DEFHOOK_UNDOC