Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov <ppluzhnikov@google.com>]

[-- Attachment #1: Type: text/plain, Size: 5427 bytes --]

On Tue, Sep 8, 2009 at 10:58 PM, Joel Brobecker<brobecker@adacore.com> wrote:

>> +/* Return 1 if SECTION should be inserted into the section map.
>> +   We want to insert only non-overlay and non-TLS section.  */
>
> Can you explain what we do not want to add TLS sections? Is that
> just an optimization (code addresses should never point to TLS)?

TLS sections for different objfiles do in fact overlap (and would trigger
a complaint). The loader relocates them (separately for each thread and
each objfile), so their "on disk" VMA has nothing to do with their final
location in memory.

>> +/* Filter out overlapping sections where one section came from the real
>> +    objfile, and the other from a separate debuginfo file.
>> +    Return the size of table after redundant sections have been eliminated.   */
>> +         if (sect1_addr == sect2_addr
>> +          && (objfile1->separate_debug_objfile == objfile2
>> +                || objfile2->separate_debug_objfile == objfile1))
>
> Looks like "overlapping" above also means start at the same address?
> Is that normal? Or good enough for our purpose?

The separate debuginfo objfile is essentially just a copy of the "primary"
objfile, but with '.text' and '.data' removed (to conserve space).

You build a file with debug info, then split it into two: from the "primary"
file you strip debug info (not needed for execution). From the "secondary"
file you strip the code/data (not needed for debugging).

[The separate debuginfo file is thus fundamentally different from the
"bunch of .o files with debug info on MacOS", and extending the separate
debuginfo to cover MacOS situation (mentioned elsewhere as a possible
solution) is (IMHO) the wrong approach.]

Given above, one would expect the section table for the "primary" and
"secondary" files to be identical, and for every section in the "primary"
to be exactly matched by a corresponding section in the "secondary".

This was in fact my expectation, implemented in
gdb-find_pc_section-20090722-4.txt (and earlier versions) in this thread.

This expectation proved wrong: on Fedora 11 with prelinking, .rel.dyn may
change type (and therefore size), triggering an assert for Tom Tromey:
  http://sourceware.org/ml/gdb-patches/2009-08/msg00044.html
analyzed here:
  http://sourceware.org/ml/gdb-patches/2009-08/msg00120.html
and fixed here:
  http://sourceware.org/ml/gdb-patches/2009-08/msg00130.html

Therefore, we now check just the start of the section (should be identical)
but not the size (could be different under rare conditions).

>> +/* Filter out overlapping sections, issuing a warning if any are found.
>> +    Overlapping sections could really be overlay sections which we didn't
>> +    classify as such in insert_section_p, or we could be dealing with a
>> +    corrupt binary.   */
>
> I think we should also mention the MacOS port where we load all sections
> of all .o files instead of just the debugging info.
> It looks like a design flaw in the MacOS port, but it was really a shortcut in getting
> things to work (aka a hack).   I believe Tristan is planning on fixing
> this in the relatively near future, but in the meantime, it might be
> a useful comment.

I thought this patch would also fix MacOS, by filtering out sections from
.o because they would satisfy 'vma != lma' condition.

I was wrong (I don't even understand now why I thought that) -- they don't
and this patch doesn't fix MacOS. A separate patch, something like
  http://sourceware.org/ml/gdb-patches/2009-08/msg00195.html
is needed.

Given this, I don't think I should mention MacOS here.

>> +                warning (_("Unexpected overlap between "
>> +                                 "section `%s' from `%s' [%s, %s) and "
>> +                                 "section `%s' from `%s' [%s, %s)"),
>> +                              bfd_section_name (abfd1, bfds1), objf1->name,
>> +                              paddress (gdbarch, sect1_addr),
>> +                              paddress (gdbarch, sect1_endaddr),
>> +                              bfd_section_name (abfd2, bfds2), objf2->name,
>> +                              paddress (gdbarch, sect2_addr),
>> +                              paddress (gdbarch, sect2_endaddr));
>
> Let's please use a complaint rather than a warning. As explained in
> one of my other messages, the warning causes too much output on MacOS.

Yes: until MacOS is really fixed, you'll see a ton of warnings. They are
indication of a real problem.

> But I also see, now, after reading Ulrich's messages, that he suggested
> the same thing.

When I was reading his message, I didn't realize that "complaint" was a
GDB function (I've never seen it before).

I am not sure calling complaint rather than warning is really the right
thing here: complaints are silent unless the user explicitly turns them on,
and overlapping sections indicate a real problem of some sort.

>>   /* Update PMAP, PMAP_SIZE with non-TLS sections from all objfiles.   */
>
> We should update the comment to explain that overlay sections are
> also eliminated, as well as overlapping sections.

Done in the attached patch.

Thanks,
-- 
Paul Pluzhnikov

2009-09-10  Paul Pluzhnikov  <ppluzhnikov@google.com>

       * objfiles.c (qsort_cmp): Remove asserts.
       (insert_section_p, filter_debuginfo_sections): New function.
       (filter_overlapping_sections): Likewise.
       (update_section_map): Adjust.

[-- Attachment #2: gdb-find_pc_section-20090910.txt --]
[-- Type: text/plain, Size: 6981 bytes --]

Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.94
diff -u -p -u -r1.94 objfiles.c
--- objfiles.c	27 Aug 2009 21:56:38 -0000	1.94
+++ objfiles.c	10 Sep 2009 17:19:42 -0000
@@ -802,15 +802,9 @@ qsort_cmp (const void *a, const void *b)
   const CORE_ADDR sect2_addr = obj_section_addr (sect2);
 
   if (sect1_addr < sect2_addr)
-    {
-      gdb_assert (obj_section_endaddr (sect1) <= sect2_addr);
-      return -1;
-    }
+    return -1;
   else if (sect1_addr > sect2_addr)
-    {
-      gdb_assert (sect1_addr >= obj_section_endaddr (sect2));
-      return 1;
-    }
+    return 1;
 
   return 0;
 }
@@ -835,12 +829,134 @@ preferred_obj_section (struct obj_sectio
   return b;
 }
 
-/* Update PMAP, PMAP_SIZE with non-TLS sections from all objfiles.  */
+/* Return 1 if SECTION should be inserted into the section map.
+   We want to insert only non-overlay and non-TLS section.  */
+
+static int
+insert_section_p (const struct bfd *abfd,
+		  const struct bfd_section *section)
+{
+  const bfd_vma lma = bfd_section_lma (abfd, section);
+
+  if (lma != 0 && lma != bfd_section_vma (abfd, section)
+      && (bfd_get_file_flags (abfd) & BFD_IN_MEMORY) == 0)
+    /* This is an overlay section.  IN_MEMORY check is needed to avoid
+       discarding sections from the "system supplied DSO" (aka vdso)
+       on Linux.  */
+    return 0;
+  if ((bfd_get_section_flags (abfd, section) & SEC_THREAD_LOCAL) != 0)
+    /* This is a TLS section.  */
+    return 0;
+
+  return 1;
+}
+
+/* Filter out overlapping sections where one section came from the real
+   objfile, and the other from a separate debuginfo file.
+   Return the size of table after redundant sections have been eliminated.  */
+
+static int
+filter_debuginfo_sections (struct obj_section **map, int map_size)
+{
+  int i, j;
+
+  for (i = 0, j = 0; i < map_size - 1; i++)
+    {
+      struct obj_section *const sect1 = map[i];
+      struct obj_section *const sect2 = map[i + 1];
+      const struct objfile *const objfile1 = sect1->objfile;
+      const struct objfile *const objfile2 = sect2->objfile;
+      const CORE_ADDR sect1_addr = obj_section_addr (sect1);
+      const CORE_ADDR sect2_addr = obj_section_addr (sect2);
+
+      if (sect1_addr == sect2_addr
+	  && (objfile1->separate_debug_objfile == objfile2
+	      || objfile2->separate_debug_objfile == objfile1))
+	{
+	  map[j++] = preferred_obj_section (sect1, sect2);
+	  ++i;
+	}
+      else
+	map[j++] = sect1;
+    }
+
+  if (i < map_size)
+    map[j++] = map[i];
+
+  /* The map should not have shrunk to less than half the original size.  */
+  gdb_assert (map_size / 2 <= j);
+
+  return j;
+}
+
+/* Filter out overlapping sections, issuing a warning if any are found.
+   Overlapping sections could really be overlay sections which we didn't
+   classify as such in insert_section_p, or we could be dealing with a
+   corrupt binary.  */
+
+static int
+filter_overlapping_sections (struct obj_section **map, int map_size)
+{
+  int i, j;
+
+  for (i = 0, j = 0; i < map_size - 1; )
+    {
+      int k;
+
+      map[j++] = map[i];
+      for (k = i + 1; k < map_size; k++)
+	{
+	  struct obj_section *const sect1 = map[i];
+	  struct obj_section *const sect2 = map[k];
+	  const CORE_ADDR sect1_addr = obj_section_addr (sect1);
+	  const CORE_ADDR sect2_addr = obj_section_addr (sect2);
+	  const CORE_ADDR sect1_endaddr = obj_section_endaddr (sect1);
+
+	  gdb_assert (sect1_addr <= sect2_addr);
+
+	  if (sect1_endaddr <= sect2_addr)
+	    break;
+	  else
+	    {
+	      /* We have an overlap.  Report it.  */
+
+	      struct objfile *const objf1 = sect1->objfile;
+	      struct objfile *const objf2 = sect2->objfile;
+
+	      const struct bfd *const abfd1 = objf1->obfd;
+	      const struct bfd *const abfd2 = objf2->obfd;
+
+	      const struct bfd_section *const bfds1 = sect1->the_bfd_section;
+	      const struct bfd_section *const bfds2 = sect2->the_bfd_section;
+
+	      const CORE_ADDR sect2_endaddr = obj_section_endaddr (sect2);
+
+	      struct gdbarch *const gdbarch = get_objfile_arch (objf1);
+
+	      warning (_("Unexpected overlap between "
+			 "section `%s' from `%s' [%s, %s) and "
+			 "section `%s' from `%s' [%s, %s)"),
+		       bfd_section_name (abfd1, bfds1), objf1->name,
+		       paddress (gdbarch, sect1_addr),
+		       paddress (gdbarch, sect1_endaddr),
+		       bfd_section_name (abfd2, bfds2), objf2->name,
+		       paddress (gdbarch, sect2_addr),
+		       paddress (gdbarch, sect2_endaddr));
+	    }
+	}
+      i = k;
+    }
+  return map_size;
+}
+
+
+/* Update PMAP, PMAP_SIZE with sections from all objfiles, excluding any
+   TLS, overlay and overlapping sections.  */
 
 static void
 update_section_map (struct obj_section ***pmap, int *pmap_size)
 {
-  int map_size, i, j;
+  int alloc_size, map_size, i;
   struct obj_section *s, **map;
   struct objfile *objfile;
 
@@ -849,55 +965,27 @@ update_section_map (struct obj_section *
   map = *pmap;
   xfree (map);
 
-#define insert_p(objf, sec) \
-  ((bfd_get_section_flags ((objf)->obfd, (sec)->the_bfd_section) \
-    & SEC_THREAD_LOCAL) == 0)
-
-  map_size = 0;
+  alloc_size = 0;
   ALL_OBJSECTIONS (objfile, s)
-    if (insert_p (objfile, s))
-      map_size += 1;
+    if (insert_section_p (objfile->obfd, s->the_bfd_section))
+      alloc_size += 1;
 
-  map = xmalloc (map_size * sizeof (*map));
+  map = xmalloc (alloc_size * sizeof (*map));
 
   i = 0;
   ALL_OBJSECTIONS (objfile, s)
-    if (insert_p (objfile, s))
+    if (insert_section_p (objfile->obfd, s->the_bfd_section))
       map[i++] = s;
 
-#undef insert_p
-
-  qsort (map, map_size, sizeof (*map), qsort_cmp);
-
-  /* With separate debuginfo files, we may have up to two (almost)
-     identical copies of some obj_sections in the map.
-     Filter out duplicates.  */
-  for (i = 0, j = 0; i < map_size; ++i)
-    {
-      struct obj_section *sect1 = map[i];
-      struct obj_section *sect2 = (i + 1 < map_size) ? map[i + 1] : NULL;
-
-      if (sect2 == NULL
-	  || obj_section_addr (sect1) != obj_section_addr (sect2))
-	map[j++] = sect1;
-      else
-	{
-	  map[j++] = preferred_obj_section (sect1, sect2);
-	  ++i;
-	}
-    }
-
-  if (j < map_size)
-    {
-      /* Some duplicates were eliminated.
-	 The new size shouldn't be less than half of the original. */
-      gdb_assert (map_size / 2 <= j);
-      map_size = j;
-
-      map = xrealloc (map, map_size * sizeof (*map));  /* Trim excess space.  */
-    }
+  qsort (map, alloc_size, sizeof (*map), qsort_cmp);
+  map_size = filter_debuginfo_sections(map, alloc_size);
+  map_size = filter_overlapping_sections(map, map_size);
+
+  if (map_size < alloc_size)
+    /* Some sections were eliminated.  Trim excess space.  */
+    map = xrealloc (map, map_size * sizeof (*map));
   else
-    gdb_assert (j == map_size);
+    gdb_assert (alloc_size == map_size);
 
   *pmap = map;
   *pmap_size = map_size;