On Fri, Feb 26, 2021 at 11:04 PM Iain Sandoe <i...@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
>
>
