diff mbox series

Fix P81033 for FDEs in partitioned code.

Message ID EF7D1E44-B4CB-41F1-ADC0-38D49635C260@sandoe.co.uk
State New
Headers show
Series Fix P81033 for FDEs in partitioned code. | expand

Commit Message

Iain Sandoe Aug. 14, 2018, 11:20 a.m. UTC 
Hi,

A more detailed explanation is in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81033#c37

When function sub-sections are enabled, Darwin’s assembler needs the FDE local start
label for each sub-section to follow a linker-visible one so that the FDE will be correctly
associated with the code of the subsection.

The current code in final.c emits a linker-visible symbol, as needed by several targets.
However the local label used to define the FDE start precedes the linker-visible one
which, for Darwin causes it (the FDE start) to be associated with the previous linker-
visible symbol (or the section start if there isn’t one).  This applies regardless of the 
actual address of the label, for toolchain assemblers that have strict interpretation of
the Darwin sub-sections-via-symbols ABI.

The patch adds a new local label (analogous to the "LFBn” emitted for the regular
function starts) just after the linker-visible label emitted after switching text section.
The FDE second entry is made to point to this instead of the LcoldStartn one.  This
should be a no-op for targets using .cfi_ and for targets without sub-sections-via-symbols.

Bootstrapped on x86_64 and i686 linux and on a number of Darwin platforms (from 
i686-darwin9 to x86_64-darwin17).

