--gc-section leftovers workaround.

--gc-section leftovers workaround.

[Pedro Alves]

With --gc-sections, gnu ld zeros out the start addresses of
unreferences FDEs, but leaves them behind.  On targets where
addresses near 0 are valid, GDB can easily confuse these
leftovers for the valid FDEs, ending up with invalid debug
info loaded for such low addresses.  Since  this is a problem
that has beein in the field for a while, an FSF GDB
workaround seems appropriate, given that it is a simple
workaround --- give preference to FDEs that do not
start at zero when overlaps are detected.

Given that we now sort and bsearch FDEs, GDB is already giving
such preference as a side effect, but the code that discards
duplicate FDEs could be discarding the valid FDEs at 0, and
keep an FDE that should have been garbage collected.  This patch
makes the overlap check explicit so as to avoid that.   Tested on
arm-none-eabi where it fixed the problem, and on
x86_64-unknown-linux-gnu.  Applied.

-- 
Pedro Alves

2009-10-28  Pedro Alves  <pedro@codesourcery.com>

        * dwarf2-frame.c (dwarf2_build_frame_info): Discard --gc-section
	leftover FDEs.

---
 gdb/dwarf2-frame.c |   44 +++++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 13 deletions(-)

Index: src/gdb/dwarf2-frame.c
===================================================================
--- src.orig/gdb/dwarf2-frame.c	2009-10-23 19:05:16.000000000 +0100
+++ src/gdb/dwarf2-frame.c	2009-10-28 17:15:10.000000000 +0000
@@ -2101,7 +2101,8 @@ dwarf2_build_frame_info (struct objfile 
   if (fde_table.num_entries != 0)
     {
       struct dwarf2_fde_table *fde_table2;
-      int i, j;
+      struct dwarf2_fde *fde_prev = NULL;
+      int i;
 
       /* Prepare FDE table for lookups.  */
       qsort (fde_table.entries, fde_table.num_entries,
@@ -2110,22 +2111,39 @@ dwarf2_build_frame_info (struct objfile 
       /* Copy fde_table to obstack: it is needed at runtime.  */
       fde_table2 = (struct dwarf2_fde_table *)
           obstack_alloc (&objfile->objfile_obstack, sizeof (*fde_table2));
+      fde_table2->num_entries = 0;
 
       /* Since we'll be doing bsearch, squeeze out identical (except for
          eh_frame_p) fde entries so bsearch result is predictable.  */
-      for (i = 0, j = 0; j < fde_table.num_entries; ++i)
-        {
-          const int k = j;
-
-          obstack_grow (&objfile->objfile_obstack, &fde_table.entries[j],
-                        sizeof (fde_table.entries[0]));
-          while (++j < fde_table.num_entries
-                 && (fde_table.entries[k]->initial_location
-                     == fde_table.entries[j]->initial_location))
-            /* Skip.  */;
-        }
+      for (i = 0; i < fde_table.num_entries; i++)
+	{
+	  struct dwarf2_fde *fde = fde_table.entries[i];
+
+	  /* Check for leftovers from --gc-sections.  The GNU linker
+	     sets the relevant symbols to zero, but doesn't zero the
+	     FDE *end* ranges because there's no relocation there.
+	     It's (offset, length), not (start, end).  On targets
+	     where address zero is just another valid address this can
+	     be a problem, since the FDEs appear to be non-empty in
+	     the output --- we could pick out the wrong FDE.  To work
+	     around this, when overlaps are detected, we prefer FDEs
+	     that do not start at zero.  */
+	  if (fde->initial_location == 0
+	      && (i + 1) < fde_table.num_entries
+	      && ((fde_table.entries[i + 1])->initial_location
+		  < fde->initial_location + fde->address_range))
+	    continue;
+
+	  if (fde_prev != NULL
+	      && fde_prev->initial_location == fde->initial_location)
+	    continue;
+
+	  obstack_grow (&objfile->objfile_obstack, &fde_table.entries[i],
+			sizeof (fde_table.entries[0]));
+	  ++fde_table2->num_entries;
+	  fde_prev = fde;
+	}
       fde_table2->entries = obstack_finish (&objfile->objfile_obstack);
-      fde_table2->num_entries = i;
       set_objfile_data (objfile, dwarf2_frame_objfile_data, fde_table2);
 
       /* Discard the original fde_table.  */

Re: --gc-section leftovers workaround.

[Paul Pluzhnikov]

On Wed, Oct 28, 2009 at 11:51 AM, Pedro Alves <pedro@codesourcery.com> wrote:

> With --gc-sections, gnu ld zeros out the start addresses of
> unreferences FDEs, but leaves them behind.

I believe the same thing happens when there are multiple instances of the
same inline function in several translation units: one is selected, and
debug info for the other ones gets zero start address.

The patch looks good to me.

Thanks,
-- 
Paul Pluzhnikov

Re: --gc-section leftovers workaround.

[Pedro Alves]

On Wednesday 28 October 2009 18:51:00, Pedro Alves wrote:
> With --gc-sections, gnu ld zeros out the start addresses of
> unreferences FDEs, but leaves them behind.  On targets where
> addresses near 0 are valid, GDB can easily confuse these
> leftovers for the valid FDEs, ending up with invalid debug
> info loaded for such low addresses.  Since  this is a problem
> that has beein in the field for a while, an FSF GDB
> workaround seems appropriate, given that it is a simple
> workaround --- give preference to FDEs that do not
> start at zero when overlaps are detected.
> 
> Given that we now sort and bsearch FDEs, GDB is already giving
> such preference as a side effect, but the code that discards
> duplicate FDEs could be discarding the valid FDEs at 0, and
> keep an FDE that should have been garbage collected.  This patch
> makes the overlap check explicit so as to avoid that.   Tested on
> arm-none-eabi where it fixed the problem, and on
> x86_64-unknown-linux-gnu.  Applied.

I managed to post and apply the wrong patch.  I've reverted
it already.

Here's the correct patch that I'll (re-)apply in a bit.

-- 
Pedro Alves

2009-10-28  Pedro Alves  <pedro@codesourcery.com>

        * dwarf2-frame.c (dwarf2_build_frame_info): Discard --gc-section
	leftover FDEs.

---
 gdb/dwarf2-frame.c |   64 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 15 deletions(-)

Index: src/gdb/dwarf2-frame.c
===================================================================
--- src.orig/gdb/dwarf2-frame.c	2009-10-28 18:59:53.000000000 +0000
+++ src/gdb/dwarf2-frame.c	2009-10-28 19:07:29.000000000 +0000
@@ -2101,7 +2101,9 @@ dwarf2_build_frame_info (struct objfile 
   if (fde_table.num_entries != 0)
     {
       struct dwarf2_fde_table *fde_table2;
-      int i, j;
+      struct dwarf2_fde *fde_prev = NULL;
+      struct dwarf2_fde *first_non_zero_fde = NULL;
+      int i;
 
       /* Prepare FDE table for lookups.  */
       qsort (fde_table.entries, fde_table.num_entries,
@@ -2110,22 +2112,54 @@ dwarf2_build_frame_info (struct objfile 
       /* Copy fde_table to obstack: it is needed at runtime.  */
       fde_table2 = (struct dwarf2_fde_table *)
           obstack_alloc (&objfile->objfile_obstack, sizeof (*fde_table2));
+      fde_table2->num_entries = 0;
 
-      /* Since we'll be doing bsearch, squeeze out identical (except for
-         eh_frame_p) fde entries so bsearch result is predictable.  */
-      for (i = 0, j = 0; j < fde_table.num_entries; ++i)
-        {
-          const int k = j;
-
-          obstack_grow (&objfile->objfile_obstack, &fde_table.entries[j],
-                        sizeof (fde_table.entries[0]));
-          while (++j < fde_table.num_entries
-                 && (fde_table.entries[k]->initial_location
-                     == fde_table.entries[j]->initial_location))
-            /* Skip.  */;
-        }
+      /* Check for leftovers from --gc-sections.  The GNU linker sets
+	 the relevant symbols to zero, but doesn't zero the FDE *end*
+	 ranges because there's no relocation there.  It's (offset,
+	 length), not (start, end).  On targets where address zero is
+	 just another valid address this can be a problem, since the
+	 FDEs appear to be non-empty in the output --- we could pick
+	 out the wrong FDE.  To work around this, when overlaps are
+	 detected, we prefer FDEs that do not start at zero.
+
+	 Start by finding the first FDE with non-zero start.  Below
+	 we'll discard all FDEs that start at zero and overlap this
+	 one.  */
+      for (i = 0; i < fde_table.num_entries; i++)
+	{
+	  struct dwarf2_fde *fde = fde_table.entries[i];
+
+	  if (fde->initial_location != 0)
+	    {
+	      first_non_zero_fde = fde;
+	      break;
+	    }
+	}
+
+      /* Since we'll be doing bsearch, squeeze out identical (except
+	 for eh_frame_p) fde entries so bsearch result is predictable.
+	 Also discard leftovers from --gc-sections.  */
+      for (i = 0; i < fde_table.num_entries; i++)
+	{
+	  struct dwarf2_fde *fde = fde_table.entries[i];
+
+	  if (fde->initial_location == 0
+	      && first_non_zero_fde != NULL
+	      && (first_non_zero_fde->initial_location
+		  < fde->initial_location + fde->address_range))
+	    continue;
+
+	  if (fde_prev != NULL
+	      && fde_prev->initial_location == fde->initial_location)
+	    continue;
+
+	  obstack_grow (&objfile->objfile_obstack, &fde_table.entries[i],
+			sizeof (fde_table.entries[0]));
+	  ++fde_table2->num_entries;
+	  fde_prev = fde;
+	}
       fde_table2->entries = obstack_finish (&objfile->objfile_obstack);
-      fde_table2->num_entries = i;
       set_objfile_data (objfile, dwarf2_frame_objfile_data, fde_table2);
 
       /* Discard the original fde_table.  */

Re: --gc-section leftovers workaround.

[Pedro Alves]

On Wednesday 28 October 2009 19:23:11, Paul Pluzhnikov wrote:
> On Wed, Oct 28, 2009 at 11:51 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> 
> > With --gc-sections, gnu ld zeros out the start addresses of
> > unreferences FDEs, but leaves them behind.
> 
> I believe the same thing happens when there are multiple instances of the
> same inline function in several translation units: one is selected, and
> debug info for the other ones gets zero start address.

Ah.

> The patch looks good to me.

It wasn't unfortunately.  :-( I had posted an early patch
that got one thing wrong.  It compared FDE->initial_location to
(FDE+1)->initial_location, to check for overlap, and discarded
FDE if so.  But, this isn't correct since FDE may be the valid one
we should keep, and (FDE+1)->initial_location could be 0!  The
corrected patch first looks for the first FDE with
initial_location != 0 and uses that one for overlap and discarding
decisions.  Anyway, I think that patch is itself clearer than I
can manage to explain it.  :-)

-- 
Pedro Alves

Re: --gc-section leftovers workaround.

[Paul Pluzhnikov]

On Wed, Oct 28, 2009 at 12:35 PM, Pedro Alves <pedro@codesourcery.com> wrote:

>> The patch looks good to me.
>
> It wasn't unfortunately.  :-( I had posted an early patch
> that got one thing wrong.  It compared FDE->initial_location to
> (FDE+1)->initial_location, to check for overlap, and discarded
> FDE if so.  But, this isn't correct since FDE may be the valid one
> we should keep, and (FDE+1)->initial_location could be 0!

Since we've just sorted them, (FDE+1)->initial_location == 0 implies that
FDE->initial_location == 0 as well, doesn't it?


-- 
Paul Pluzhnikov

Re: --gc-section leftovers workaround.

[Pedro Alves]

On Wednesday 28 October 2009 20:13:43, Paul Pluzhnikov wrote:
> Since we've just sorted them, (FDE+1)->initial_location == 0 implies that
> FDE->initial_location == 0 as well, doesn't it?

Correct, but it may be FDE+1 that needs discarding, not FDE.

We could even have FDE,FDE+1,FDE+2,FDE+3, where
FDE,FDE+1,FDE+2 all have initial_location == 0, and FDE+3 not.
We want to compare each of FDE,FDE+1,FDE+2 with FDE+3 for
overlap and discard those that do overlap,
not FDE with FDE+1 ; FDE+1 with FDE+2 ; FDE+2 with FDE+3.

-- 
Pedro Alves

Re: --gc-section leftovers workaround.

[Paul Pluzhnikov]

On Wed, Oct 28, 2009 at 1:20 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> On Wednesday 28 October 2009 20:13:43, Paul Pluzhnikov wrote:
>> Since we've just sorted them, (FDE+1)->initial_location == 0 implies that
>> FDE->initial_location == 0 as well, doesn't it?
>
> Correct, but it may be FDE+1 that needs discarding, not FDE.

I think you mean "it may be that FDE+1 *also* needs discarding, not
just FDE".

Would you mind rephrasing comment to mention multiple inline and template
instances in addition to --gc-sections?  Also, it's not just GNU linker;
gold also does the same (after fix for PR gold/10400 that is), and I think
every other ELF linker has to do so as well.

Thanks,
-- 
Paul Pluzhnikov

Re: --gc-section leftovers workaround.

[Pedro Alves]

On Wednesday 28 October 2009 20:35:20, Paul Pluzhnikov wrote:
> Would you mind rephrasing comment to mention multiple inline and template
> instances in addition to --gc-sections?  Also, it's not just GNU linker;
> gold also does the same (after fix for PR gold/10400 that is), and I think
> every other ELF linker has to do so as well.

Does gold also not zero out FDE->address_range?
I don't mind at all.  Do you want to propose the wording / a patch?

-- 
Pedro Alves

Re: --gc-section leftovers workaround.

[Paul Pluzhnikov]

On Wed, Oct 28, 2009 at 1:55 PM, Pedro Alves <pedro@codesourcery.com> wrote:

> Does gold also not zero out FDE->address_range?

Correct (AFAICT).

> I don't mind at all.  Do you want to propose the wording / a patch?

Something like this, perhaps?

      /* Check for FDEs with zero initial_location.

         Such FDEs could come about when linker discards a section (either
         a COMDAT section when the linker selects a different section
         from the same group, or just a regular section discarded due
         to --gc-sections), but keeps the corresponding .debug_info.

	 The linker sets the relevant FDE.initial_location to zero, but
	 doesn't zero the FDE.address_range because there's no relocation
	 there.  It's (offset, length), not (start, end).

... rest as before ...

Thanks,
-- 
Paul Pluzhnikov