OK for trunk?
open branches? (although it's a regression on 8, it’s a latent wrong-code on all branches)

thanks
Iain

2018-08-14  Iain Sandoe  <iain@sandoe.co.uk>

gcc:

        PR target/81033
	* dwarf2out.c (FUNC_SECOND_SECT_LABEL): New.
	(dwarf2out_switch_text_section): Generate subsection label and
	assign it to the FDE second section start.
	* final.c (final_scan_insn_1): Emit second section label when
	required.
---
 gcc/dwarf2out.c | 11 +++++++++--
 gcc/final.c     |  3 +++
 2 files changed, 12 insertions(+), 2 deletions(-)

Comments

Mike Stump Aug. 14, 2018, 8:18 p.m. UTC | #1 
On Aug 14, 2018, at 4:20 AM, Iain Sandoe <iain@sandoe.co.uk> wrote:
> When function sub-sections are enabled, Darwin’s assembler needs the FDE local start
> label for each sub-section to follow a linker-visible one so that the FDE will be correctly
> associated with the code of the subsection.
> 
> The current code in final.c emits a linker-visible symbol, as needed by several targets.
> However the local label used to define the FDE start precedes the linker-visible one
> which, for Darwin causes it (the FDE start) to be associated with the previous linker-
> visible symbol (or the section start if there isn’t one).  This applies regardless of the 
> actual address of the label, for toolchain assemblers that have strict interpretation of
> the Darwin sub-sections-via-symbols ABI.
> 
> The patch adds a new local label (analogous to the "LFBn” emitted for the regular
> function starts) just after the linker-visible label emitted after switching text section.
> The FDE second entry is made to point to this instead of the LcoldStartn one.  This
> should be a no-op for targets using .cfi_ and for targets without sub-sections-via-symbols.
> 
> Bootstrapped on x86_64 and i686 linux and on a number of Darwin platforms (from 
> i686-darwin9 to x86_64-darwin17).
> 
> OK for trunk?
> open branches? (although it's a regression on 8, it’s a latent wrong-code on all branches)

I'm fine with the darwin aspects of it, but I think it needs review/approval by final.c/dwarf people.
Richard Biener Aug. 20, 2018, 7:27 a.m. UTC | #2 
On Tue, Aug 14, 2018 at 10:18 PM Mike Stump <mikestump@comcast.net> wrote:
>
> On Aug 14, 2018, at 4:20 AM, Iain Sandoe <iain@sandoe.co.uk> wrote:
> > When function sub-sections are enabled, Darwin’s assembler needs the FDE local start
> > label for each sub-section to follow a linker-visible one so that the FDE will be correctly
> > associated with the code of the subsection.
> >
> > The current code in final.c emits a linker-visible symbol, as needed by several targets.
> > However the local label used to define the FDE start precedes the linker-visible one
> > which, for Darwin causes it (the FDE start) to be associated with the previous linker-
> > visible symbol (or the section start if there isn’t one).  This applies regardless of the
> > actual address of the label, for toolchain assemblers that have strict interpretation of
> > the Darwin sub-sections-via-symbols ABI.
> >
> > The patch adds a new local label (analogous to the "LFBn” emitted for the regular
> > function starts) just after the linker-visible label emitted after switching text section.
> > The FDE second entry is made to point to this instead of the LcoldStartn one.  This
> > should be a no-op for targets using .cfi_ and for targets without sub-sections-via-symbols.
> >
> > Bootstrapped on x86_64 and i686 linux and on a number of Darwin platforms (from
> > i686-darwin9 to x86_64-darwin17).
> >
> > OK for trunk?
> > open branches? (although it's a regression on 8, it’s a latent wrong-code on all branches)
>
> I'm fine with the darwin aspects of it, but I think it needs review/approval by final.c/dwarf people.

The approach looks fine (though we have extra labels even for targets
that do not need it).  But,

+  fde->dw_fde_second_begin = xstrdup (label);

to me it looks like dw_fde_node is GC allocated so the above needs ggc_strdup.

Richard.
Iain Sandoe Aug. 21, 2018, 9:45 a.m. UTC | #3 
> On 20 Aug 2018, at 08:27, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Tue, Aug 14, 2018 at 10:18 PM Mike Stump <mikestump@comcast.net> wrote:
>> 
>> On Aug 14, 2018, at 4:20 AM, Iain Sandoe <iain@sandoe.co.uk> wrote:
>>> When function sub-sections are enabled, Darwin’s assembler needs the FDE local start
>>> label for each sub-section to follow a linker-visible one so that the FDE will be correctly
>>> associated with the code of the subsection.
>>> 
>>> The current code in final.c emits a linker-visible symbol, as needed by several targets.
>>> However the local label used to define the FDE start precedes the linker-visible one
>>> which, for Darwin causes it (the FDE start) to be associated with the previous linker-
>>> visible symbol (or the section start if there isn’t one).  This applies regardless of the
>>> actual address of the label, for toolchain assemblers that have strict interpretation of
>>> the Darwin sub-sections-via-symbols ABI.
>>> 
>>> The patch adds a new local label (analogous to the "LFBn” emitted for the regular
>>> function starts) just after the linker-visible label emitted after switching text section.
>>> The FDE second entry is made to point to this instead of the LcoldStartn one.  This
>>> should be a no-op for targets using .cfi_ and for targets without sub-sections-via-symbols.
>>> 
>>> Bootstrapped on x86_64 and i686 linux and on a number of Darwin platforms (from
>>> i686-darwin9 to x86_64-darwin17).
>>> 
>>> OK for trunk?
>>> open branches? (although it's a regression on 8, it’s a latent wrong-code on all branches)
>> 
>> I'm fine with the darwin aspects of it, but I think it needs review/approval by final.c/dwarf people.
> 
> The approach looks fine (though we have extra labels even for targets
> that do not need it).  But,
> 
> +  fde->dw_fde_second_begin = xstrdup (label);
> 
> to me it looks like dw_fde_node is GC allocated so the above needs ggc_strdup.

Revised below, (bootstrapped Linux and Darwin).

OK to apply now?

thanks
Iain

gcc/

	PR bootstrap/81033
	PR target/81733
	PR target/52795
	* gcc/dwarf2out.c (FUNC_SECOND_SECT_LABEL): New.
	(dwarf2out_switch_text_section): Generate a local label for the second
	function sub-section and apply it as the second FDE start label.
	* gcc/final.c (final_scan_insn_1): Emit second FDE label after the second
	sub-section start.

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 236f199..6a66de7 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -297,6 +297,10 @@ static unsigned int rnglist_idx;
 #define FUNC_BEGIN_LABEL	"LFB"
 #endif
 
+#ifndef FUNC_SECOND_SECT_LABEL
+#define FUNC_SECOND_SECT_LABEL	"LFSB"
+#endif
+
 #ifndef FUNC_END_LABEL
 #define FUNC_END_LABEL		"LFE"
 #endif
@@ -1212,21 +1216,24 @@ static void set_cur_line_info_table (section *);
 void
 dwarf2out_switch_text_section (void)
 {
+  char label[MAX_ARTIFICIAL_LABEL_BYTES];
   section *sect;
   dw_fde_ref fde = cfun->fde;
 
   gcc_assert (cfun && fde && fde->dw_fde_second_begin == NULL);
 
+  ASM_GENERATE_INTERNAL_LABEL (label, FUNC_SECOND_SECT_LABEL,
+			       current_function_funcdef_no);
+
+  fde->dw_fde_second_begin = ggc_strdup (label);
   if (!in_cold_section_p)
     {
       fde->dw_fde_end = crtl->subsections.cold_section_end_label;
-      fde->dw_fde_second_begin = crtl->subsections.hot_section_label;
       fde->dw_fde_second_end = crtl->subsections.hot_section_end_label;
     }
   else
     {
       fde->dw_fde_end = crtl->subsections.hot_section_end_label;
-      fde->dw_fde_second_begin = crtl->subsections.cold_section_label;
       fde->dw_fde_second_end = crtl->subsections.cold_section_end_label;
     }
   have_multiple_function_sections = true;
diff --git a/gcc/final.c b/gcc/final.c
index 842e5e0..6943c07 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -2232,6 +2232,9 @@ final_scan_insn_1 (rtx_insn *insn, FILE *file, int optimize_p ATTRIBUTE_UNUSED,
 	      ASM_OUTPUT_LABEL (asm_out_file,
 				IDENTIFIER_POINTER (cold_function_name));
 #endif
+	      if (dwarf2out_do_frame ()
+	          && cfun->fde->dw_fde_second_begin != NULL)
+		ASM_OUTPUT_LABEL (asm_out_file, cfun->fde->dw_fde_second_begin);
 	    }
 	  break;
Richard Biener Aug. 21, 2018, 9:58 a.m. UTC | #4 
On Tue, Aug 21, 2018 at 11:45 AM Iain Sandoe <iain@sandoe.co.uk> wrote:
>
>
> > On 20 Aug 2018, at 08:27, Richard Biener <richard.guenther@gmail.com> wrote:
> >
> > On Tue, Aug 14, 2018 at 10:18 PM Mike Stump <mikestump@comcast.net> wrote:
> >>
> >> On Aug 14, 2018, at 4:20 AM, Iain Sandoe <iain@sandoe.co.uk> wrote:
> >>> When function sub-sections are enabled, Darwin’s assembler needs the FDE local start
> >>> label for each sub-section to follow a linker-visible one so that the FDE will be correctly
> >>> associated with the code of the subsection.
> >>>
> >>> The current code in final.c emits a linker-visible symbol, as needed by several targets.
> >>> However the local label used to define the FDE start precedes the linker-visible one
> >>> which, for Darwin causes it (the FDE start) to be associated with the previous linker-
> >>> visible symbol (or the section start if there isn’t one).  This applies regardless of the
> >>> actual address of the label, for toolchain assemblers that have strict interpretation of
> >>> the Darwin sub-sections-via-symbols ABI.
> >>>
> >>> The patch adds a new local label (analogous to the "LFBn” emitted for the regular
> >>> function starts) just after the linker-visible label emitted after switching text section.
> >>> The FDE second entry is made to point to this instead of the LcoldStartn one.  This
> >>> should be a no-op for targets using .cfi_ and for targets without sub-sections-via-symbols.
> >>>
> >>> Bootstrapped on x86_64 and i686 linux and on a number of Darwin platforms (from
> >>> i686-darwin9 to x86_64-darwin17).
> >>>
> >>> OK for trunk?
> >>> open branches? (although it's a regression on 8, it’s a latent wrong-code on all branches)
> >>
> >> I'm fine with the darwin aspects of it, but I think it needs review/approval by final.c/dwarf people.
> >
> > The approach looks fine (though we have extra labels even for targets
> > that do not need it).  But,
> >
> > +  fde->dw_fde_second_begin = xstrdup (label);
> >
> > to me it looks like dw_fde_node is GC allocated so the above needs ggc_strdup.
>
> Revised below, (bootstrapped Linux and Darwin).
>
> OK to apply now?

OK.

Richard.

> thanks
> Iain
>
> gcc/
>
>         PR bootstrap/81033
>         PR target/81733
>         PR target/52795
>         * gcc/dwarf2out.c (FUNC_SECOND_SECT_LABEL): New.
>         (dwarf2out_switch_text_section): Generate a local label for the second
>         function sub-section and apply it as the second FDE start label.
>         * gcc/final.c (final_scan_insn_1): Emit second FDE label after the second
>         sub-section start.
>
> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index 236f199..6a66de7 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -297,6 +297,10 @@ static unsigned int rnglist_idx;
>  #define FUNC_BEGIN_LABEL       "LFB"
>  #endif
>
> +#ifndef FUNC_SECOND_SECT_LABEL
> +#define FUNC_SECOND_SECT_LABEL "LFSB"
> +#endif
> +
>  #ifndef FUNC_END_LABEL
>  #define FUNC_END_LABEL         "LFE"
>  #endif
> @@ -1212,21 +1216,24 @@ static void set_cur_line_info_table (section *);
>  void
>  dwarf2out_switch_text_section (void)
>  {
> +  char label[MAX_ARTIFICIAL_LABEL_BYTES];
>    section *sect;
>    dw_fde_ref fde = cfun->fde;
>
>    gcc_assert (cfun && fde && fde->dw_fde_second_begin == NULL);
>
> +  ASM_GENERATE_INTERNAL_LABEL (label, FUNC_SECOND_SECT_LABEL,
> +                              current_function_funcdef_no);
> +
> +  fde->dw_fde_second_begin = ggc_strdup (label);
>    if (!in_cold_section_p)
>      {
>        fde->dw_fde_end = crtl->subsections.cold_section_end_label;
> -      fde->dw_fde_second_begin = crtl->subsections.hot_section_label;
>        fde->dw_fde_second_end = crtl->subsections.hot_section_end_label;
>      }
>    else
>      {
>        fde->dw_fde_end = crtl->subsections.hot_section_end_label;
> -      fde->dw_fde_second_begin = crtl->subsections.cold_section_label;
>        fde->dw_fde_second_end = crtl->subsections.cold_section_end_label;
>      }
>    have_multiple_function_sections = true;
> diff --git a/gcc/final.c b/gcc/final.c
> index 842e5e0..6943c07 100644
> --- a/gcc/final.c
> +++ b/gcc/final.c
> @@ -2232,6 +2232,9 @@ final_scan_insn_1 (rtx_insn *insn, FILE *file, int optimize_p ATTRIBUTE_UNUSED,
>               ASM_OUTPUT_LABEL (asm_out_file,
>                                 IDENTIFIER_POINTER (cold_function_name));
>  #endif
> +             if (dwarf2out_do_frame ()
> +                 && cfun->fde->dw_fde_second_begin != NULL)
> +               ASM_OUTPUT_LABEL (asm_out_file, cfun->fde->dw_fde_second_begin);
>             }
>           break;
>
>
diff mbox series

Patch

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 9ed473088e..c7557eb51d 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -297,6 +297,10 @@  static unsigned int rnglist_idx;
 #define FUNC_BEGIN_LABEL	"LFB"
 #endif
 
+#ifndef FUNC_SECOND_SECT_LABEL
+#define FUNC_SECOND_SECT_LABEL	"LFSB"
+#endif
+
 #ifndef FUNC_END_LABEL
 #define FUNC_END_LABEL		"LFE"
 #endif
@@ -1212,21 +1216,24 @@  static void set_cur_line_info_table (section *);
 void
 dwarf2out_switch_text_section (void)
 {
+  char label[MAX_ARTIFICIAL_LABEL_BYTES];
   section *sect;
   dw_fde_ref fde = cfun->fde;
 
   gcc_assert (cfun && fde && fde->dw_fde_second_begin == NULL);
 
+  ASM_GENERATE_INTERNAL_LABEL (label, FUNC_SECOND_SECT_LABEL,
+			       current_function_funcdef_no);
+
+  fde->dw_fde_second_begin = xstrdup (label);
   if (!in_cold_section_p)
     {
       fde->dw_fde_end = crtl->subsections.cold_section_end_label;
-      fde->dw_fde_second_begin = crtl->subsections.hot_section_label;
       fde->dw_fde_second_end = crtl->subsections.hot_section_end_label;
     }
   else
     {
       fde->dw_fde_end = crtl->subsections.hot_section_end_label;
-      fde->dw_fde_second_begin = crtl->subsections.cold_section_label;
       fde->dw_fde_second_end = crtl->subsections.cold_section_end_label;
     }
   have_multiple_function_sections = true;
diff --git a/gcc/final.c b/gcc/final.c
index 842e5e067d..6943c073d9 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -2232,6 +2232,9 @@  final_scan_insn_1 (rtx_insn *insn, FILE *file, int optimize_p ATTRIBUTE_UNUSED,
 	      ASM_OUTPUT_LABEL (asm_out_file,
 				IDENTIFIER_POINTER (cold_function_name));
 #endif
+	      if (dwarf2out_do_frame ()
+	          && cfun->fde->dw_fde_second_begin != NULL)
+		ASM_OUTPUT_LABEL (asm_out_file, cfun->fde->dw_fde_second_begin);
 	    }
 	  break;