[patch] Speed up find_pc_section

[patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

Greetings,

While working on something else, I noticed find_pc_section consuming 90%+
of CPU in my GDB profiles :-(

It is called from quite a number of places, and is surprisingly
inefficient. Further, it exhibits yet another quadratic slowdown on number
of objfiles, as the attached test case demonstrates.

Running "perl gen.pl NNN" generates NNN shared libs, and a main executable,
which crashes with NNN+C levels in stack trace, crossing all shared libs.

I then collected timing like this:
time gdb64-cvs -ex run -ex where -ex quit ./a.out > /dev/null

Here are user-time results before the patch (clearly showing non-linear
slowdown):

32:  0m0.426s
64:  0m1.344s
128: 0m5.254s
256: 0m35.792s
512: 4m55.999s

Attached patch turns linear search into binary search, speeding up
find_pc_section drastically :-)

Here are the same user-time results after the patch:

128: 0m1.064s
256: 0m3.062s
512: 0m10.675s

Tested on Linux/x86_64 with no regressions.

Comments?
--
Paul Pluzhnikov

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

Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.85
diff -u -p -u -r1.85 objfiles.c
--- objfiles.c	14 Jul 2009 14:55:06 -0000	1.85
+++ objfiles.c	17 Jul 2009 00:21:13 -0000
@@ -64,6 +64,18 @@ struct objfile *current_objfile;	/* For 
 struct objfile *symfile_objfile;	/* Main symbol table loaded from */
 struct objfile *rt_common_objfile;	/* For runtime common symbols */
 
+struct address_to_obj_section
+{
+  CORE_ADDR addr;
+  CORE_ADDR endaddr;
+  struct obj_section *section;
+};
+
+/* Records whether any objfiles appeared or disappeared since we last updated
+   address to obj section map.  */
+
+static int objfiles_updated_p;
+
 /* Locate all mappable sections of a BFD file. 
    objfile_p_char is a char * to get it through
    bfd_map_over_sections; we cast it back to its proper type.  */
@@ -94,6 +106,7 @@ add_to_objfile_sections (struct bfd *abf
   obstack_grow (&objfile->objfile_obstack, (char *) &section, sizeof (section));
   objfile->sections_end
     = (struct obj_section *) (((size_t) objfile->sections_end) + 1);
+  objfiles_updated_p += 1;  /* Rebuild section map next time we need it.  */
 }
 
 /* Builds a section table for OBJFILE.
@@ -394,6 +407,7 @@ unlink_objfile (struct objfile *objfile)
 void
 free_objfile (struct objfile *objfile)
 {
+  objfiles_updated_p += 1;  /* Rebuild section map next time we need it.  */
   if (objfile->separate_debug_objfile)
     {
       free_objfile (objfile->separate_debug_objfile);
@@ -757,22 +771,121 @@ have_minimal_symbols (void)
   return 0;
 }
 
+/* Qsort comparison function.  */
+static int
+section_map_compare (const void *a, const void *b)
+{
+  const struct address_to_obj_section *aa =
+      (const struct address_to_obj_section *) a;
+  const struct address_to_obj_section *bb =
+      (const struct address_to_obj_section *) b;
+
+  if (aa->addr < bb->addr)
+    {
+      gdb_assert (aa->endaddr <= bb->addr);
+      return -1;
+    }
+  else if (aa->addr > bb->addr)
+    {
+      gdb_assert (aa->addr >= bb->endaddr);
+      return 1;
+    }
+  /* This can happen for separate debug-info files.  */
+  gdb_assert (aa->endaddr == bb->endaddr);
+
+  return 0;
+}
+
+/* Update PMAP, PMAP_SIZE with non-TLS sections from all objfiles.  */
+
+static void
+update_section_map (struct address_to_obj_section **pmap,
+                    int *pmap_size)
+{
+  int map_size, idx;
+  struct obj_section *s;
+  struct objfile *objfile;
+  struct address_to_obj_section *map;
+
+  gdb_assert (objfiles_updated_p != 0);
+
+  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;
+  ALL_OBJSECTIONS (objfile, s)
+    if (insert_p (objfile, s))
+      map_size += 1;
+
+  map = xmalloc (map_size * sizeof (*map));
+
+  idx = 0;
+  ALL_OBJSECTIONS (objfile, s)
+    if (insert_p (objfile, s))
+      {
+        map[idx].section = s;
+        map[idx].addr = obj_section_addr (s);
+        map[idx].endaddr = obj_section_endaddr (s);
+        idx += 1;
+      }
+
+#undef insert_p
+
+  qsort (map, map_size, sizeof (*map), section_map_compare);
+
+  *pmap = map;
+  *pmap_size = map_size;
+}
+
+/* Bsearch comparison function. */
+
+int
+find_pc_section_cmp (const void *key, const void *elt)
+{
+  CORE_ADDR pc = *(CORE_ADDR *) key;
+  const struct address_to_obj_section *entry =
+      (const struct address_to_obj_section *) elt;
+
+  if (pc < entry->addr)
+    return -1;
+  if (pc < entry->endaddr)
+    return 0;
+  return 1;
+}
+
 /* Returns a section whose range includes PC or NULL if none found.   */
 
 struct obj_section *
 find_pc_section (CORE_ADDR pc)
 {
+  static struct address_to_obj_section *sections;
+  static int num_sections;
+
   struct obj_section *s;
-  struct objfile *objfile;
+  struct address_to_obj_section *aos;
 
   /* Check for mapped overlay section first.  */
   s = find_pc_mapped_section (pc);
   if (s)
     return s;
 
-  ALL_OBJSECTIONS (objfile, s)
-    if (obj_section_addr (s) <= pc && pc < obj_section_endaddr (s))
-      return s;
+  if (objfiles_updated_p != 0)
+    {
+      update_section_map (&sections, &num_sections);
+
+      /* Don't need updates to section map until objfiles are added
+         or removed.  */
+      objfiles_updated_p = 0;
+    }
+
+  aos = bsearch (&pc, sections, num_sections, sizeof (*sections),
+                 find_pc_section_cmp);
+  if (aos)
+    return aos->section;
 
   return NULL;
 }

[-- Attachment #3: gen.pl --]
[-- Type: application/x-perl, Size: 633 bytes --]

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Thu, Jul 16, 2009 at 5:24 PM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:

> Attached patch turns linear search into binary search, speeding up
> find_pc_section drastically :-)

A cleaner patch, with ChangeLog this time.

Thanks,
-- 
Paul Pluzhnikov

2009-07-17  Paul Pluzhnikov  <ppluzhnikov@google.com>

        * objfiles.c (objfiles_updated_p): New variable.
        (qsort_cmp, bsearch_cmp, update_section_map): New function.
        (find_pc_section): Use bsearch.

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

Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.85
diff -u -p -u -r1.85 objfiles.c
--- objfiles.c	14 Jul 2009 14:55:06 -0000	1.85
+++ objfiles.c	17 Jul 2009 14:52:47 -0000
@@ -64,6 +64,11 @@ struct objfile *current_objfile;	/* For 
 struct objfile *symfile_objfile;	/* Main symbol table loaded from */
 struct objfile *rt_common_objfile;	/* For runtime common symbols */
 
+/* Records whether any objfiles appeared or disappeared since we last updated
+   address to obj section map.  */
+
+static int objfiles_updated_p;
+
 /* Locate all mappable sections of a BFD file. 
    objfile_p_char is a char * to get it through
    bfd_map_over_sections; we cast it back to its proper type.  */
@@ -94,6 +99,7 @@ add_to_objfile_sections (struct bfd *abf
   obstack_grow (&objfile->objfile_obstack, (char *) &section, sizeof (section));
   objfile->sections_end
     = (struct obj_section *) (((size_t) objfile->sections_end) + 1);
+  objfiles_updated_p += 1;  /* Rebuild section map next time we need it.  */
 }
 
 /* Builds a section table for OBJFILE.
@@ -394,6 +400,7 @@ unlink_objfile (struct objfile *objfile)
 void
 free_objfile (struct objfile *objfile)
 {
+  objfiles_updated_p += 1;  /* Rebuild section map next time we need it.  */
   if (objfile->separate_debug_objfile)
     {
       free_objfile (objfile->separate_debug_objfile);
@@ -757,23 +764,113 @@ have_minimal_symbols (void)
   return 0;
 }
 
+/* Qsort comparison function.  */
+
+static int
+qsort_cmp (const void *a, const void *b)
+{
+  const struct obj_section *sect1 = *(const struct obj_section **) a;
+  const struct obj_section *sect2 = *(const struct obj_section **) b;
+  const CORE_ADDR sect1_addr = obj_section_addr (sect1);
+  const CORE_ADDR sect2_addr = obj_section_addr (sect2);
+
+  if (sect1_addr < sect2_addr)
+    {
+      gdb_assert (obj_section_endaddr (sect1) <= sect2_addr);
+      return -1;
+    }
+  else if (sect1_addr > sect2_addr)
+    {
+      gdb_assert (sect1_addr >= obj_section_endaddr (sect2));
+      return 1;
+    }
+  /* This can happen for separate debug-info files.  */
+  gdb_assert (obj_section_endaddr (sect1) == obj_section_endaddr (sect2));
+
+  return 0;
+}
+
+/* Update PMAP, PMAP_SIZE with non-TLS sections from all objfiles.  */
+
+static void
+update_section_map (struct obj_section ***pmap, int *pmap_size)
+{
+  int map_size, idx;
+  struct obj_section *s, **map;
+  struct objfile *objfile;
+
+  gdb_assert (objfiles_updated_p != 0);
+
+  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;
+  ALL_OBJSECTIONS (objfile, s)
+    if (insert_p (objfile, s))
+      map_size += 1;
+
+  map = xmalloc (map_size * sizeof (*map));
+
+  idx = 0;
+  ALL_OBJSECTIONS (objfile, s)
+    if (insert_p (objfile, s))
+      map[idx++] = s;
+
+#undef insert_p
+
+  qsort (map, map_size, sizeof (*map), qsort_cmp);
+
+  *pmap = map;
+  *pmap_size = map_size;
+}
+
+/* Bsearch comparison function. */
+
+static int
+bsearch_cmp (const void *key, const void *elt)
+{
+  const CORE_ADDR pc = *(CORE_ADDR *) key;
+  const struct obj_section *section = *(const struct obj_section **) elt;
+
+  if (pc < obj_section_addr (section))
+    return -1;
+  if (pc < obj_section_endaddr (section))
+    return 0;
+  return 1;
+}
+
 /* Returns a section whose range includes PC or NULL if none found.   */
 
 struct obj_section *
 find_pc_section (CORE_ADDR pc)
 {
-  struct obj_section *s;
-  struct objfile *objfile;
+  static struct obj_section **sections;
+  static int num_sections;
+
+  struct obj_section *s, **sp;
 
   /* Check for mapped overlay section first.  */
   s = find_pc_mapped_section (pc);
   if (s)
     return s;
 
-  ALL_OBJSECTIONS (objfile, s)
-    if (obj_section_addr (s) <= pc && pc < obj_section_endaddr (s))
-      return s;
+  if (objfiles_updated_p != 0)
+    {
+      update_section_map (&sections, &num_sections);
+
+      /* Don't need updates to section map until objfiles are added
+         or removed.  */
+      objfiles_updated_p = 0;
+    }
 
+  sp = (struct obj_section **) bsearch (&pc, sections, num_sections,
+					sizeof (*sections), bsearch_cmp);
+  if (sp != NULL)
+    return *sp;
   return NULL;
 }
 

Re: [patch] Speed up find_pc_section

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> Attached patch turns linear search into binary search, speeding up
Paul> find_pc_section drastically :-)

I love patches like this.

Paul> +static int objfiles_updated_p;

This (plus the map itself) seems like another thing that will have to be
per-inferior or per-address-space.  But, perhaps we aren't quite ready
for that yet.

Paul> +  objfiles_updated_p += 1;  /* Rebuild section map next time we need it.  */

It seems like we would need one of these in objfile_relocate, and maybe
in reread_symbols.  What do you think?

Paul> +/* Bsearch comparison function. */
Paul> +
Paul> +int
Paul> +find_pc_section_cmp (const void *key, const void *elt)

Should be static.

Tom

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Fri, Jul 17, 2009 at 8:06 AM, Tom Tromey<tromey@redhat.com> wrote:

> Paul> +static int objfiles_updated_p;
>
> This (plus the map itself) seems like another thing that will have to be
> per-inferior or per-address-space.  But, perhaps we aren't quite ready
> for that yet.

Yes, this would definitely need to be modified to be per-address-space.

> Paul> +  objfiles_updated_p += 1;  /* Rebuild section map next time we need it.  */
>
> It seems like we would need one of these in objfile_relocate

Yes, looks like it. Added.

> and maybe in reread_symbols.  What do you think?

Hmm, this one is trickier (I don't want to make objfiles_updated_p global).

I moved the 'objfiles_updated_p += 1' from free_objfile() into
clear_objfile_data() -- that's called from reread_symbols() as well.

> Paul> +int
> Paul> +find_pc_section_cmp (const void *key, const void *elt)
>
> Should be static.

Caught this one in the followup patch :-)

Here is the update.
Re-tested on Linux/x86_64 with no regressions.

Thanks,
-- 
Paul Pluzhnikov

2009-07-17  Paul Pluzhnikov  <ppluzhnikov@google.com>

       * objfiles.c (objfiles_updated_p): New variable.
       (qsort_cmp, bsearch_cmp, update_section_map): New functions.
       (find_pc_section): Use bsearch.

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

Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.85
diff -u -p -u -r1.85 objfiles.c
--- objfiles.c	14 Jul 2009 14:55:06 -0000	1.85
+++ objfiles.c	17 Jul 2009 16:23:30 -0000
@@ -64,6 +64,11 @@ struct objfile *current_objfile;	/* For 
 struct objfile *symfile_objfile;	/* Main symbol table loaded from */
 struct objfile *rt_common_objfile;	/* For runtime common symbols */
 
+/* Records whether any objfiles appeared or disappeared since we last updated
+   address to obj section map.  */
+
+static int objfiles_updated_p;
+
 /* Locate all mappable sections of a BFD file. 
    objfile_p_char is a char * to get it through
    bfd_map_over_sections; we cast it back to its proper type.  */
@@ -94,6 +99,7 @@ add_to_objfile_sections (struct bfd *abf
   obstack_grow (&objfile->objfile_obstack, (char *) &section, sizeof (section));
   objfile->sections_end
     = (struct obj_section *) (((size_t) objfile->sections_end) + 1);
+  objfiles_updated_p += 1;  /* Rebuild section map next time we need it.  */
 }
 
 /* Builds a section table for OBJFILE.
@@ -676,6 +682,7 @@ objfile_relocate (struct objfile *objfil
 
   /* Relocate breakpoints as necessary, after things are relocated. */
   breakpoint_re_set ();
+  objfiles_updated_p += 1;  /* Rebuild section map next time we need it.  */
 }
 \f
 /* Many places in gdb want to test just to see if we have any partial
@@ -757,23 +764,113 @@ have_minimal_symbols (void)
   return 0;
 }
 
+/* Qsort comparison function.  */
+
+static int
+qsort_cmp (const void *a, const void *b)
+{
+  const struct obj_section *sect1 = *(const struct obj_section **) a;
+  const struct obj_section *sect2 = *(const struct obj_section **) b;
+  const CORE_ADDR sect1_addr = obj_section_addr (sect1);
+  const CORE_ADDR sect2_addr = obj_section_addr (sect2);
+
+  if (sect1_addr < sect2_addr)
+    {
+      gdb_assert (obj_section_endaddr (sect1) <= sect2_addr);
+      return -1;
+    }
+  else if (sect1_addr > sect2_addr)
+    {
+      gdb_assert (sect1_addr >= obj_section_endaddr (sect2));
+      return 1;
+    }
+  /* This can happen for separate debug-info files.  */
+  gdb_assert (obj_section_endaddr (sect1) == obj_section_endaddr (sect2));
+
+  return 0;
+}
+
+/* Update PMAP, PMAP_SIZE with non-TLS sections from all objfiles.  */
+
+static void
+update_section_map (struct obj_section ***pmap, int *pmap_size)
+{
+  int map_size, idx;
+  struct obj_section *s, **map;
+  struct objfile *objfile;
+
+  gdb_assert (objfiles_updated_p != 0);
+
+  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;
+  ALL_OBJSECTIONS (objfile, s)
+    if (insert_p (objfile, s))
+      map_size += 1;
+
+  map = xmalloc (map_size * sizeof (*map));
+
+  idx = 0;
+  ALL_OBJSECTIONS (objfile, s)
+    if (insert_p (objfile, s))
+      map[idx++] = s;
+
+#undef insert_p
+
+  qsort (map, map_size, sizeof (*map), qsort_cmp);
+
+  *pmap = map;
+  *pmap_size = map_size;
+}
+
+/* Bsearch comparison function. */
+
+static int
+bsearch_cmp (const void *key, const void *elt)
+{
+  const CORE_ADDR pc = *(CORE_ADDR *) key;
+  const struct obj_section *section = *(const struct obj_section **) elt;
+
+  if (pc < obj_section_addr (section))
+    return -1;
+  if (pc < obj_section_endaddr (section))
+    return 0;
+  return 1;
+}
+
 /* Returns a section whose range includes PC or NULL if none found.   */
 
 struct obj_section *
 find_pc_section (CORE_ADDR pc)
 {
-  struct obj_section *s;
-  struct objfile *objfile;
+  static struct obj_section **sections;
+  static int num_sections;
+
+  struct obj_section *s, **sp;
 
   /* Check for mapped overlay section first.  */
   s = find_pc_mapped_section (pc);
   if (s)
     return s;
 
-  ALL_OBJSECTIONS (objfile, s)
-    if (obj_section_addr (s) <= pc && pc < obj_section_endaddr (s))
-      return s;
+  if (objfiles_updated_p != 0)
+    {
+      update_section_map (&sections, &num_sections);
+
+      /* Don't need updates to section map until objfiles are added
+         or removed.  */
+      objfiles_updated_p = 0;
+    }
 
+  sp = (struct obj_section **) bsearch (&pc, sections, num_sections,
+					sizeof (*sections), bsearch_cmp);
+  if (sp != NULL)
+    return *sp;
   return NULL;
 }
 
@@ -876,6 +973,7 @@ clear_objfile_data (struct objfile *objf
       registration->data->cleanup (objfile, objfile->data[i]);
 
   memset (objfile->data, 0, objfile->num_data * sizeof (void *));
+  objfiles_updated_p += 1;  /* Rebuild section map next time we need it.  */
 }
 
 void

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

On Thu, Jul 16, 2009 at 5:24 PM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:

> Here are user-time results before the patch (clearly showing non-linear
> slowdown):

It turned out that I had another local patch which was unexpectedly slowing
this down.

Here is what the unpatched/patched timing looks like without it:

 NNN       before        after

 256     0m1.042s     0m0.337s
 512     0m6.837s     0m0.887s
1024    0m31.406s     0m4.594s

Sorry about the confusion ...

[There is still another quadratic factor hiding in dwarf2_frame_find_fde,
and it's next on my list.]

-- 
Paul Pluzhnikov

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Fri, Jul 17, 2009 at 10:19 AM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:

>  NNN  before     after
>
>  256  0m1.042s 0m0.337s
>  512  0m6.837s 0m0.887s
> 1024 0m31.406s 0m4.594s

Looks like I managed to confuse myself even more (too many variants of
gen.pl :-(

Above results are for a simplified test case (gen-20090720.pl, attached).

Here are the timings I get using unpatched vs. patched GDB using the
original gen.pl from
http://sourceware.org/ml/gdb-patches/2009-07/msg00417/gen.pl

 NNN     before       after

 256   0m03.544s     0m02.724s
 512   0m15.693s     0m10.025s
1024   1m06.844s     0m39.742s

So not such a drastic improvement for *this* test case.

Most of the time for this test is actually spent in dwarf2_frame_find_fde,
for which I'll send another patch shortly.

Thanks,
-- 
Paul Pluzhnikov

[-- Attachment #2: gen-20090720.pl --]
[-- Type: application/x-perl, Size: 518 bytes --]

Re: [patch] Speed up find_pc_section

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> Hmm, this one is trickier (I don't want to make objfiles_updated_p
Paul> global).

Yeah, good call.

Paul> I moved the 'objfiles_updated_p += 1' from free_objfile() into
Paul> clear_objfile_data() -- that's called from reread_symbols() as well.

I don't like this so much.  clear_objfile_data is a clear,
single-purpose function.  I'm reluctant to add unrelated code to it.

Perhaps a new function would be better.  Or, maybe there is a way to do
this by registering some observers.

Paul> 2009-07-17  Paul Pluzhnikov  <ppluzhnikov@google.com>
Paul>        * objfiles.c (objfiles_updated_p): New variable.
Paul>        (qsort_cmp, bsearch_cmp, update_section_map): New functions.
Paul>        (find_pc_section): Use bsearch.

This should mention the modified functions: add_to_objfile_sections,
objfile_relocate, and clear_objfile_data.


+  objfiles_updated_p += 1;  /* Rebuild section map next time we need it.  */

Since this is just a boolean flag I think a plain assignment is clearer.
An increment leads me to wonder what the count means.

Tom

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Tue, Jul 21, 2009 at 9:06 AM, Tom Tromey<tromey@redhat.com> wrote:

> Paul> I moved the 'objfiles_updated_p += 1' from free_objfile() into
> Paul> clear_objfile_data() -- that's called from reread_symbols() as well.
>
> I don't like this so much. clear_objfile_data is a clear,
> single-purpose function. I'm reluctant to add unrelated code to it.
>
> Perhaps a new function would be better. Or, maybe there is a way to do
> this by registering some observers.

Good call.
I added observers and removed all other places where the flag was set.

> Paul> 2009-07-17  Paul Pluzhnikov  <ppluzhnikov@google.com>
...
> This should mention the modified functions: add_to_objfile_sections,
> objfile_relocate, and clear_objfile_data.

No longer applicable.

> +  objfiles_updated_p += 1;  /* Rebuild section map next time we need it.  */
>
> Since this is just a boolean flag I think a plain assignment is clearer.

Done.

Re-tested on Linux/x86_64 with no new failures.

Thanks,
-- 
Paul Pluzhnikov

2009-07-21  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* objfiles.c (objfiles_updated_p): New variable.
	(qsort_cmp, bsearch_cmp, update_section_map): New function.
	(find_pc_section): Use bsearch.
	(set_objfiles_updated_on_exe_change): New function.
	(set_objfiles_updated_on_solib_activity): New function.
	(_initialize_objfiles): New function.

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

Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.85
diff -u -p -u -r1.85 objfiles.c
--- objfiles.c	14 Jul 2009 14:55:06 -0000	1.85
+++ objfiles.c	21 Jul 2009 20:12:18 -0000
@@ -50,6 +50,7 @@
 #include "addrmap.h"
 #include "arch-utils.h"
 #include "exec.h"
+#include "observer.h"
 
 /* Prototypes for local functions */
 
@@ -64,6 +65,11 @@ struct objfile *current_objfile;	/* For 
 struct objfile *symfile_objfile;	/* Main symbol table loaded from */
 struct objfile *rt_common_objfile;	/* For runtime common symbols */
 
+/* Records whether any objfiles appeared or disappeared since we last updated
+   address to obj section map.  */
+
+static int objfiles_updated_p;
+
 /* Locate all mappable sections of a BFD file. 
    objfile_p_char is a char * to get it through
    bfd_map_over_sections; we cast it back to its proper type.  */
@@ -757,23 +763,113 @@ have_minimal_symbols (void)
   return 0;
 }
 
+/* Qsort comparison function.  */
+
+static int
+qsort_cmp (const void *a, const void *b)
+{
+  const struct obj_section *sect1 = *(const struct obj_section **) a;
+  const struct obj_section *sect2 = *(const struct obj_section **) b;
+  const CORE_ADDR sect1_addr = obj_section_addr (sect1);
+  const CORE_ADDR sect2_addr = obj_section_addr (sect2);
+
+  if (sect1_addr < sect2_addr)
+    {
+      gdb_assert (obj_section_endaddr (sect1) <= sect2_addr);
+      return -1;
+    }
+  else if (sect1_addr > sect2_addr)
+    {
+      gdb_assert (sect1_addr >= obj_section_endaddr (sect2));
+      return 1;
+    }
+  /* This can happen for separate debug-info files.  */
+  gdb_assert (obj_section_endaddr (sect1) == obj_section_endaddr (sect2));
+
+  return 0;
+}
+
+/* Update PMAP, PMAP_SIZE with non-TLS sections from all objfiles.  */
+
+static void
+update_section_map (struct obj_section ***pmap, int *pmap_size)
+{
+  int map_size, idx;
+  struct obj_section *s, **map;
+  struct objfile *objfile;
+
+  gdb_assert (objfiles_updated_p != 0);
+
+  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;
+  ALL_OBJSECTIONS (objfile, s)
+    if (insert_p (objfile, s))
+      map_size += 1;
+
+  map = xmalloc (map_size * sizeof (*map));
+
+  idx = 0;
+  ALL_OBJSECTIONS (objfile, s)
+    if (insert_p (objfile, s))
+      map[idx++] = s;
+
+#undef insert_p
+
+  qsort (map, map_size, sizeof (*map), qsort_cmp);
+
+  *pmap = map;
+  *pmap_size = map_size;
+}
+
+/* Bsearch comparison function. */
+
+static int
+bsearch_cmp (const void *key, const void *elt)
+{
+  const CORE_ADDR pc = *(CORE_ADDR *) key;
+  const struct obj_section *section = *(const struct obj_section **) elt;
+
+  if (pc < obj_section_addr (section))
+    return -1;
+  if (pc < obj_section_endaddr (section))
+    return 0;
+  return 1;
+}
+
 /* Returns a section whose range includes PC or NULL if none found.   */
 
 struct obj_section *
 find_pc_section (CORE_ADDR pc)
 {
-  struct obj_section *s;
-  struct objfile *objfile;
+  static struct obj_section **sections;
+  static int num_sections;
+
+  struct obj_section *s, **sp;
 
   /* Check for mapped overlay section first.  */
   s = find_pc_mapped_section (pc);
   if (s)
     return s;
 
-  ALL_OBJSECTIONS (objfile, s)
-    if (obj_section_addr (s) <= pc && pc < obj_section_endaddr (s))
-      return s;
+  if (objfiles_updated_p != 0)
+    {
+      update_section_map (&sections, &num_sections);
+
+      /* Don't need updates to section map until objfiles are added
+         or removed.  */
+      objfiles_updated_p = 0;
+    }
 
+  sp = (struct obj_section **) bsearch (&pc, sections, num_sections,
+					sizeof (*sections), bsearch_cmp);
+  if (sp != NULL)
+    return *sp;
   return NULL;
 }
 
@@ -892,3 +988,29 @@ objfile_data (struct objfile *objfile, c
   gdb_assert (data->index < objfile->num_data);
   return objfile->data[data->index];
 }
+
+/* Set objfiles_updated_p so section map will be rebuilt next time it
+   is used.  Called by executable_changed observer.  */
+
+static void
+set_objfiles_updated_on_exe_change (void)
+{
+  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
+}
+
+/* Set objfiles_updated_p so section map will be rebuilt next time it
+   is used.  Called by solib_loaded/unloaded observer.  */
+
+static void
+set_objfiles_updated_on_solib_activity (struct so_list *so_list)
+{
+  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
+}
+
+void
+_initialize_objfiles (void)
+{
+  observer_attach_executable_changed (set_objfiles_updated_on_exe_change);
+  observer_attach_solib_loaded (set_objfiles_updated_on_solib_activity);
+  observer_attach_solib_unloaded (set_objfiles_updated_on_solib_activity);
+}

Re: [patch] Speed up find_pc_section

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> 2009-07-21  Paul Pluzhnikov  <ppluzhnikov@google.com>
Paul> 	* objfiles.c (objfiles_updated_p): New variable.
Paul> 	(qsort_cmp, bsearch_cmp, update_section_map): New function.
Paul> 	(find_pc_section): Use bsearch.
Paul> 	(set_objfiles_updated_on_exe_change): New function.
Paul> 	(set_objfiles_updated_on_solib_activity): New function.
Paul> 	(_initialize_objfiles): New function.

This is ok, thanks.

Tom

Re: [patch] Speed up find_pc_section

[Pedro Alves]

On Tuesday 21 July 2009 21:18:26, Paul Pluzhnikov wrote:

> +
> +/* Set objfiles_updated_p so section map will be rebuilt next time it
> +   is used.  Called by executable_changed observer.  */
> +
> +static void
> +set_objfiles_updated_on_exe_change (void)
> +{
> +  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
> +}
> +
> +/* Set objfiles_updated_p so section map will be rebuilt next time it
> +   is used.  Called by solib_loaded/unloaded observer.  */
> +
> +static void
> +set_objfiles_updated_on_solib_activity (struct so_list *so_list)
> +{
> +  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
> +}
> +
> +void
> +_initialize_objfiles (void)
> +{
> +  observer_attach_executable_changed (set_objfiles_updated_on_exe_change);
> +  observer_attach_solib_loaded (set_objfiles_updated_on_solib_activity);
> +  observer_attach_solib_unloaded (set_objfiles_updated_on_solib_activity);
> +}

Are these observers sufficient?  There are other sources of
objfiles !exec && !solibs.  What about e.g., add-symbol-file?  E.g.:

$ gdb
(gdb) add-symbol-file fooprogram 0x1234 (-s ... ... ...)
(gdb) p foosymbol

I don't think the map will be built, thus the section by pc won't
be found.

I'm also looking at objfile_relocate (from remote targets using
qOffsets for example), and wondering where is the map reset?

-- 
Pedro Alves

Re: [patch] Speed up find_pc_section

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> I'm also looking at objfile_relocate (from remote targets using
Pedro> qOffsets for example), and wondering where is the map reset?

Yeah, whoops.  And here I even caught that the first time around :(

Paul, could you please either fix this ASAP, or revert the change while
coming up with a fix?  Thanks.

Tom

Re: [patch] Speed up find_pc_section

[Pedro Alves]

On Wednesday 22 July 2009 16:49:39, Paul Pluzhnikov wrote:
> On Wed, Jul 22, 2009 at 7:25 AM, Tom Tromey<tromey@redhat.com> wrote:
> 
> > Paul, could you please either fix this ASAP, or revert the change while
> > coming up with a fix?  Thanks.
> 
> Here is the fix for both issues.
> Tested on Linux/x86_64, no regressions.
> 
> I'll work up a test case for add-symbol-file in a separate patch.
> Not sure if I can make a test case for objfile_relocate though.
> 
> Thanks,

Thanks, this looks better, but, there are a few issues I'd like to point out.

solib.c:

	  /* Notify any observer that the shared object has been
	     unloaded before we remove it from GDB's tables.  */
	  observer_notify_solib_unloaded (gdb);

	  *gdb_link = gdb->next;

	  /* Unless the user loaded it explicitly, free SO's objfile.  */
	  if (gdb->objfile && ! (gdb->objfile->flags & OBJF_USERLOADED))
	    free_objfile (gdb->objfile);

In the OBJF_USERLOADED case, you rebuild the map when you don't
really need to.

You're also likely breaking xcoffsolib.c and the vmap code in exec.c, as
it calls free_objfile without going through observers.

Given the new_objfile observer, do we still need the exec_changed and
solib observers?  It sounds strange to me that this objfiles.c private
cache needs observers from *other* modules.

Please look around and check that objfile->section_offsets or
obj_section_offset isn't being used as an lvalue somewhere where
the map cache isn't being flushed.

-- 
Pedro Alves

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Wed, Jul 22, 2009 at 7:25 AM, Tom Tromey<tromey@redhat.com> wrote:

> Paul, could you please either fix this ASAP, or revert the change while
> coming up with a fix?  Thanks.

Here is the fix for both issues.
Tested on Linux/x86_64, no regressions.

I'll work up a test case for add-symbol-file in a separate patch.
Not sure if I can make a test case for objfile_relocate though.

Thanks,
-- 
Paul Pluzhnikov


2009-07-22  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* objfiles.c (objfile_relocate): Must rebuild section map.
	(set_objfiles_updated_on_new_objfile): New observer.
	(_initialize_objfiles): Register it.

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

Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.86
diff -u -p -u -r1.86 objfiles.c
--- objfiles.c	21 Jul 2009 20:54:30 -0000	1.86
+++ objfiles.c	22 Jul 2009 15:43:47 -0000
@@ -682,6 +682,7 @@ objfile_relocate (struct objfile *objfil
 
   /* Relocate breakpoints as necessary, after things are relocated. */
   breakpoint_re_set ();
+  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
 }
 \f
 /* Many places in gdb want to test just to see if we have any partial
@@ -1007,10 +1008,20 @@ set_objfiles_updated_on_solib_activity (
   objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
 }
 
+/* Set objfiles_updated_p so section map will be rebuilt next time it
+   is used.  Called by new_objfile observer.  */
+
+static void
+set_objfiles_updated_on_new_objfile (struct objfile *objfile)
+{
+  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
+}
+
 void
 _initialize_objfiles (void)
 {
   observer_attach_executable_changed (set_objfiles_updated_on_exe_change);
+  observer_attach_new_objfile (set_objfiles_updated_on_new_objfile);
   observer_attach_solib_loaded (set_objfiles_updated_on_solib_activity);
   observer_attach_solib_unloaded (set_objfiles_updated_on_solib_activity);
 }

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Wed, Jul 22, 2009 at 9:32 AM, Pedro Alves<pedro@codesourcery.com> wrote:

> In the OBJF_USERLOADED case, you rebuild the map when you don't
> really need to.

I think it's pretty clear that what I really need is a new 'remove_objfile'
kind of observer.

> You're also likely breaking xcoffsolib.c and the vmap code in exec.c, as
> it calls free_objfile without going through observers.

This would be handled by the 'remove_objfile' observer, I think.

> Given the new_objfile observer, do we still need the exec_changed and
> solib observers?

No, I've removed that (new patch attached).

> Please look around and check that objfile->section_offsets or
> obj_section_offset isn't being used as an lvalue somewhere where
> the map cache isn't being flushed.

I think new_objfile/remove_objfile will take care of any of these.

Should I revert the find_pc_section patch, work up remove_objfile observer
patch, then re-apply find_pc_section patch once it's in better shape?

Thanks,
-- 
Paul Pluzhnikov

2009-07-22  Paul Pluzhnikov  <ppluzhnikov@google.com>

       * objfiles.c (objfile_relocate): Must rebuild section map.
       (set_objfiles_updated_on_exe_change): Remove.
       (set_objfiles_updated_on_new_objfile): New observer.
       (_initialize_objfiles): Adjust.

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

Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.86
diff -u -p -u -r1.86 objfiles.c
--- objfiles.c	21 Jul 2009 20:54:30 -0000	1.86
+++ objfiles.c	22 Jul 2009 17:12:52 -0000
@@ -682,6 +682,7 @@ objfile_relocate (struct objfile *objfil
 
   /* Relocate breakpoints as necessary, after things are relocated. */
   breakpoint_re_set ();
+  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
 }
 \f
 /* Many places in gdb want to test just to see if we have any partial
@@ -990,19 +991,19 @@ objfile_data (struct objfile *objfile, c
 }
 
 /* Set objfiles_updated_p so section map will be rebuilt next time it
-   is used.  Called by executable_changed observer.  */
+   is used.  Called by solib_loaded/unloaded observer.  */
 
 static void
-set_objfiles_updated_on_exe_change (void)
+set_objfiles_updated_on_solib_activity (struct so_list *so_list)
 {
   objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
 }
 
 /* Set objfiles_updated_p so section map will be rebuilt next time it
-   is used.  Called by solib_loaded/unloaded observer.  */
+   is used.  Called by new_objfile observer.  */
 
 static void
-set_objfiles_updated_on_solib_activity (struct so_list *so_list)
+set_objfiles_updated_on_new_objfile (struct objfile *objfile)
 {
   objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
 }
@@ -1010,7 +1011,7 @@ set_objfiles_updated_on_solib_activity (
 void
 _initialize_objfiles (void)
 {
-  observer_attach_executable_changed (set_objfiles_updated_on_exe_change);
+  observer_attach_new_objfile (set_objfiles_updated_on_new_objfile);
   observer_attach_solib_loaded (set_objfiles_updated_on_solib_activity);
   observer_attach_solib_unloaded (set_objfiles_updated_on_solib_activity);
 }

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

On Wed, Jul 22, 2009 at 10:16 AM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:
> On Wed, Jul 22, 2009 at 9:32 AM, Pedro Alves<pedro@codesourcery.com> wrote:
>
>> Given the new_objfile observer, do we still need the exec_changed and
>> solib observers?
>
> No, I've removed that (new patch attached).

Um, sorry.

We don't need exec_changed observer (removed), and perhaps solib_loaded one
(I am not 100% sure), but solib_unload observer is still needed, I think.

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Speed up find_pc_section

[Pedro Alves]

On Wednesday 22 July 2009 18:16:06, Paul Pluzhnikov wrote:
> On Wed, Jul 22, 2009 at 9:32 AM, Pedro Alves<pedro@codesourcery.com> wrote:
> 
> > In the OBJF_USERLOADED case, you rebuild the map when you don't
> > really need to.
> 
> I think it's pretty clear that what I really need is a new 'remove_objfile'
> kind of observer.

Would setting objfiles_updated_p from e.g., unlink_objfile work?
Both the map and unlink_objfile/free_objfile are private/implemented in
objfiles.c; if nothing else outside needs the observer, we can avoid
adding it.  Otherwise, a new observer is fine.  (the more transparent
the cache is, the better the design looks, IMHO)

> > You're also likely breaking xcoffsolib.c and the vmap code in exec.c, as
> > it calls free_objfile without going through observers.
> 
> This would be handled by the 'remove_objfile' observer, I think.

I think so.

> 
> > Given the new_objfile observer, do we still need the exec_changed and
> > solib observers?
> 
> No, I've removed that (new patch attached).

I think that the need for the solib load/unload observer
would go away too if the map is flushed on objfile
removal/freeing/unlinking as well?  Why would you need to know about
solibs, if without the cache (that is, before the original patch)
we'd just iterate over the objfiles' sections?  That is, the lifetime
of the cache should be at most the same as of the objfiles'
sections, yes?

Come to look at it deeper, what is happening with
symbol_file_add_with_addrs_or_offsets, if the objfile has only
minimal symbols (but still has sections)?  allocate_objfile is called,
which builds the section table, but, there's a path that does an
early return before calling the new_objfile observer, if there are
no symbols.

> > Please look around and check that objfile->section_offsets or
> > obj_section_offset isn't being used as an lvalue somewhere where
> > the map cache isn't being flushed.
> 
> I think new_objfile/remove_objfile will take care of any of these.

Okay then.

> Should I revert the find_pc_section patch, work up remove_objfile observer
> patch, then re-apply find_pc_section patch once it's in better shape?

It would be cleaner and easier to review, easier for you (I think),
and better for the archives in the future, IMHO.  But I don't mind
much if a new patch is cooked on top.

-- 
Pedro Alves

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Wed, Jul 22, 2009 at 10:16 AM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:

> On Wed, Jul 22, 2009 at 9:32 AM, Pedro Alves<pedro@codesourcery.com> wrote:
>
>> In the OBJF_USERLOADED case, you rebuild the map when you don't
>> really need to.
>
> I think it's pretty clear that what I really need is a new 'remove_objfile'
> kind of observer.

I am not thinking clearly today :-(

I don't need a new observer: free_objfile is in the same source, so I just
need to set the flag there as well.

>> You're also likely breaking xcoffsolib.c and the vmap code in exec.c, as
>> it calls free_objfile without going through observers.
>
> This would be handled by the 'remove_objfile' observer, I think.
>
>> Given the new_objfile observer, do we still need the exec_changed and
>> solib observers?
>
> No, I've removed that (new patch attached).

I feel going in circles here. The exec_changed observer was to address
reread_symbols, which doesn't create a new objfile. I believe that's
still necessary.

The solib_load/unload observers don't appear to be needed though: the load
case will create a new objfile, the unload case (when !OBJF_USERLOADED)
will do free_objfile).

On Wed, Jul 22, 2009 at 10:40 AM, Pedro Alves<pedro@codesourcery.com> wrote:

>> I think it's pretty clear that what I really need is a new 'remove_objfile'
>> kind of observer.
>
> Would setting objfiles_updated_p from e.g., unlink_objfile work?

Exactly. Though I think free_objfile is a more logical place for it.

> I think that the need for the solib load/unload observer
> would go away too if the map is flushed on objfile
> removal/freeing/unlinking as well?

Yes.

> Come to look at it deeper, what is happening with
> symbol_file_add_with_addrs_or_offsets, if the objfile has only
> minimal symbols (but still has sections)?  allocate_objfile is called,
> which builds the section table, but, there's a path that does an
> early return before calling the new_objfile observer, if there are
> no symbols.

That sounds like a bug: we created a new_objfile, but didn't notify
observers.

Eeasy enough to work around though: I'll set the flag in allocate_objfile
as well.

> It would be cleaner and easier to review, easier for you (I think),
> and better for the archives in the future, IMHO.  But I don't mind
> much if a new patch is cooked on top.

Let's try for one more fix before reverting ...

Re-tested on Linux/x86_64 with no new failures.

Thanks,
-- 
Paul Pluzhnikov

2009-07-22  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* objfiles.c (allocate_objfile): Must rebuild section map.
	(free_objfile, objfile_relocate): Likewise.
	(set_objfiles_updated_on_solib_activity): Remove.
	(_initialize_objfiles): Adjust.

[-- Attachment #2: gdb-find_pc_section-20090722-3.txt --]
[-- Type: text/plain, Size: 1766 bytes --]

Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.86
diff -u -p -u -r1.86 objfiles.c
--- objfiles.c	21 Jul 2009 20:54:30 -0000	1.86
+++ objfiles.c	22 Jul 2009 17:55:41 -0000
@@ -235,6 +235,8 @@ allocate_objfile (bfd *abfd, int flags)
   /* Save passed in flag bits. */
   objfile->flags |= flags;
 
+  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
+
   return (objfile);
 }
 
@@ -501,6 +503,7 @@ free_objfile (struct objfile *objfile)
   obstack_free (&objfile->objfile_obstack, 0);
   xfree (objfile);
   objfile = NULL;
+  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
 }
 
 static void
@@ -682,6 +685,7 @@ objfile_relocate (struct objfile *objfil
 
   /* Relocate breakpoints as necessary, after things are relocated. */
   breakpoint_re_set ();
+  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
 }
 \f
 /* Many places in gdb want to test just to see if we have any partial
@@ -998,19 +1002,8 @@ set_objfiles_updated_on_exe_change (void
   objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
 }
 
-/* Set objfiles_updated_p so section map will be rebuilt next time it
-   is used.  Called by solib_loaded/unloaded observer.  */
-
-static void
-set_objfiles_updated_on_solib_activity (struct so_list *so_list)
-{
-  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
-}
-
 void
 _initialize_objfiles (void)
 {
   observer_attach_executable_changed (set_objfiles_updated_on_exe_change);
-  observer_attach_solib_loaded (set_objfiles_updated_on_solib_activity);
-  observer_attach_solib_unloaded (set_objfiles_updated_on_solib_activity);
 }

Re: [patch] Speed up find_pc_section

[Pedro Alves]

On Wednesday 22 July 2009 19:10:48, Paul Pluzhnikov wrote:
> On Wed, Jul 22, 2009 at 10:16 AM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:
> 
> > On Wed, Jul 22, 2009 at 9:32 AM, Pedro Alves<pedro@codesourcery.com> wrote:
> >
> >> In the OBJF_USERLOADED case, you rebuild the map when you don't
> >> really need to.
> >
> > I think it's pretty clear that what I really need is a new 'remove_objfile'
> > kind of observer.
> 
> I am not thinking clearly today :-(

:-)

> I don't need a new observer: free_objfile is in the same source, so I just
> need to set the flag there as well.

Bingo.

> I feel going in circles here. The exec_changed observer was to address
> reread_symbols, which doesn't create a new objfile. I believe that's
> still necessary.

Ah!  I see.  That, is pretty ugly/nasty.  I think that objfiles.c would
be a better home for reread_symbols, which would also remove that
requirement.  A new function objfiles.c:objfiles_changed that is called
from reread_symbols, would still be better, that abusing that
observer, IMHO.  Do you feel like adding it?  If not, I won't insist;
the observer is fine for now, *if* you do something for me please:  Could
you please add a comment in _initialize_objfiles explaining that why
it is needed, and spell out "reread_symbols" and "objfile_updated_p" in
the comment, so that grepping finds it.

> The solib_load/unload observers don't appear to be needed though: the load
> case will create a new objfile, the unload case (when !OBJF_USERLOADED)
> will do free_objfile).

Exactly.

> 
> On Wed, Jul 22, 2009 at 10:40 AM, Pedro Alves<pedro@codesourcery.com> wrote:
> 
> >> I think it's pretty clear that what I really need is a new 'remove_objfile'
> >> kind of observer.
> >
> > Would setting objfiles_updated_p from e.g., unlink_objfile work?
> 
> Exactly. Though I think free_objfile is a more logical place for it.

I was thinking that when you look for a section, you loop over linked in
objfiles, while free_objfile could in principle also be called even
if the objfile hadn't been linked in.  But that's a minor detail;
free_objfile is fine.

> > I think that the need for the solib load/unload observer
> > would go away too if the map is flushed on objfile
> > removal/freeing/unlinking as well?
> 
> Yes.

Great.

> 
> > Come to look at it deeper, what is happening with
> > symbol_file_add_with_addrs_or_offsets, if the objfile has only
> > minimal symbols (but still has sections)?  allocate_objfile is called,
> > which builds the section table, but, there's a path that does an
> > early return before calling the new_objfile observer, if there are
> > no symbols.
> 
> That sounds like a bug: we created a new_objfile, but didn't notify
> observers.

Indeed.

> 
> Eeasy enough to work around though: I'll set the flag in allocate_objfile
> as well.
> 
> > It would be cleaner and easier to review, easier for you (I think),
> > and better for the archives in the future, IMHO.  But I don't mind
> > much if a new patch is cooked on top.
> 
> Let's try for one more fix before reverting ...
> 
> Re-tested on Linux/x86_64 with no new failures.

Looks good to me now (minus missing comment).  Thank you!

-- 
Pedro Alves

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Wed, Jul 22, 2009 at 11:34 AM, Pedro Alves<pedro@codesourcery.com> wrote:

> A new function objfiles.c:objfiles_changed that is called
> from reread_symbols, would still be better, that abusing that
> observer, IMHO.

I agree. Try #4 tested and attached.

Thanks,
-- 
Paul Pluzhnikov

2009-07-22  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* objfiles.h (objfiles_changed): New prototype.
        * objfiles.c (objfiles_updated_p): Rename to objfiles_changed_p.
	(allocate_objfile, free_objfile): Must rebuild section map.
        (objfile_relocate): Likewise.
	(update_section_map, find_pc_section): Adjust.
	(set_objfiles_updated_on_exe_change): Remove.
       	(set_objfiles_updated_on_solib_activity): Remove.
       	(_initialize_objfiles): Remove.
	(objfiles_changed): New function.
	* symfile.c (reread_symbols): Call objfiles_changed.

[-- Attachment #2: gdb-find_pc_section-20090722-4.txt --]
[-- Type: text/plain, Size: 4422 bytes --]

Index: objfiles.h
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.h,v
retrieving revision 1.59
diff -u -p -u -r1.59 objfiles.h
--- objfiles.h	15 Jan 2009 16:35:22 -0000	1.59
+++ objfiles.h	22 Jul 2009 18:48:36 -0000
@@ -482,6 +482,8 @@ extern int have_partial_symbols (void);
 
 extern int have_full_symbols (void);
 
+extern void objfiles_changed (void);
+
 /* This operation deletes all objfile entries that represent solibs that
    weren't explicitly loaded by the user, via e.g., the add-symbol-file
    command.
Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.86
diff -u -p -u -r1.86 objfiles.c
--- objfiles.c	21 Jul 2009 20:54:30 -0000	1.86
+++ objfiles.c	22 Jul 2009 18:48:36 -0000
@@ -68,7 +68,7 @@ struct objfile *rt_common_objfile;	/* Fo
 /* Records whether any objfiles appeared or disappeared since we last updated
    address to obj section map.  */
 
-static int objfiles_updated_p;
+static int objfiles_changed_p;
 
 /* Locate all mappable sections of a BFD file. 
    objfile_p_char is a char * to get it through
@@ -235,6 +235,8 @@ allocate_objfile (bfd *abfd, int flags)
   /* Save passed in flag bits. */
   objfile->flags |= flags;
 
+  objfiles_changed_p = 1;  /* Rebuild section map next time we need it.  */
+
   return (objfile);
 }
 
@@ -501,6 +503,7 @@ free_objfile (struct objfile *objfile)
   obstack_free (&objfile->objfile_obstack, 0);
   xfree (objfile);
   objfile = NULL;
+  objfiles_changed_p = 1;  /* Rebuild section map next time we need it.  */
 }
 
 static void
@@ -682,6 +685,7 @@ objfile_relocate (struct objfile *objfil
 
   /* Relocate breakpoints as necessary, after things are relocated. */
   breakpoint_re_set ();
+  objfiles_changed_p = 1;  /* Rebuild section map next time we need it.  */
 }
 \f
 /* Many places in gdb want to test just to see if we have any partial
@@ -798,7 +802,7 @@ update_section_map (struct obj_section *
   struct obj_section *s, **map;
   struct objfile *objfile;
 
-  gdb_assert (objfiles_updated_p != 0);
+  gdb_assert (objfiles_changed_p != 0);
 
   map = *pmap;
   xfree (map);
@@ -857,13 +861,13 @@ find_pc_section (CORE_ADDR pc)
   if (s)
     return s;
 
-  if (objfiles_updated_p != 0)
+  if (objfiles_changed_p != 0)
     {
       update_section_map (&sections, &num_sections);
 
       /* Don't need updates to section map until objfiles are added
          or removed.  */
-      objfiles_updated_p = 0;
+      objfiles_changed_p = 0;
     }
 
   sp = (struct obj_section **) bsearch (&pc, sections, num_sections,
@@ -989,28 +993,11 @@ objfile_data (struct objfile *objfile, c
   return objfile->data[data->index];
 }
 
-/* Set objfiles_updated_p so section map will be rebuilt next time it
-   is used.  Called by executable_changed observer.  */
-
-static void
-set_objfiles_updated_on_exe_change (void)
-{
-  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
-}
-
-/* Set objfiles_updated_p so section map will be rebuilt next time it
-   is used.  Called by solib_loaded/unloaded observer.  */
-
-static void
-set_objfiles_updated_on_solib_activity (struct so_list *so_list)
-{
-  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
-}
+/* Set objfiles_changed_p so section map will be rebuilt next time it
+   is used.  Called by reread_symbols.  */
 
 void
-_initialize_objfiles (void)
+objfiles_changed (void)
 {
-  observer_attach_executable_changed (set_objfiles_updated_on_exe_change);
-  observer_attach_solib_loaded (set_objfiles_updated_on_solib_activity);
-  observer_attach_solib_unloaded (set_objfiles_updated_on_solib_activity);
+  objfiles_changed_p = 1;  /* Rebuild section map next time we need it.  */
 }
Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.237
diff -u -p -u -r1.237 symfile.c
--- symfile.c	2 Jul 2009 17:25:58 -0000	1.237
+++ symfile.c	22 Jul 2009 18:48:37 -0000
@@ -2457,8 +2457,10 @@ reread_symbols (void)
       /* At least one objfile has changed, so we can consider that
          the executable we're debugging has changed too.  */
       observer_notify_executable_changed ();
+
+      /* Notify objfiles that we've modified objfile sections.  */
+      objfiles_changed ();
     }
-      
 }
 
 

Re: [patch] Speed up find_pc_section

[Pedro Alves]

On Wednesday 22 July 2009 20:05:10, Paul Pluzhnikov wrote:
> 2009-07-22  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>         * objfiles.h (objfiles_changed): New prototype.
>         * objfiles.c (objfiles_updated_p): Rename to objfiles_changed_p.
>         (allocate_objfile, free_objfile): Must rebuild section map.
>         (objfile_relocate): Likewise.
>         (update_section_map, find_pc_section): Adjust.
>         (set_objfiles_updated_on_exe_change): Remove.
>         (set_objfiles_updated_on_solib_activity): Remove.
>         (_initialize_objfiles): Remove.
>         (objfiles_changed): New function.
>         * symfile.c (reread_symbols): Call objfiles_changed.

Ok.  Thanks again!

-- 
Pedro Alves

Re: [patch] Speed up find_pc_section

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Hi Paul.

Paul> 2009-07-21  Paul Pluzhnikov  <ppluzhnikov@google.com>
Paul> 	* objfiles.c (objfiles_updated_p): New variable.
Paul> 	(qsort_cmp, bsearch_cmp, update_section_map): New function.
Paul> 	(find_pc_section): Use bsearch.
Paul> 	(set_objfiles_updated_on_exe_change): New function.
Paul> 	(set_objfiles_updated_on_solib_activity): New function.
Paul> 	(_initialize_objfiles): New function.

Paul> +static int
Paul> +qsort_cmp (const void *a, const void *b)

[...]

Paul> +  /* This can happen for separate debug-info files.  */
Paul> +  gdb_assert (obj_section_endaddr (sect1) == obj_section_endaddr (sect2));

I managed to trigger this assert yesterday.

I was debugging a gcj-compiled program on F11.  I used the system gcj,
and I had the libgcj debuginfo installed.  The libgcj separate debuginfo
had a section with the same start address but a different length as a
section in libgcj itself.

Would you mind looking at this and figuring out how to fix it?  If you
need more information, I can provide it -- either details from debugging
or instructions on reproducing if you have F11 available.

Tom

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

On Tue, Aug 4, 2009 at 7:22 AM, Tom Tromey<tromey@redhat.com> wrote:

> Paul> +  /* This can happen for separate debug-info files.  */
> Paul> +  gdb_assert (obj_section_endaddr (sect1) == obj_section_endaddr (sect2));
>
> I managed to trigger this assert yesterday.

:-(

I think this means that find_pc_section was giving possibly incorrect
answer before this patch.

> Would you mind looking at this and figuring out how to fix it?  If you
> need more information, I can provide it -- either details from debugging
> or instructions on reproducing if you have F11 available.

I do have F11 available, so please give reproducing instructions.

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Speed up find_pc_section

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> I do have F11 available, so please give reproducing instructions.

Make sure you have gcj installed.

Install the libgcj debuginfo.  I did:

    sudo debuginfo-install libgcj

This will install an amazing amount of stuff.

Then write a simple java program, like:

public class q {
  public static void main(String[] args) {
    System.out.println("hi");
  }
}

Compile:

  gcj -g --main=q -o q q.java

Run gdb, "break q.main", "run".

On my machine this is enough to provoke the crash.

Tom

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

For those following the issue on the list ...

I was able to reproduce the failed assert on F11/i586, but only after the
system ran for a while.

On Fri, Aug 7, 2009 at 12:30 AM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:

> It looks like lack of prelink (freshly-installed system) is what
> prevented me from observing the crash:
>
> $ readelf -S /usr/lib/debug/usr/lib/libgcj.so.10.0.0.debug | grep .rel.dyn
>  [ 7] .rel.dyn NOBITS   005e1c84 000138 4ee290 08   A  3   0  4
>
> $ readelf -S /usr/lib/libgcj.so.10.0.0  | grep rel.dyn
>  [ 7] .rel.dyn  RELA    415e1c84 5e1c84 7653d8 0c   A  3   0  4
>
> My libgcj has been prelinked, and I now see the crash.
>
> Prelink changes .rel.dyn from REL to RELA, and that changes the section size!
>
> Running 'prelink -u /usr/lib/libgcj.so.10.0.0' makes the crash
> disappear, and re-prelinking makes it appear again.
> Here is what non-prelinked /usr/lib/libgcj.so.10.0.0 looks like:
>  [ 7] .rel.dyn  REL  005e1c84 5e1c84 4ee290 08   A  3   0  4
>
> I'll send a patch to fix this soon.


-- 
Paul Pluzhnikov

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Fri, Aug 7, 2009 at 3:29 PM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:

> I was able to reproduce the failed assert on F11/i586, but only after the
> system ran for a while.

Update: running 'make check' on F11 after prelinking shows many
similar failures. The "java setup" in Tom's repro instructions doesn't
appear necessary.

Here is a fix. Tested on F11/i586 (with prelinking) and Linux/x86_64 (without).

Thanks,
-- 
Paul Pluzhnikov

2009-08-08  Paul Pluzhnikov  <ppluzhnikov@google.com>

        * objfiles.c (qsort_cmp): Remove assert.
        (preferred_obj_section): New function.
        (update_section_map): Filter duplicates.

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

Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.89
diff -p -u -r1.89 objfiles.c
--- objfiles.c	4 Aug 2009 18:46:05 -0000	1.89
+++ objfiles.c	8 Aug 2009 17:55:54 -0000
@@ -797,18 +797,36 @@ qsort_cmp (const void *a, const void *b)
       gdb_assert (sect1_addr >= obj_section_endaddr (sect2));
       return 1;
     }
-  /* This can happen for separate debug-info files.  */
-  gdb_assert (obj_section_endaddr (sect1) == obj_section_endaddr (sect2));
 
   return 0;
 }
 
+/* Select "better" obj_section to keep.  We prefer the one that came from
+   the real object, rather than the one from separate debuginfo.
+   Most of the time the two sections are exactly identical, but with
+   prelinking the .rel.dyn section in the real object may have different
+   size.  */
+
+static struct obj_section *
+preferred_obj_section (struct obj_section *a, struct obj_section *b)
+{
+  gdb_assert (obj_section_addr (a) == obj_section_addr (b));
+  gdb_assert ((a->objfile->separate_debug_objfile == b->objfile)
+	      || (b->objfile->separate_debug_objfile == a->objfile));
+  gdb_assert ((a->objfile->separate_debug_objfile_backlink == b->objfile)
+	      || (b->objfile->separate_debug_objfile_backlink == a->objfile));
+
+  if (a->objfile->separate_debug_objfile != NULL)
+    return a;
+  return b;
+}
+
 /* Update PMAP, PMAP_SIZE with non-TLS sections from all objfiles.  */
 
 static void
 update_section_map (struct obj_section ***pmap, int *pmap_size)
 {
-  int map_size, idx;
+  int map_size, i, j;
   struct obj_section *s, **map;
   struct objfile *objfile;
 
@@ -828,15 +846,45 @@ update_section_map (struct obj_section *
 
   map = xmalloc (map_size * sizeof (*map));
 
-  idx = 0;
+  i = 0;
   ALL_OBJSECTIONS (objfile, s)
     if (insert_p (objfile, s))
-      map[idx++] = s;
+      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.  */
+    }
+  else
+    gdb_assert (j == map_size);
+
   *pmap = map;
   *pmap_size = map_size;
 }

Re: [patch] Speed up find_pc_section

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> Update: running 'make check' on F11 after prelinking shows many
Paul> similar failures. The "java setup" in Tom's repro instructions
Paul> doesn't appear necessary.

Paul> Here is a fix. Tested on F11/i586 (with prelinking) and
Paul> Linux/x86_64 (without).

Thanks for addressing this so quickly.

This patch is ok.

Can I trouble you to also look at the related bug reported by Twist on
the gdb list?  This one happens on Darwin; I don't know whether Darwin
has prelink or not...

http://sourceware.org/ml/gdb/2009-08/msg00056.html

Tom

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

On Mon, Aug 10, 2009 at 10:58 AM, Tom Tromey<tromey@redhat.com> wrote:

> This patch is ok.

So committed.

> Can I trouble you to also look at the related bug reported by Twist on
> the gdb list?

Will do.

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Speed up find_pc_section

[Ulrich Weigand]

Paul Pluzhnikov wrote:

> 2009-08-08  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>         * objfiles.c (qsort_cmp): Remove assert.
>         (preferred_obj_section): New function.
>         (update_section_map): Filter duplicates.

This patch breaks overlay support:

+static struct obj_section *
+preferred_obj_section (struct obj_section *a, struct obj_section *b)
+{
+  gdb_assert (obj_section_addr (a) == obj_section_addr (b));
+  gdb_assert ((a->objfile->separate_debug_objfile == b->objfile)
+	      || (b->objfile->separate_debug_objfile == a->objfile));
+  gdb_assert ((a->objfile->separate_debug_objfile_backlink == b->objfile)
+	      || (b->objfile->separate_debug_objfile_backlink == a->objfile));
+
+  if (a->objfile->separate_debug_objfile != NULL)
+    return a;
+  return b;
+}

+  /* 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;
+	}
+    }

The patch appears to assume that two sections with the same start
address must be from a separate debuginfo file.  Of course, in the
presence of overlays, you may have multiple regular sections with
the same start address -- that's the point of overlays :-)

These now always run into the assert above.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] Speed up find_pc_section

[Ulrich Weigand]

I wrote:
> Paul Pluzhnikov wrote:
> 
> > 2009-08-08  Paul Pluzhnikov  <ppluzhnikov@google.com>
> > 
> >         * objfiles.c (qsort_cmp): Remove assert.
> >         (preferred_obj_section): New function.
> >         (update_section_map): Filter duplicates.
> 
> This patch breaks overlay support:

This part also doesn't really work with overlay sections:

static int
qsort_cmp (const void *a, const void *b)
{
  const struct obj_section *sect1 = *(const struct obj_section **) a;
  const struct obj_section *sect2 = *(const struct obj_section **) b;
  const CORE_ADDR sect1_addr = obj_section_addr (sect1);
  const CORE_ADDR sect2_addr = obj_section_addr (sect2);

  if (sect1_addr < sect2_addr)
    {
      gdb_assert (obj_section_endaddr (sect1) <= sect2_addr);
      return -1;
    }
  else if (sect1_addr > sect2_addr)
    {
      gdb_assert (sect1_addr >= obj_section_endaddr (sect2));
      return 1;
    }

Section can partially overlap with overlays ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

[commit] Fix reread_symbols crash (Re: [patch] Speed up find_pc_section)

[Ulrich Weigand]

> 2009-07-22  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	* objfiles.h (objfiles_changed): New prototype.
>         * objfiles.c (objfiles_updated_p): Rename to objfiles_changed_p.
> 	(allocate_objfile, free_objfile): Must rebuild section map.
>         (objfile_relocate): Likewise.
> 	(update_section_map, find_pc_section): Adjust.
> 	(set_objfiles_updated_on_exe_change): Remove.
>        	(set_objfiles_updated_on_solib_activity): Remove.
>        	(_initialize_objfiles): Remove.
> 	(objfiles_changed): New function.
> 	* symfile.c (reread_symbols): Call objfiles_changed.

This patch is causing crashes in reread.exp for me, because the stale
section list is accessed in breakpoint_re_set called from clear_symtab_users
called from reread_symbols *before* objfiles_changed is called.

Fixed by the following patch that calls objfiles_changed before
reread_symbols.

Tested on spu-elf; committed to mainline.

Bye,
Ulrich


ChangeLog:

	* symfile.c (reread_symbols): Call objfiles_changed *before*
	calling clear_symtab_users.

Index: gdb/symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.242
diff -u -p -r1.242 symfile.c
--- gdb/symfile.c	14 Aug 2009 23:35:33 -0000	1.242
+++ gdb/symfile.c	17 Aug 2009 19:46:44 -0000
@@ -2467,13 +2467,13 @@ reread_symbols (void)
 
   if (reread_one)
     {
+      /* Notify objfiles that we've modified objfile sections.  */
+      objfiles_changed ();
+
       clear_symtab_users ();
       /* At least one objfile has changed, so we can consider that
          the executable we're debugging has changed too.  */
       observer_notify_executable_changed ();
-
-      /* Notify objfiles that we've modified objfile sections.  */
-      objfiles_changed ();
     }
 }
 

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

On Mon, Aug 17, 2009 at 12:54 PM, Ulrich Weigand<uweigand@de.ibm.com> wrote:

> Section can partially overlap with overlays ...

Yes, the same thing also is happening on Darwin; some discussion here:
  http://sourceware.org/ml/gdb-patches/2009-08/msg00201.html

The thing is: when the assumptions are violated, (AFAICT) find_pc_section
couldn't have worked correctly before my patch, except by accident.

What is the simplest way to test overlays?

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Speed up find_pc_section

[Ulrich Weigand]

Paul Pluzhnikov <ppluzhnikov@google.com> wrote on 08/17/2009 11:15:20 PM:

> On Mon, Aug 17, 2009 at 12:54 PM, Ulrich Weigand<uweigand@de.ibm.com> 
wrote:
> 
> > Section can partially overlap with overlays ...
> 
> Yes, the same thing also is happening on Darwin; some discussion here:
>   http://sourceware.org/ml/gdb-patches/2009-08/msg00201.html
> 
> The thing is: when the assumptions are violated, (AFAICT) 
find_pc_section
> couldn't have worked correctly before my patch, except by accident.

Which assumptions?   For overlay sections, find_pc_section would
delegate its work to find_pc_mapped_section anyway:

  s = find_pc_mapped_section (pc);
  if (s)
    return s;

so the rest of find_pc_section does not need to handle overlay sections 
...

It just shouldn't abort simply because overlays are present anywhere.

> What is the simplest way to test overlays?

The only target with overlays I know to test is the SPU.  (Apparently, 
d10v
and m32r also support overlays, but I don't know how to test those.)

I'd be happy to test patches on the SPU ...


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

-- 
  Dr. Ulrich Weigand | Phone: +49-7031/16-3727
  STSM, GNU compiler and toolchain for Linux on System z and Cell/B.E.
  IBM Deutschland Research & Development GmbH
  Vorsitzender des Aufsichtsrats: Martin Jetter | Geschäftsführung: Erich 
Baier
  Sitz der Gesellschaft: Böblingen | Registergericht: Amtsgericht 
Stuttgart, HRB 243294

Re: [patch] Speed up find_pc_section

[Michael Snyder]

Paul Pluzhnikov wrote:
> On Mon, Aug 17, 2009 at 12:54 PM, Ulrich Weigand<uweigand@de.ibm.com> wrote:
> 
>> Section can partially overlap with overlays ...
> 
> Yes, the same thing also is happening on Darwin; some discussion here:
>   http://sourceware.org/ml/gdb-patches/2009-08/msg00201.html
> 
> The thing is: when the assumptions are violated, (AFAICT) find_pc_section
> couldn't have worked correctly before my patch, except by accident.
> 
> What is the simplest way to test overlays?

If you can build a d10v or m32r toolchain, you can run the
gdb overlay tests against the simulator.  That is, I hope
they still work -- I haven't touched them in years...

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Tue, Aug 18, 2009 at 5:23 AM, Ulrich
Weigand<Ulrich.Weigand@de.ibm.com> wrote:

> For overlay sections, find_pc_section would
> delegate its work to find_pc_mapped_section anyway:

Thanks, I understand how that works now.

> I'd be happy to test patches on the SPU ...

Could you please test attached patch?

I believe it is correct to simply not insert any overlay sections into
the section map. Tested on Linux/x86_64 with no new failures.

Thanks,
-- 
Paul Pluzhnikov

2009-08-20  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* objfiles.c (update_section_map): Skip overlay sections.

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

Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.92
diff -u -p -u -r1.92 objfiles.c
--- objfiles.c	17 Aug 2009 11:16:13 -0000	1.92
+++ objfiles.c	20 Aug 2009 17:47:56 -0000
@@ -837,9 +837,10 @@ 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)
+#define insert_p(objf, sec)						\
+  (section_is_mapped (sec) == 0 &&					\
+   ((bfd_get_section_flags ((objf)->obfd, (sec)->the_bfd_section)	\
+     & SEC_THREAD_LOCAL) == 0))
 
   map_size = 0;
   ALL_OBJSECTIONS (objfile, s)

Re: [patch] Speed up find_pc_section

[Ulrich Weigand]

Paul Pluzhnikov wrote:

> Could you please test attached patch?
> 
> I believe it is correct to simply not insert any overlay sections into
> the section map. Tested on Linux/x86_64 with no new failures.

Unfortunately, this doesn't help.  First of all "section_is_mapped"
depends on the target's current overlay state, which changes all the
time as overlays get swapped in and out.  If your section map were
to depend on section_is_mapped, it would have to be re-generated
every time the overlay state changes.

If you want to remove *all* overlay sections, mapped or not, you
could attempt to use "section_is_overlay" instead.  But that won't
work either -- all the overlay routines have a shortcut that says
"no" to all such questions before the user actually enabled overlay
support.  This means that when loading a binary with overlays,
you still crash at the assertion before the user even has the
chance to give the "overlay auto" command.

It used to be the case that you could debug -to some extent- binaries
with overlays even in "overlay off" mode, e.g. because GDB does not
support the particular overlay manager.  I think this should be preserved;
at least GDB should not run into assertion failures just because section
addresses overlap ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Thu, Aug 20, 2009 at 11:22 AM, Ulrich Weigand<uweigand@de.ibm.com> wrote:

> Unfortunately, this doesn't help. ...
> If you want to remove *all* overlay sections, mapped or not, you

Yes.

> could attempt to use "section_is_overlay" instead.  But that won't
> work either -- all the overlay routines have a shortcut that says
> "no" to all such questions before the user actually enabled overlay
> support.  This means that when loading a binary with overlays,
> you still crash at the assertion before the user even has the
> chance to give the "overlay auto" command.

Right. Here is an updated patch ...

> It used to be the case that you could debug -to some extent- binaries
> with overlays even in "overlay off" mode, e.g. because GDB does not
> support the particular overlay manager.  I think this should be preserved;
> at least GDB should not run into assertion failures just because section
> addresses overlap ...

I believe the crash at load should be gone now, and some debugging will
be possible, though find_pc_section will fail to find any overlay section
which find_pc_mapped_section doesn't find. I think that's still better
than what GDB was doing before under these conditions: finding a "random"
overlapping overlay section.

Thanks,
-- 
Paul Pluzhnikov

2009-08-20  Paul Pluzhnikov  <ppluzhnikov@google.com>

	*objfiles.c (insert_section_p): New function.
	(update_section_map): Call it.

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

Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.92
diff -u -p -u -r1.92 objfiles.c
--- objfiles.c	17 Aug 2009 11:16:13 -0000	1.92
+++ objfiles.c	20 Aug 2009 20:37:22 -0000
@@ -823,6 +823,24 @@ preferred_obj_section (struct obj_sectio
   return b;
 }
 
+/* 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))
+    /* This is an overlay section.  */
+    return 0;
+  if ((bfd_get_section_flags (abfd, section) & SEC_THREAD_LOCAL) != 0)
+    return 0;
+
+  return 1;
+}
+
 /* Update PMAP, PMAP_SIZE with non-TLS sections from all objfiles.  */
 
 static void
@@ -837,24 +855,18 @@ 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;
   ALL_OBJSECTIONS (objfile, s)
-    if (insert_p (objfile, s))
+    if (insert_section_p (objfile->obfd, s->the_bfd_section))
       map_size += 1;
 
   map = xmalloc (map_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)

Re: [patch] Speed up find_pc_section

[Daniel Jacobowitz]

On Thu, Aug 20, 2009 at 01:40:11PM -0700, Paul Pluzhnikov wrote:
> +  if (lma != 0 && lma != bfd_section_vma (abfd, section))
> +    /* This is an overlay section.  */
> +    return 0;

I see that you are using the same check as section_is_overlay, but
this really jumped out at me: I don't think this has anything to do
with overlay-ness.  In a bare metal application, it's not uncommon to
have LMA != VMA for every section.

That may be good enough for what you need here... I suppose you could
define such an application as 'only overlays', but I think of overlays
as having more than one set of code for the same VMA.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Speed up find_pc_section

[Ulrich Weigand]

Paul Pluzhnikov wrote:

> I believe the crash at load should be gone now, and some debugging will
> be possible, though find_pc_section will fail to find any overlay section
> which find_pc_mapped_section doesn't find. I think that's still better
> than what GDB was doing before under these conditions: finding a "random"
> overlapping overlay section.

With this patch, GDB no longer crashes, but some overlay tests fail anyway.
The problem is related to resolving an address in an overlay back to a
symbol via lookup_minimal_symbol_by_pc_section_1.  The caller passes the
correct PC (VMA value) and overlay section to this routine, but the
first thing the routine does is:

  /* PC has to be in a known section.  This ensures that anything
     beyond the end of the last segment doesn't appear to be part of
     the last function in the last segment.  */
  pc_section = find_pc_section (pc);
  if (pc_section == NULL)
    return NULL;

The new find_pc_section logic now returns NULL as the PC is in an
overlay section, which is not in the section map.

I think this routine needs to be fixed to take into account the section
it is passed as argument, probably along the following lines:

Index: gdb/minsyms.c
===================================================================
RCS file: /cvs/src/src/gdb/minsyms.c,v
retrieving revision 1.66
diff -u -p -r1.66 minsyms.c
--- gdb/minsyms.c	28 Jun 2009 00:20:22 -0000	1.66
+++ gdb/minsyms.c	21 Aug 2009 11:16:26 -0000
@@ -457,7 +457,7 @@ lookup_minimal_symbol_by_pc_section_1 (C
   struct objfile *objfile;
   struct minimal_symbol *msymbol;
   struct minimal_symbol *best_symbol = NULL;
-  struct obj_section *pc_section;
+  struct obj_section *pc_section = section;
   enum minimal_symbol_type want_type, other_type;
 
   want_type = want_trampoline ? mst_solib_trampoline : mst_text;
@@ -466,7 +466,8 @@ lookup_minimal_symbol_by_pc_section_1 (C
   /* PC has to be in a known section.  This ensures that anything
      beyond the end of the last segment doesn't appear to be part of
      the last function in the last segment.  */
-  pc_section = find_pc_section (pc);
+  if (pc_section == NULL)
+    pc_section = find_pc_section (pc);
   if (pc_section == NULL)
     return NULL;

(A cleaner fix might be to fix all callers to never pass in a NULL
section argument --most already don't-- and simply rely on it.)
 
With this patch in addition to yours, all overlay tests pass again
on spu-elf.


I'm still not completely happy about the assertions you added.  An
assertion failure is supposed to be an indication of a bug in GDB
-- it should never be possible to trigger the assertion just by
providing particular user input (this includes the binary file).
If for whatever reason the user provides a binary that has overlapping
section that are not recognized as overlays by your logic, you'll
still run into the failure ...

In other places where we detect weird contents of the binary (e.g.
in the DWARF data), we issue a "complaint" and then try to continue.
Maybe you should do the same here: if you find overlapping sections,
issue a complaint and then continue, while ignoring those sections.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Fri, Aug 21, 2009 at 4:30 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:

> (A cleaner fix might be to fix all callers to never pass in a NULL
> section argument --most already don't-- and simply rely on it.)

Attached patch does that ...
Not tested (I still haven't figured out how to test overlays).

> I'm still not completely happy about the assertions you added.  An
> assertion failure is supposed to be an indication of a bug in GDB
> -- it should never be possible to trigger the assertion just by
> providing particular user input (this includes the binary file).

I am still working on fixing that part...

Thanks,
--
Paul Pluzhnikov

2009-08-23  Paul Pluzhnikov  <ppluzhnikov@google.com>

        * minsyms.c (lookup_minimal_symbol_by_pc_section_1): Assume
non-NULL section.
        (lookup_minimal_symbol_by_pc_section): Check for NULL section.
        (lookup_minimal_symbol_by_pc): Adjust.

[-- Attachment #2: gdb-overlay-fix-20090823.txt --]
[-- Type: text/plain, Size: 2082 bytes --]

Index: minsyms.c
===================================================================
RCS file: /cvs/src/src/gdb/minsyms.c,v
retrieving revision 1.66
diff -u -p -u -r1.66 minsyms.c
--- minsyms.c	28 Jun 2009 00:20:22 -0000	1.66
+++ minsyms.c	22 Aug 2009 18:01:37 -0000
@@ -457,18 +457,10 @@ lookup_minimal_symbol_by_pc_section_1 (C
   struct objfile *objfile;
   struct minimal_symbol *msymbol;
   struct minimal_symbol *best_symbol = NULL;
-  struct obj_section *pc_section;
   enum minimal_symbol_type want_type, other_type;
 
   want_type = want_trampoline ? mst_solib_trampoline : mst_text;
   other_type = want_trampoline ? mst_text : mst_solib_trampoline;
-  
-  /* PC has to be in a known section.  This ensures that anything
-     beyond the end of the last segment doesn't appear to be part of
-     the last function in the last segment.  */
-  pc_section = find_pc_section (pc);
-  if (pc_section == NULL)
-    return NULL;
 
   /* We can not require the symbol found to be in pc_section, because
      e.g. IRIX 6.5 mdebug relies on this code returning an absolute
@@ -479,7 +471,7 @@ lookup_minimal_symbol_by_pc_section_1 (C
      files, search both the file and its separate debug file.  There's
      no telling which one will have the minimal symbols.  */
 
-  objfile = pc_section->objfile;
+  objfile = section->objfile;
   if (objfile->separate_debug_objfile)
     objfile = objfile->separate_debug_objfile;
 
@@ -680,6 +672,12 @@ lookup_minimal_symbol_by_pc_section_1 (C
 struct minimal_symbol *
 lookup_minimal_symbol_by_pc_section (CORE_ADDR pc, struct obj_section *section)
 {
+  if (section == NULL)
+    {
+      section = find_pc_section (pc);
+      if (section == NULL)
+	return NULL;
+    }
   return lookup_minimal_symbol_by_pc_section_1 (pc, section, 0);
 }
 
@@ -695,7 +693,7 @@ lookup_minimal_symbol_by_pc (CORE_ADDR p
   struct obj_section *section = find_pc_section (pc);
   if (section == NULL)
     return NULL;
-  return lookup_minimal_symbol_by_pc_section (pc, section);
+  return lookup_minimal_symbol_by_pc_section_1 (pc, section, 0);
 }
 \f
 

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Sun, Aug 23, 2009 at 3:48 PM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:
> On Fri, Aug 21, 2009 at 4:30 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
...
>> I'm still not completely happy about the assertions you added.  An
>> assertion failure is supposed to be an indication of a bug in GDB
>> -- it should never be possible to trigger the assertion just by
>> providing particular user input (this includes the binary file).
>
> I am still working on fixing that part...

Here is the patch. Tested on Linux/x86_64 and on Fedora11/i686 with
prelinking. No regressions.
It also passes Jan's new gdb.base/solib-overlap.exp test case for RH
Bug 515434, issuing these warnings:

warning: Unexpected overlap between section `.note.gnu.build-id' from
`/home/paul/gdb-cvs/build/gdb/testsuite/gdb.base/solib-overlap-lib1-0x50000000.so'
[0xf4, 0x118) and section `.note.gnu.build-id' from
`/home/paul/gdb-cvs/build/gdb/testsuite/gdb.base/solib-overlap-lib2-0x50000000.so'
[0xf4, 0x118)
warning: Unexpected overlap between section `.gnu.hash' from
`/home/paul/gdb-cvs/build/gdb/testsuite/gdb.base/solib-overlap-lib1-0x50000000.so'
[0x118, 0x158) and section `.gnu.hash' from
`/home/paul/gdb-cvs/build/gdb/testsuite/gdb.base/solib-overlap-lib2-0x50000000.so'
[0x118, 0x158)
... etc ... etc ...

Thanks,
-- 
Paul Pluzhnikov

2009-08-25  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-20090825.txt --]
[-- Type: text/plain, Size: 6857 bytes --]

Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.93
diff -p -u -r1.93 objfiles.c
--- objfiles.c	21 Aug 2009 17:57:17 -0000	1.93
+++ objfiles.c	26 Aug 2009 07:07:04 -0000
@@ -790,15 +790,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;
 }
@@ -823,12 +817,133 @@ preferred_obj_section (struct obj_sectio
   return b;
 }
 
+/* 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 non-TLS sections from all objfiles.  */
 
 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;
 
@@ -837,55 +952,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;

Re: [patch] Speed up find_pc_section

[Jan Kratochvil]

On Wed, 26 Aug 2009 09:20:06 +0200, Paul Pluzhnikov wrote:
> Here is the patch. Tested on Linux/x86_64 and on Fedora11/i686 with
> prelinking. No regressions.
> It also passes Jan's new gdb.base/solib-overlap.exp test case for RH
> Bug 515434, issuing these warnings:

Thanks for the fix.
(No regressions on {x86_64,x86_64-m32,i686}-fedorarawhide-linux-gnu.)


> +  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.  */

Interesting it exists, do you have a sample + kernel version of such VDSO?


> +  map_size = filter_debuginfo_sections(map, alloc_size);
> +  map_size = filter_overlapping_sections(map, map_size);

GNU spacing.


Thanks,
Jan

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Wed, Aug 26, 2009 at 5:03 AM, Jan
Kratochvil<jan.kratochvil@redhat.com> wrote:

>> +      && (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.  */
>
> Interesting it exists, do you have a sample + kernel version of such VDSO?

This showed up on F11/i686.

$ uname -a
Linux fc11i 2.6.29.6-217.2.3.fc11.i686.PAE #1 SMP Wed Jul 29 16:05:22
EDT 2009 i686 i686 i386 GNU/Linux

When I extract vdso.so via attached dump-vdso.c (Fedora disallows
reading of /proc/self/mem), I see this:

[paul@fc11i tmp]$ readelf -l ./vdso32.so

Elf file type is DYN (Shared object file)
Entry point 0xffffe414
There are 4 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000000 0xffffe000 0x00000000 0x00428 0x00428 R E 0x1000
  DYNAMIC        0x000350 0xffffe350 0x00000350 0x00078 0x00078 R   0x4
  NOTE           0x0001c4 0xffffe1c4 0x000001c4 0x00060 0x00060 R   0x4
  GNU_EH_FRAME   0x000224 0xffffe224 0x00000224 0x00024 0x00024 R   0x4


>> +  map_size = filter_debuginfo_sections(map, alloc_size);
>> +  map_size = filter_overlapping_sections(map, map_size);
>
> GNU spacing.

Oops. Fix attached.

Thanks,
-- 
Paul Pluzhnikov

2009-08-26  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-20090826.txt --]
[-- Type: text/plain, Size: 6859 bytes --]

Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.93
diff -p -u -r1.93 objfiles.c
--- objfiles.c	21 Aug 2009 17:57:17 -0000	1.93
+++ objfiles.c	26 Aug 2009 07:07:04 -0000
@@ -790,15 +790,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;
 }
@@ -823,12 +817,133 @@ preferred_obj_section (struct obj_sectio
   return b;
 }
 
+/* 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 non-TLS sections from all objfiles.  */
 
 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;
 
@@ -837,55 +952,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;

[-- Attachment #3: dump-vdso.c --]
[-- Type: application/octet-stream, Size: 742 bytes --]

#include <elf.h>
#include <stdio.h>
#include <stdlib.h>
#include <fcntl.h>
#include <errno.h>
#include <unistd.h>

void dump_vdso(void *ptr)
{
  int fd = open("vdso32.so", O_WRONLY|O_CREAT, 0755);
  if (-1 == fd) {
    perror("open2"); exit(1);
  }
  printf("vdso at %p\n", ptr);
  if (write(fd, ptr, 4096) != 4096) {
    perror("write"); exit(1);
  }
}

int main()
{
  int fd = open("/proc/self/auxv", O_RDONLY, 0);
  if (-1 == fd) {
    perror("open"); exit(1);
  }
  Elf32_auxv_t auxv;
  while (0 < read(fd, &auxv, sizeof(auxv))) {
    switch (auxv.a_type) {
      case AT_NULL: break;
      case AT_SYSINFO_EHDR:
        dump_vdso((void*)auxv.a_un.a_val);
        return 0;
    }
  }
  printf("No AT_SYSINFO_EHDR found\n");
  return 0;
}

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

On Wed, Aug 26, 2009 at 7:37 AM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:

> $ uname -a
> Linux fc11i 2.6.29.6-217.2.3.fc11.i686.PAE #1 SMP Wed Jul 29 16:05:22
> EDT 2009 i686 i686 i386 GNU/Linux

Forgot to mention: this is running inside VirtualBox
(http://www.virtualbox.org). Don't know whether this makes any
difference.

-- 
Paul Pluzhnikov

Re: [patch] Speed up find_pc_section

[Jan Kratochvil]

On Wed, 26 Aug 2009 16:37:24 +0200, Paul Pluzhnikov wrote:
> When I extract vdso.so via attached dump-vdso.c (Fedora disallows
> reading of /proc/self/mem),

Sorry I did not expect a dumper would be needed.  I usually dump VDSO
by GDB `dump memory' (after reading its address in /proc/PID/maps).


> I see this:

OK, reproduced, both native i386 and i386-on-x86_64 (thus vdso32 in general),
even with upstream kernel, posted:
	http://marc.info/?l=linux-kernel&m=125132628701515&w=2

Still I understand your workaround is unfortunately needed for GDB.


Thanks,
Jan

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

On Wed, Aug 26, 2009 at 3:45 PM, Jan
Kratochvil<jan.kratochvil@redhat.com> wrote:
> On Wed, 26 Aug 2009 16:37:24 +0200, Paul Pluzhnikov wrote:
>> When I extract vdso.so via attached dump-vdso.c (Fedora disallows
>> reading of /proc/self/mem),
>
> Sorry I did not expect a dumper would be needed.  I usually dump VDSO
> by GDB `dump memory'

Thanks for the pointer.

> (after reading its address in /proc/PID/maps).

You don't have to do that: 'info auxv' will show you the address in
AT_SYSINFO_EHDR entry :-)



-- 
Paul Pluzhnikov

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

On Wed, Aug 26, 2009 at 12:20 AM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:

> Here is the patch. Tested on Linux/x86_64 and on Fedora11/i686 with
> prelinking. No regressions.
> It also passes Jan's new gdb.base/solib-overlap.exp test case for RH
> Bug 515434, issuing these warnings:

Ping?

http://sourceware.org/ml/gdb-patches/2009-08/msg00372.html
http://sourceware.org/ml/gdb-patches/2009-08/msg00437.html


-- 
Paul Pluzhnikov

What should we do re: "[patch] Speed up find_pc_section"

[Joel Brobecker]

Hello everyone,

According to Tristan Gingold, this optimization is currently causing
the Darwin debugger to fail completely (I think Paul knows). Basically,
what happens is that, on Darwin, the debugging info is in the .o files,
and the sections there will necessarily overlap with the sections in
the executable. On MacOS, the way things are currently setup, I think
that we read the .o files as extra object files, rather than sources
of debug info (a-la .gnu_debug_link).

I don't know how dodgy it was in the first place to load the entire
.o as opposed to just its debugging info, but fixing this will require
a non-trivial effort. Tristan has some ideas on how to do this, but
it would be too late for the release - we're probably looking at fixing
this for 7.1. In the meantime, the MacOS port is badly broken.

I would like us to consider the idea of reverting that patch (and all
followup patches), only to re-apply it after the gdb-7.0 branch is cut.
The reason behind this suggestion is that we're still negotiating some
issues uncovered by this patch (I understand that some of the issues
were latent), and we're just so close to the branch time that I think
it would be less risky to release without it.

Paul, before we discuss the merits of that suggestion, do you think
that it would be doable to revert? I know that several patches have
already been applied on top in order to fix some of the issues we
later uncovered.

Other ideas?

Perhaps one possiblity would be to remove that assertion from the code
and let it return one section at random that matches the PC? Paul seems
to say that, if the assertion is tripped, it means that, before his
patch, we were already potentially returning the wrong section anyway.
Does removing the assertion bring us back to that situation?

One last suggestion that Tristan did make this morning, is to downgrade
Darwin support to DSYM files only, as opposed to automatically load
all .o files. That would be too bad, however, as this introduces an
extra step in the build that's inconvenient, and potentially time-
consuming.

-- 
Joel

Re: What should we do re: "[patch] Speed up find_pc_section"

[Paul Pluzhnikov]

On Tue, Sep 8, 2009 at 11:36 AM, Joel Brobecker <brobecker@adacore.com> wrote:

> According to Tristan Gingold, this optimization is currently causing
> the Darwin debugger to fail completely (I think Paul knows).

I certainly do. I also have a fix in the works, and Tristan confirmed that
what I have now fixes the problem for him.

> Basically,
> what happens is that, on Darwin, the debugging info is in the .o files,
> and the sections there will necessarily overlap with the sections in
> the executable.

Rather, all the .o files are relocatable, so they all start at zero
and overlap each other.

> On MacOS, the way things are currently setup, I think
> that we read the .o files as extra object files, rather than sources
> of debug info (a-la .gnu_debug_link).

Correct.

> I don't know how dodgy it was in the first place to load the entire
> .o as opposed to just its debugging info, but fixing this will require
> a non-trivial effort.

I don't believe there is non-trivial effort in fixing this.
In fact, I believe (but have not yet confirmed) this may just be fixed by
the other patch:
  http://sourceware.org/ml/gdb-patches/2009-08/msg00437.html

Specifically, I believe all the .o files will have lma != vma and will
get filtered out as if they are overlay sections:

+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;


> Tristan has some ideas on how to do this, but
> it would be too late for the release - we're probably looking at fixing
> this for 7.1. In the meantime, the MacOS port is badly broken.
>
> I would like us to consider the idea of reverting that patch (and all
> followup patches), only to re-apply it after the gdb-7.0 branch is cut.
> The reason behind this suggestion is that we're still negotiating some
> issues uncovered by this patch (I understand that some of the issues
> were latent), and we're just so close to the branch time that I think
> it would be less risky to release without it.
>
> Paul, before we discuss the merits of that suggestion, do you think
> that it would be doable to revert? I know that several patches have
> already been applied on top in order to fix some of the issues we
> later uncovered.

It appears to me that reverting this patch is more work than fixing the
overlay and MacOS problems exposed by it. But I haven't looked in detail
into what it will take to revert.

> Other ideas?
>
> Perhaps one possiblity would be to remove that assertion from the code
> and let it return one section at random that matches the PC? Paul seems
> to say that, if the assertion is tripped, it means that, before his
> patch, we were already potentially returning the wrong section anyway.

I believe that's correct.

> Does removing the assertion bring us back to that situation?

Almost, but not exactly: before the patch, the order of adding objfiles
mattered: the earlier objfile with overlapping section was the one that
was always found. Now the order of objfiles doesn't matter, as sections
are sorted independently.

> One last suggestion that Tristan did make this morning, is to downgrade
> Darwin support to DSYM files only, as opposed to automatically load
> all .o files. That would be too bad, however, as this introduces an
> extra step in the build that's inconvenient, and potentially time-
> consuming.

Note that the separate '*.o' addition has its own bug: the sections from
all the .o files are added, but never removed (and this is unrelated to
my patch in any way). This is in fact why Tristan said that my earlier patch
"works, but makes GDB spend 10 minutes in CPU loop".

What really happens there is that with lots of separate .o files to load for
a shared library, the first run takes 10s (600+ *.o files loaded), the
second 150s (1200+ *.o loaded), the third 600s.

This is also the part I haven't finished fixing yet.

Now that I am back from vacation, I will confirm that the
gdb-find_pc_section-20090825.txt from
  http://sourceware.org/ml/gdb-patches/2009-08/msg00437.html
in fact does also fix MacOS, and will work on eliminating unbounded growth
of the separate .o files.

If someone could just review the above patch, we'll be good to go :=)

Thanks,
--
Paul Pluzhnikov

Re: What should we do re: "[patch] Speed up find_pc_section"

[Joel Brobecker]

That was a very informative answer, thank you.

> Rather, all the .o files are relocatable, so they all start at zero
> and overlap each other.

After having applied your patch, the debugger reports overlaps between
the exec file and the .o files, not overlaps between .o files:

warning: Unexpected overlap between section `.data' from `/lensk.a/users/brobecke/ex/a' [0x100014000, 0x10001e850) and section `.data' from `a-except.o' [0x100014020, 0x100014031)
warning: Unexpected overlap between section `.data' from `/lensk.a/users/brobecke/ex/a' [0x100014000, 0x10001e850) and section `.data' from `s-except.o' [0x100014199, 0x10001419a)

> I don't believe there is non-trivial effort in fixing this.
> In fact, I believe (but have not yet confirmed) this may just be fixed by
> the other patch:
>   http://sourceware.org/ml/gdb-patches/2009-08/msg00437.html

I can confirm that this appears to fix the issue. The only complaint
I have with this patch is that it emits warnings each and every overlap
that it finds. Unfortunately, even for the smallest of programs in Ada,
you have a dozen object files involved (because of the GNAT runtime,
usually involving exceptions, etc). So I would suggest using a complaint
instead of a warning (this is similar to what we do with invalid DWARF
data that we find, I believe).

> Note that the separate '*.o' addition has its own bug: the sections from
> all the .o files are added, but never removed (and this is unrelated to
> my patch in any way). This is in fact why Tristan said that my earlier patch
> "works, but makes GDB spend 10 minutes in CPU loop".

Right - that's the part that would be less trivial to fix, hence
the suggestion to leave this for 7.1.

> Now that I am back from vacation, I will confirm that the
> gdb-find_pc_section-20090825.txt from
>   http://sourceware.org/ml/gdb-patches/2009-08/msg00437.html
> in fact does also fix MacOS, and will work on eliminating unbounded growth
> of the separate .o files.
> 
> If someone could just review the above patch, we'll be good to go :=)

I'll try to review. Not sure if I'm competent or not yet, though.

-- 
Joel

Re: [patch] Speed up find_pc_section

[Joel Brobecker]

> > (A cleaner fix might be to fix all callers to never pass in a NULL
> > section argument --most already don't-- and simply rely on it.)
> 
> Attached patch does that ...
> Not tested (I still haven't figured out how to test overlays).

This is just an informal review of the patch, since it has not been
tested (I assume one would like to test it in an overlay environment
as well - maybe Ulrich could take care of that; otherwise we'll have
to do without).

I feel like a dummy, sometimes, but it took me a while to understand
why find_pc_section might return NULL if PC is inside an overlay
section. I guess I overlooked the fact that the section might not
be mapped...

> 2009-08-23  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>         * minsyms.c (lookup_minimal_symbol_by_pc_section_1): Assume
> non-NULL section.

The function description needs to be updated. I would also add
an assertion that section is not NULL.

>         (lookup_minimal_symbol_by_pc_section): Check for NULL section.
>         (lookup_minimal_symbol_by_pc): Adjust.

Looks like lookup_minimal_symbol_by_pc is now almost a duplicate
of lookup_minimal_symbol_by_pc_section. We could replace its
implementation by:

  struct minimal_symbol *
  lookup_minimal_symbol_by_pc (CORE_ADDR pc)
  {
    return lookup_minimal_symbol_by_pc_section (pc, NULL);
  }

The comment from AndrewC needs to be preserved, but applies equally
to lookup_minimal_symbol_by_pc_section, I believe.

>    /* We can not require the symbol found to be in pc_section, because
                                                     ^^^^^^^^^^ section
>       e.g. IRIX 6.5 mdebug relies on this code returning an absolute

Otherwise, the patch seems fine. I would love some input from Ulrich,
though.

-- 
Joel

Re: [patch] Speed up find_pc_section

[Joel Brobecker]

> 2009-08-25  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.

Looks good to me, save for the few little comments I wrote below.
Please wait until Friday to give Ulrich a chance to comment on this
patch, since he's been pretty involved in that discussion.  But,
based on all the information you and him provided, I believe that
the patch incorporates all suggestions and should be correct.

> +/* 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)?

> +/* 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?

> +/* 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.

> +	      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.
But I also see, now, after reading Ulrich's messages, that he suggested
the same thing.

>  /* 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.

-- 
Joel

Re: [patch] Speed up find_pc_section

[Tristan Gingold]

On Sep 9, 2009, at 7:58 AM, Joel Brobecker 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)?

(I suppose that the address of a TLS section is not relevant as it is  
'duplicated' and relocated
  for each thread).

...

>> +/* 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).

What is the correct way to load just the debugging info ?  These  
object files are
loaded using 'symbol_file_add_from_bfd' and I thought it was the only  
way.

Tristan.

Re: [patch] Speed up find_pc_section

[Joel Brobecker]

> What is the correct way to load just the debugging info ?  These
> object files are loaded using 'symbol_file_add_from_bfd' and I thought
> it was the only  way.

I am not sure yet; I don't even know if there is a way. How do we do it
in case of .gnu_debug_link? symbol_file_add_from_bfd might be the only
way right now, but it does cause GDB to load too much, right? (symbol table,
sections, etc). (I was wondering out loud more than anything)

-- 
Joel

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

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

> Otherwise, the patch seems fine. I would love some input from Ulrich,
> though.

Thanks for the review. Attached patch implements your suggestions.
-- 
Paul Pluzhnikov


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

        *minsyms.c (lookup_minimal_symbol_by_pc_section_1): Assert non-NULL
        section.
        (lookup_minimal_symbol_by_pc_section): Check for NULL section.
        (lookup_minimal_symbol_by_pc): Adjust.

[-- Attachment #2: gdb-overlay-fix-20090910.txt --]
[-- Type: text/plain, Size: 4135 bytes --]

Index: minsyms.c
===================================================================
RCS file: /cvs/src/src/gdb/minsyms.c,v
retrieving revision 1.66
diff -u -p -u -r1.66 minsyms.c
--- minsyms.c	28 Jun 2009 00:20:22 -0000	1.66
+++ minsyms.c	10 Sep 2009 16:17:13 -0000
@@ -434,13 +434,14 @@ lookup_minimal_symbol_solib_trampoline (
 
 /* Search through the minimal symbol table for each objfile and find
    the symbol whose address is the largest address that is still less
-   than or equal to PC, and matches SECTION (if non-NULL).  Returns a
-   pointer to the minimal symbol if such a symbol is found, or NULL if
-   PC is not in a suitable range.  Note that we need to look through
-   ALL the minimal symbol tables before deciding on the symbol that
-   comes closest to the specified PC.  This is because objfiles can
-   overlap, for example objfile A has .text at 0x100 and .data at
-   0x40000 and objfile B has .text at 0x234 and .data at 0x40048.
+   than or equal to PC, and matches SECTION (which is not NULL).
+   Returns a pointer to the minimal symbol if such a symbol is found,
+   or NULL if PC is not in a suitable range.
+   Note that we need to look through ALL the minimal symbol tables
+   before deciding on the symbol that comes closest to the specified PC.
+   This is because objfiles can overlap, for example objfile A has .text
+   at 0x100 and .data at 0x40000 and objfile B has .text at 0x234 and
+   .data at 0x40048.
 
    If WANT_TRAMPOLINE is set, prefer mst_solib_trampoline symbols when
    there are text and trampoline symbols at the same address.
@@ -457,20 +458,12 @@ lookup_minimal_symbol_by_pc_section_1 (C
   struct objfile *objfile;
   struct minimal_symbol *msymbol;
   struct minimal_symbol *best_symbol = NULL;
-  struct obj_section *pc_section;
   enum minimal_symbol_type want_type, other_type;
 
   want_type = want_trampoline ? mst_solib_trampoline : mst_text;
   other_type = want_trampoline ? mst_text : mst_solib_trampoline;
-  
-  /* PC has to be in a known section.  This ensures that anything
-     beyond the end of the last segment doesn't appear to be part of
-     the last function in the last segment.  */
-  pc_section = find_pc_section (pc);
-  if (pc_section == NULL)
-    return NULL;
 
-  /* We can not require the symbol found to be in pc_section, because
+  /* We can not require the symbol found to be in section, because
      e.g. IRIX 6.5 mdebug relies on this code returning an absolute
      symbol - but find_pc_section won't return an absolute section and
      hence the code below would skip over absolute symbols.  We can
@@ -479,7 +472,8 @@ lookup_minimal_symbol_by_pc_section_1 (C
      files, search both the file and its separate debug file.  There's
      no telling which one will have the minimal symbols.  */
 
-  objfile = pc_section->objfile;
+  gdb_assert (section != NULL);
+  objfile = section->objfile;
   if (objfile->separate_debug_objfile)
     objfile = objfile->separate_debug_objfile;
 
@@ -680,6 +674,15 @@ lookup_minimal_symbol_by_pc_section_1 (C
 struct minimal_symbol *
 lookup_minimal_symbol_by_pc_section (CORE_ADDR pc, struct obj_section *section)
 {
+  if (section == NULL)
+    {
+      /* NOTE: cagney/2004-01-27: This was using find_pc_mapped_section to
+	 force the section but that (well unless you're doing overlay
+	 debugging) always returns NULL making the call somewhat useless.  */
+      section = find_pc_section (pc);
+      if (section == NULL)
+	return NULL;
+    }
   return lookup_minimal_symbol_by_pc_section_1 (pc, section, 0);
 }
 
@@ -689,13 +692,7 @@ lookup_minimal_symbol_by_pc_section (COR
 struct minimal_symbol *
 lookup_minimal_symbol_by_pc (CORE_ADDR pc)
 {
-  /* NOTE: cagney/2004-01-27: This was using find_pc_mapped_section to
-     force the section but that (well unless you're doing overlay
-     debugging) always returns NULL making the call somewhat useless.  */
-  struct obj_section *section = find_pc_section (pc);
-  if (section == NULL)
-    return NULL;
-  return lookup_minimal_symbol_by_pc_section (pc, section);
+  return lookup_minimal_symbol_by_pc_section (pc, NULL);
 }
 \f
 

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

[-- 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;

Re: [patch] Speed up find_pc_section

[Joel Brobecker]

> 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.

Clever approach, I like it. Are you setup to test this change
on MacOS by any chance, now? Otherwise, since MacOS is completely
broken, verifying that it helps with any program should be sufficient.
In any case, we need to test this patch in a non-MacOS target as well,
to test the opposite situation, just in case.

Would you mind doing that?  If it simplifies things for you, you may
check-in the two other pending patches that I reviewed, even if I said
to wait for Friday. If there is any objection or issue, it's easy enough
to revert or fix.

> 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.

I still think we should use a complaint in this case. This is still
similar to what we do in dwarf2read - we detected a problem with
the debugging info, and we emit a complaint. The idea is that,
we don't know how many times we're going to encounter that problem,
and flooding the user with the same message over something that he
probably has little control over is counter productive.

> 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.

That being said, it's not a very strong opinion, and if you really
prefer a warning, please go ahead with this version of the patch.
You're fixing the issue on MacOS anyway, so it's making this discussion
less important.

-- 
Joel

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Thu, Sep 10, 2009 at 11:29 AM, Joel Brobecker <brobecker@adacore.com> wrote:

>> 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.
>
> I still think we should use a complaint in this case.

Adjusted patch attached.

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-2.txt --]
[-- Type: text/plain, Size: 7135 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	11 Sep 2009 01:25:48 -0000
@@ -51,6 +51,7 @@
 #include "arch-utils.h"
 #include "exec.h"
 #include "observer.h"
+#include "complaints.h"
 
 /* Prototypes for local functions */
 
@@ -802,15 +803,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 +830,135 @@ 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);
+
+	      complaint (&symfile_complaints,
+			 _("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 +967,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;

RE: [patch] Speed up find_pc_section

[Pierre Muller]

The patch looks OK (not that I have any say on this),
but I am still puzzled by what can happen if
you have overlapping sections in the end.

  Is the CORE_ADDR to section mapping still 
deterministic in that case?
  Using binary search for ill order item
can lead to different results if you add
items to the list, no?
  And I suspect that dynamic loading of 
library can add items to the list with running 
a debuggee, thus leading to a different result 
before and after that loading for the same CORE_ADDR.
  
  Shouldn't we by brute force shorten one of the two
overlapping sections to remove that error?
  If I understood your patch correctly,
it doesn't do that for now.
  Please tell me if I am wrong.

Adding a simple
	 obj_section_endaddr (sect1):= sect2_addr;
after the complaint should be enough to
be sure that the binary search is deterministic, no?



Pierre Muller
Pascal language support maintainer for GDB

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

On Thu, Sep 10, 2009 at 11:49 PM, Pierre Muller <muller@ics.u-strasbg.fr> wrote:
> The patch looks OK (not that I have any say on this),
> but I am still puzzled by what can happen if
> you have overlapping sections in the end.

At that point GDB is in a bad state, and *may* not work right.
Further, since we are now doing complaint() instead of warning(), the
fact that GDB is in a bad state is not immediately obvious.

>  Is the CORE_ADDR to section mapping still
> deterministic in that case?

Depends on what you mean by that. The mapping is deterministic for a
given set of objfiles, but may change when the set changes (even if a
new objfile does not have any sections "covering" CORE_ADDR for which
the mapping will change). That is because qsort is not a stable sort.
I wonder if it makes sense to modify qsort_cmp() like this:

 if (sect1_addr < sect2_addr)
    return -1;
  else if (sect1_addr > sect2_addr)
    return 1;

-  return 0;
+  if (sect1->objfile->mtime < sect2->objfile->mtime) return -1;
+  if (sect1->objfile->mtime > sect2->objfile->mtime) return 1;
+  return 0;
}

to try to make the sort order for sections starting at the same
address more stable. This attempt could still fail if the two object
files were copied together and have the same mtime.

Perhaps I should just add a sequence number to 'struct objfile'? That
looks like it would almost give exact same ordering as what GDB used
before qsort+bsearch was introduced.

>  Using binary search for ill order item
> can lead to different results if you add
> items to the list, no?

All overlaps are removed in filter_overlapping_sections, before the
first binary search is performed.

>  And I suspect that dynamic loading of
> library can add items to the list with running
> a debuggee, thus leading to a different result
> before and after that loading for the same CORE_ADDR.

That is correct.

>  Shouldn't we by brute force shorten one of the two
> overlapping sections to remove that error?

filter_overlapping_sections simply removes one of such sections from the map.

>  If I understood your patch correctly,
> it doesn't do that for now.
>  Please tell me if I am wrong.
>
> Adding a simple
>         obj_section_endaddr (sect1):= sect2_addr;
> after the complaint should be enough to
> be sure that the binary search is deterministic, no?

The binary search (I believe) is already deterministic, and modifying
the_bfd_section (where obj_section_endaddr comes from) "behind
libbfd's back" strikes me as very undesirable.

Cheers,
-- 
Paul Pluzhnikov

Re: [patch] Speed up find_pc_section

[Mark Kettenis]

> Date: Fri, 11 Sep 2009 00:29:19 -0700
> From: Paul Pluzhnikov <ppluzhnikov@google.com>
> 
>  if (sect1_addr < sect2_addr)
>     return -1;
>   else if (sect1_addr > sect2_addr)
>     return 1;
> 
> -  return 0;
> +  if (sect1->objfile->mtime < sect2->objfile->mtime) return -1;
> +  if (sect1->objfile->mtime > sect2->objfile->mtime) return 1;
> +  return 0;
> }

This snippet caught my eye.  Please put those return statements on a
line of their own.

RE: [patch] Speed up find_pc_section

[Pierre Muller]

> >  Using binary search for ill order item
> > can lead to different results if you add
> > items to the list, no?
> 
> All overlaps are removed in filter_overlapping_sections, before the
> first binary search is performed.

  This is where I get lost:
I see no code in filter_overlapping_sections
that changes map_size, so how are these overlapping
elements removed and what is put in their place?


Pierre Muller
Pascal language support maintainer for GDB

Re: [patch] Speed up find_pc_section

[Tristan Gingold]

On Sep 9, 2009, at 5:04 PM, Joel Brobecker wrote:

>> What is the correct way to load just the debugging info ?  These
>> object files are loaded using 'symbol_file_add_from_bfd' and I  
>> thought
>> it was the only  way.
>
> I am not sure yet; I don't even know if there is a way. How do we do  
> it
> in case of .gnu_debug_link?

I think we almost do the same.  But .gnu_debug_link allows only one  
file.

> symbol_file_add_from_bfd might be the only
> way right now, but it does cause GDB to load too much, right?  
> (symbol table,
> sections, etc). (I was wondering out loud more than anything)

I plan to rework on this to fix severals issues.

Tristan.

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

On Fri, Sep 11, 2009 at 12:39 AM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:

>> -  return 0;
>> +  if (sect1->objfile->mtime < sect2->objfile->mtime) return -1;
>> +  if (sect1->objfile->mtime > sect2->objfile->mtime) return 1;
>> +  return 0;
>> }
>
> This snippet caught my eye.  Please put those return statements on a
> line of their own.

This wasn't a real patch, just something I typed by hand.
Yes, I will certainly put return on a separate line IF it is
considered reasonable (which I somewhat doubt).


-- 
Paul Pluzhnikov

Re: [patch] Speed up find_pc_section

[Tristan Gingold]

>
> [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.]

Can you elaborate ?

IMHO this is not fundamentally different.  In both cases, one file  
contains the code and the other/others
debug infos.

Tristan.

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

On Fri, Sep 11, 2009 at 12:39 AM, Pierre Muller <muller@ics.u-strasbg.fr> wrote:

>> All overlaps are removed in filter_overlapping_sections, before the
>> first binary search is performed.
>
>  This is where I get lost:
> I see no code in filter_overlapping_sections
> that changes map_size, so how are these overlapping
> elements removed and what is put in their place?

Ah, that's a bug; thanks for catching it.
The filter_overlapping_sections was supposed to return 'j', not 'map_size'.

I'll retest an updated patch and send it in tomorrow.
-- 
Paul Pluzhnikov

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

On Fri, Sep 11, 2009 at 12:53 AM, Tristan Gingold <gingold@adacore.com> wrote:
>>
>> [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.]
>
> Can you elaborate ?
>
> IMHO this is not fundamentally different.  In both cases, one file contains
> the code and the other/others debug infos.

As I said before, the "primary" and "secondary" objfiles in the
separate debuginfo case have a common ancestor, and are almost like
identical twins: same sections (except for the weird F11 prelinking
case), same symbol tables, same file ELF file type, etc.

That isn't at all the case for MacOS "bunch of .o files" -- now we
have a "pile of parts" and a "completed engine". Certainly the
sections from separate .o files should not appear in the section map;
not sure whether symbols should, but likely not.

It sounds to me as if you'd want to pick up just the debug bits from
*.o, without turning them into full-blown objfiles.

I should mention that on Solaris, Sun cc uses the same trick: leaves
(stabs) debug info in the .o files and doesn't put any of them into
the final linked image (unless '-xs' flag is given on command line).
The name of the object file with debug info is recorded in N_OPT stab
entries in the image, so DBX knows where to find them. It doesn't look
like GDB supports this currently.

Cheers,
-- 
Paul Pluzhnikov

Re: [patch] Speed up find_pc_section

[Tristan Gingold]

On Sep 11, 2009, at 10:33 AM, Paul Pluzhnikov wrote:

> On Fri, Sep 11, 2009 at 12:53 AM, Tristan Gingold  
> <gingold@adacore.com> wrote:
>>>
>>> [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.]
>>
>> Can you elaborate ?
>>
>> IMHO this is not fundamentally different.  In both cases, one file  
>> contains
>> the code and the other/others debug infos.
>
> As I said before, the "primary" and "secondary" objfiles in the
> separate debuginfo case have a common ancestor, and are almost like
> identical twins: same sections (except for the weird F11 prelinking
> case), same symbol tables, same file ELF file type, etc.

That's understood.

> That isn't at all the case for MacOS "bunch of .o files" -- now we
> have a "pile of parts" and a "completed engine". Certainly the
> sections from separate .o files should not appear in the section map;
> not sure whether symbols should, but likely not.

Too.

> It sounds to me as if you'd want to pick up just the debug bits from
> *.o, without turning them into full-blown objfiles.

Right, but why not doing this also for separate debuginfo ?

Tristan.

RE: [patch] Speed up find_pc_section

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Paul Pluzhnikov
> Envoyé : Friday, September 11, 2009 10:03 AM
> À : Pierre Muller
> Cc : Joel Brobecker; Ulrich Weigand; gdb-patches ml; Tom Tromey; Jan
> Kratochvil
> Objet : Re: [patch] Speed up find_pc_section
> 
> On Fri, Sep 11, 2009 at 12:39 AM, Pierre Muller <muller@ics.u-
> strasbg.fr> wrote:
> 
> >> All overlaps are removed in filter_overlapping_sections, before the
> >> first binary search is performed.
> >
> >  This is where I get lost:
> > I see no code in filter_overlapping_sections
> > that changes map_size, so how are these overlapping
> > elements removed and what is put in their place?
> 
> Ah, that's a bug; thanks for catching it.
> The filter_overlapping_sections was supposed to return 'j', not
> 'map_size'.
Maybe you could alter the complaint so that it becomes
clear which of the two sections is discarded from the mapping?
In your current patch, it isn't obvious.


Pierre Muller
Pascal language support maintainer for GDB

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

On Fri, Sep 11, 2009 at 1:39 AM, Tristan Gingold <gingold@adacore.com> wrote:

>> It sounds to me as if you'd want to pick up just the debug bits from
>> *.o, without turning them into full-blown objfiles.
>
> Right, but why not doing this also for separate debuginfo ?

I can't think of a reason, though the patch to do so might get large.
Looks like the separate debuginfo support was added here:

2003-01-23  Alexander Larsson <alexl@redhat.com>
            Jim Blandy  <jimb@redhat.com>

        Add support for executables whose debug info has been separated
        out into a separate file, leaving only a link behind.
        * objfiles.h (struct objfile): New fields: separate_debug_objfile
        and separate_debug_objfile_backlink.

Perhaps Jim could comment on the merits of creating a separate objfile
for the debuginfo file.

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Fri, Sep 11, 2009 at 1:36 AM, Pierre Muller <muller@ics.u-strasbg.fr> wrote:

> Maybe you could alter the complaint so that it becomes
> clear which of the two sections is discarded from the mapping?

Patch with all the suggestions so far incorporated.
Tested on Linux/x86_64 with no regressions.

Here is what the complaint looks like for gdb.base/solib-overlap.exp
(if I turn complaints on):

  During symbol reading, unexpected overlap between (A) section
`.hash' from `/usr/local/google/gdb/testsuite/gdb.base/solib-overlap-lib1-0x40000000.so'
[0x158, 0x19c) and (B) section `.hash' from
`/usr/local/google/gdb/testsuite/gdb.base/solib-overlap-lib2-0x40000000.so'
[0x158, 0x19c). Will ignore section B.

Thanks,
-- 
Paul Pluzhnikov

2009-09-11  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-20090911.txt --]
[-- Type: text/plain, Size: 7169 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	11 Sep 2009 16:33:08 -0000
@@ -51,6 +51,7 @@
 #include "arch-utils.h"
 #include "exec.h"
 #include "observer.h"
+#include "complaints.h"
 
 /* Prototypes for local functions */
 
@@ -802,15 +803,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 +830,136 @@ 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);
+
+	      complaint (&symfile_complaints,
+			 _("unexpected overlap between (A) "
+			   "section `%s' from `%s' [%s, %s) and (B) "
+			   "section `%s' from `%s' [%s, %s). "
+			   "Will ignore section B"),
+			 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 j;
+}
+
+
+/* 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 +968,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;

Re: [patch] Speed up find_pc_section

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

I'm finally catching up on this thread...

Paul> Perhaps I should just add a sequence number to 'struct objfile'? That
Paul> looks like it would almost give exact same ordering as what GDB used
Paul> before qsort+bsearch was introduced.

Is there some benefit to the old ordering?

I assumed that the old approach just gave the user an arbitrary
ordering, in which case any change here is no big deal.  But, if there
is a benefit to the old ordering, sure, restore it.

How stable a sort would you want?  You could just fall back to the
objfile's address.  Or is that too awful?

Tom

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

On Fri, Sep 11, 2009 at 1:50 PM, Tom Tromey <tromey@redhat.com> wrote:

> Paul> Perhaps I should just add a sequence number to 'struct objfile'? That
> Paul> looks like it would almost give exact same ordering as what GDB used
> Paul> before qsort+bsearch was introduced.
>
> Is there some benefit to the old ordering?

The only benefit I see is that it was deterministic (a section from an
earlier-added objfile always matched first and was returned).

With current sort/filter/bsearch we don't have that. If we enter "bad state"
(two sections overlap), it may happen that we sometimes return the one
from earlier-added objfile, and sometimes the one from the later-added
objfile.

This could lead to GDB bug reports which are harder to reproduce (your
qsort may produce a different final section order from my qsort even for
otherwise identical inputs).

> I assumed that the old approach just gave the user an arbitrary
> ordering, in which case any change here is no big deal.

Not completely arbitrary.

> But, if there
> is a benefit to the old ordering, sure, restore it.

I think there is -- reproducibility of the "bad state".

> How stable a sort would you want?  You could just fall back to the
> objfile's address.  Or is that too awful?

I don't think there is any advantage to that, as this will still lead to
essentially random ordering from one system to another.

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Speed up find_pc_section

[Joel Brobecker]

> 2009-09-10  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>         *minsyms.c (lookup_minimal_symbol_by_pc_section_1): Assert non-NULL
>         section.
>         (lookup_minimal_symbol_by_pc_section): Check for NULL section.
>         (lookup_minimal_symbol_by_pc): Adjust.

(I assume this has been regression-tested...)
Approved, please go ahead and commit.

-- 
Joel

Re: [patch] Speed up find_pc_section

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Tom> But, if there is a benefit to the old ordering, sure, restore it.

Paul> I think there is -- reproducibility of the "bad state".

Thanks.

If you want to add an objfile sequence number to cope with this, it
would be fine by me.

Tom

Re: [patch] Speed up find_pc_section

[Joel Brobecker]

> 2009-09-11  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.

Looks great to me. Please go ahead and commit.

> +	      complaint (&symfile_complaints,
> +			 _("unexpected overlap between (A) "
> +			   "section `%s' from `%s' [%s, %s) and (B) "
> +			   "section `%s' from `%s' [%s, %s). "
> +			   "Will ignore section B"),
> +			 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));

Just one question: Do you think it might be advantageous to split
this complaint in multiple lines? Something like:
                  _("\
unexpected overlap between:\n\
  (A) section `%s' from ...\n\
  (B) section `%s' from ...\n\
Will ignore section B")

Just a thought... If others think so, I will make the change after
you commit your patch. You've already gone beyond the call of duty
on this one.

-- 
Joel

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Fri, Sep 11, 2009 at 2:14 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>> 2009-09-11  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.
>
> Looks great to me. Please go ahead and commit.

Additional testing revealed an off-by-one bug :-( Now fixed.

> Just one question: Do you think it might be advantageous to split
> this complaint in multiple lines? Something like:

Good idea. Done.

This also implements ordering of sections (by objfile sequence, and by
section sequence within a single objfile), just to make deterministic
which sections are discarded.

Tested: Linux/x86_64 (no regressions), Fedora 11/i386 (no regressions)
Also tested Darwin/i386 (can't run a testsuite, but internal assert is
gone from the java test case by Christian Thalinger).
Also tested Solaris 10/i386, no assertions in objfiles.c [1], can't
really tell regressions because running GDB tests causes kernel panic
:(
Also tested Solaris 11/i386 (aka OpenSolaris), no regressions [2].

I think this is ready to commit :-)

Ulrich?

Thanks,

[1] There is an unrelated assert:
UNRESOLVED: gdb.mi/mi-var-cmd.exp: update all vars: i changed
-var-update *
n
-exec-step
~"Please answer y or n.\n"
~"../../src/gdb/thread.c:575: internal-error: is_thread_state:
Assertion `tp' failed.\nA problem internal to GDB has been
detected,\nfurther debugging may pro
ve unreliable.\nCreate a core file of GDB? "


[2] Also unrelated asserts:

Starting program:
/export/home/paul/gdb-cvs/build/gdb/testsuite/gdb.objc/objcdecode ^M
../../src/gdb/breakpoint.c:7939: internal-error:
breakpoint_re_set_one: Assertion `sals.nelts == 1' failed.^M
A problem internal to GDB has been detected,^M
further debugging may prove unreliable.^M
Quit this debugging session? (y or n) FAIL: gdb.objc/objcdecode.exp:
continue after break on multiply defined symbol (GDB internal error)

(gdb) PASS: gdb.threads/hand-call-in-threads.exp: prepare to make hand
call, thread 1
call hand_call()^M
../../src/gdb/inline-frame.c:322: internal-error: skip_inline_frames:
Assertion `find_inline_frame_state (ptid) == NULL' failed.^M
A problem internal to GDB has been detected,^M
further debugging may prove unreliable.^M
Quit this debugging session? FAIL:
gdb.threads/hand-call-in-threads.exp: hand call, thread 1 (GDB
internal error)


-- 
Paul Pluzhnikov


2009-09-13  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-20090912.txt --]
[-- Type: text/plain, Size: 9502 bytes --]

Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.95
diff -p -u -r1.95 objfiles.c
--- objfiles.c	11 Sep 2009 18:51:31 -0000	1.95
+++ objfiles.c	12 Sep 2009 16:17:30 -0000
@@ -51,6 +51,7 @@
 #include "arch-utils.h"
 #include "exec.h"
 #include "observer.h"
+#include "complaints.h"
 
 /* Prototypes for local functions */
 
@@ -802,16 +803,72 @@ 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;
+  else
+   {
+     /* Sections are at the same address.  This could happen if
+	A) we have an objfile and a separate debuginfo.
+	B) we are confused, and have added sections without proper relocation,
+	or something like that. */
+
+     const struct objfile *const objfile1 = sect1->objfile;
+     const struct objfile *const objfile2 = sect2->objfile;
+
+     if (objfile1->separate_debug_objfile == objfile2
+	 || objfile2->separate_debug_objfile == objfile1)
+       {
+	 /* Case A.  The ordering doesn't matter: separate debuginfo files
+	    will be filtered out later.  */
+
+	 return 0;
+       }
+
+     /* Case B.  Maintain stable sort order, so bugs in GDB are easier to
+	triage.  This section could be slow (since we iterate over all
+	objfiles in each call to qsort_cmp), but this shouldn't happen
+	very often (GDB is already in a confused state; one hopes this
+	doesn't happen at all).  If you discover that significant time is
+	spent in the loops below, do 'set complaints 100' and examine the
+	resulting complaints.  */
+
+     if (objfile1 == objfile2)
+       {
+	 /* Both sections came from the same objfile.  We are really confused.
+	    Sort on sequence order of sections within the objfile.  */
+
+	 const struct obj_section *osect;
+
+	 ALL_OBJFILE_OSECTIONS (objfile1, osect)
+	   if (osect == sect1)
+	     return -1;
+	   else if (osect == sect2)
+	     return 1;
+
+	 /* We should have found one of the sections before getting here.  */
+	 gdb_assert (0);
+       }
+     else
+       {
+	 /* Sort on sequence number of the objfile in the chain.  */
+
+	 const struct objfile *objfile;
+
+	 ALL_OBJFILES (objfile)
+	   if (objfile == objfile1)
+	     return -1;
+	   else if (objfile == objfile2)
+	     return 1;
+
+	 /* We should have found one of the objfiles before getting here.  */
+	 gdb_assert (0);
+       }
+
+   }
 
+  /* Unreachable.  */
+  gdb_assert (0);
   return 0;
 }
 
@@ -835,69 +892,175 @@ 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 void
-update_section_map (struct obj_section ***pmap, int *pmap_size)
+static int
+insert_section_p (const struct bfd *abfd,
+		  const struct bfd_section *section)
 {
-  int map_size, i, j;
-  struct obj_section *s, **map;
-  struct objfile *objfile;
+  const bfd_vma lma = bfd_section_lma (abfd, section);
 
-  gdb_assert (objfiles_changed_p != 0);
+  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 some Linux systems (e.g. Fedora 11).  */
+    return 0;
+  if ((bfd_get_section_flags (abfd, section) & SEC_THREAD_LOCAL) != 0)
+    /* This is a TLS section.  */
+    return 0;
 
-  map = *pmap;
-  xfree (map);
+  return 1;
+}
 
-#define insert_p(objf, sec) \
-  ((bfd_get_section_flags ((objf)->obfd, (sec)->the_bfd_section) \
-    & SEC_THREAD_LOCAL) == 0)
+/* 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.  */
 
-  map_size = 0;
-  ALL_OBJSECTIONS (objfile, s)
-    if (insert_p (objfile, s))
-      map_size += 1;
+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;
+    }
 
-  map = xmalloc (map_size * sizeof (*map));
+  if (i < map_size)
+    {
+      gdb_assert (i == map_size - 1);
+      map[j++] = map[i];
+    }
 
-  i = 0;
-  ALL_OBJSECTIONS (objfile, s)
-    if (insert_p (objfile, s))
-      map[i++] = s;
+  /* The map should not have shrunk to less than half the original size.  */
+  gdb_assert (map_size / 2 <= j);
 
-#undef insert_p
+  return j;
+}
 
-  qsort (map, map_size, sizeof (*map), qsort_cmp);
+/* 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.  */
 
-  /* 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)
+static int
+filter_overlapping_sections (struct obj_section **map, int map_size)
+{
+  int i, j;
+
+  for (i = 0, j = 0; i < map_size - 1; )
     {
-      struct obj_section *sect1 = map[i];
-      struct obj_section *sect2 = (i + 1 < map_size) ? map[i + 1] : NULL;
+      int k;
 
-      if (sect2 == NULL
-	  || obj_section_addr (sect1) != obj_section_addr (sect2))
-	map[j++] = sect1;
-      else
+      map[j++] = map[i];
+      for (k = i + 1; k < map_size; k++)
 	{
-	  map[j++] = preferred_obj_section (sect1, sect2);
-	  ++i;
+	  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);
+
+	      complaint (&symfile_complaints,
+			 _("unexpected overlap between:\n"
+			   " (A) section `%s' from `%s' [%s, %s)\n"
+			   " (B) section `%s' from `%s' [%s, %s).\n"
+			   "Will ignore section B"),
+			 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;
     }
 
-  if (j < map_size)
+  if (i < 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.  */
+      gdb_assert (i == map_size - 1);
+      map[j++] = map[i];
     }
+
+  return j;
+}
+
+
+/* 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 alloc_size, map_size, i;
+  struct obj_section *s, **map;
+  struct objfile *objfile;
+
+  gdb_assert (objfiles_changed_p != 0);
+
+  map = *pmap;
+  xfree (map);
+
+  alloc_size = 0;
+  ALL_OBJSECTIONS (objfile, s)
+    if (insert_section_p (objfile->obfd, s->the_bfd_section))
+      alloc_size += 1;
+
+  map = xmalloc (alloc_size * sizeof (*map));
+
+  i = 0;
+  ALL_OBJSECTIONS (objfile, s)
+    if (insert_section_p (objfile->obfd, s->the_bfd_section))
+      map[i++] = s;
+
+  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;

Re: [patch] Speed up find_pc_section

[Ulrich Weigand]

Paul Pluzhnikov wrote:

> On Tue, Sep 8, 2009 at 10:38 PM, Joel Brobecker<brobecker@adacore.com> wrote:
> 
> > Otherwise, the patch seems fine. I would love some input from Ulrich,
> > though.
> 
> Thanks for the review. Attached patch implements your suggestions.
> -- 
> Paul Pluzhnikov
> 
> 
> 2009-09-10  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>         *minsyms.c (lookup_minimal_symbol_by_pc_section_1): Assert non-NULL
>         section.
>         (lookup_minimal_symbol_by_pc_section): Check for NULL section.
>         (lookup_minimal_symbol_by_pc): Adjust.

This looks good to me as well, thanks!

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] Speed up find_pc_section

[Ulrich Weigand]

Paul Pluzhnikov wrote:

> I think this is ready to commit :-)
> 
> Ulrich?

> 2009-09-13  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.

I've tested this together with the lookup_minimal_symbol_by_pc
patch on spu-elf, and it fixes all overlay-related bugs and
introduces no regressions, so it looks good to me as well.

Thanks for your work on addressing this problem!

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

On Mon, Sep 14, 2009 at 9:43 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:

> I've tested this together with the lookup_minimal_symbol_by_pc
> patch on spu-elf, and it fixes all overlay-related bugs and
> introduces no regressions, so it looks good to me as well.

Thanks,

I've just checked in both patches. Hopefully this thread will end here,
it's been long enough :-)

-- 
Paul Pluzhnikov

Re: [patch] Speed up find_pc_section

[Joel Brobecker]

> I've just checked in both patches. Hopefully this thread will end here,
> it's been long enough :-)

There is still the OBJF_NOT_MAPPED patch. Care to test it? (it would
be sufficient for me to test it on a Linux platform while verifying
that it also gets rid of the complaints on MacOS)

I will update the gdb-7.0 Wiki accordingly.

-- 
Joel

Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov]

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

On Mon, Sep 14, 2009 at 10:35 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>> I've just checked in both patches. Hopefully this thread will end here,
>> it's been long enough :-)
>
> There is still the OBJF_NOT_MAPPED patch.

Given this subthread:

  http://sourceware.org/ml/gdb-patches/2009-09/msg00307.html

I don't believe OBJF_NOT_MAPPED is the right way to go.

I agree with Tristan that we should separate the 'read objfile for symbols,
sections, etc.' from read an object file just for its debug bits.

This would allow:
- Darwin "dwarf debug is found in the separate *.o files",
- Solaris cc/CC without '-xs' (dwarf or stabs debug is found in original
  *.o files -- very similar to Darwin),
- We could then use the same on Linux and drop the
  separate_debug_objfile/separate_debug_objfile_backlink objfile members,
  as well as filter_debuginfo_sections().

Note that current checked in code on Darwin does not fail any asserts,
it just doesn't work correctly and issues bazillion of complaints.

Given above, I don't think committing/testing the OBJF_NOT_MAPPED patch
is the right way to go -- it's more of a hack then a solution.

However, if someone needs a hack, attached is the current state of the
patch. Christian Thalinger tested essentially this version on his Darwin/Java
build, and reported success.

Cheers,
-- 
Paul Pluzhnikov

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

	* objfiles.h (OBJF_NOT_MAPPED): New define.
	* objfiles.c (update_section_map): Ignore not mapped objfiles.
	* machoread.c (macho_oso_symfile, macho_oso_symfile): Adjust.

[-- Attachment #2: gdb-macho-bp-reset-20090914.txt --]
[-- Type: text/plain, Size: 3113 bytes --]

Index: objfiles.h
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.h,v
retrieving revision 1.64
diff -u -p -u -r1.64 objfiles.h
--- objfiles.h	11 Sep 2009 18:51:31 -0000	1.64
+++ objfiles.h	14 Sep 2009 18:04:54 -0000
@@ -414,6 +414,12 @@ struct objfile
 
 #define OBJF_USERLOADED	(1 << 3)	/* User loaded */
 
+/* This objfile is not mapped into the process address space. We only have it
+   for its debug bits.  One example is MACH-O ("other source") object
+   files.  */
+
+#define OBJF_NOT_MAPPED (1 << 4)        /* Not mapped */
+
 /* The object file that the main symbol table was loaded from (e.g. the
    argument to the "symbol-file" or "file" command).  */
 
Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.96
diff -u -p -u -r1.96 objfiles.c
--- objfiles.c	14 Sep 2009 17:12:07 -0000	1.96
+++ objfiles.c	14 Sep 2009 18:04:54 -0000
@@ -1041,16 +1041,21 @@ update_section_map (struct obj_section *
   xfree (map);
 
   alloc_size = 0;
-  ALL_OBJSECTIONS (objfile, s)
-    if (insert_section_p (objfile->obfd, s->the_bfd_section))
-      alloc_size += 1;
+
+  ALL_OBJFILES (objfile)
+    if ((objfile->flags & OBJF_NOT_MAPPED) == 0)
+      ALL_OBJFILE_OSECTIONS (objfile, s)
+	if (insert_section_p (objfile->obfd, s->the_bfd_section))
+	  alloc_size += 1;
 
   map = xmalloc (alloc_size * sizeof (*map));
 
   i = 0;
-  ALL_OBJSECTIONS (objfile, s)
-    if (insert_section_p (objfile->obfd, s->the_bfd_section))
-      map[i++] = s;
+  ALL_OBJFILES (objfile)
+    if ((objfile->flags & OBJF_NOT_MAPPED) == 0)
+      ALL_OBJFILE_OSECTIONS (objfile, s)
+	if (insert_section_p (objfile->obfd, s->the_bfd_section))
+	  map[i++] = s;
 
   qsort (map, alloc_size, sizeof (*map), qsort_cmp);
   map_size = filter_debuginfo_sections(map, alloc_size);
Index: machoread.c
===================================================================
RCS file: /cvs/src/src/gdb/machoread.c,v
retrieving revision 1.5
diff -u -p -u -r1.5 machoread.c
--- machoread.c	19 Jun 2009 14:30:30 -0000	1.5
+++ machoread.c	14 Sep 2009 18:04:54 -0000
@@ -406,7 +406,8 @@ macho_oso_symfile (struct objfile *main_
 	      bfd_close (member_bfd);
 	    }
 	    else
-	      symbol_file_add_from_bfd (member_bfd, 0, addrs, 0);
+	      symbol_file_add_from_bfd (member_bfd, SYMFILE_DEFER_BP_RESET,
+					addrs, OBJF_NOT_MAPPED);
 	}
       else
 	{
@@ -429,7 +430,8 @@ macho_oso_symfile (struct objfile *main_
 	      continue;
 	    }
   
-	  symbol_file_add_from_bfd (abfd, 0, addrs, 0);
+	  symbol_file_add_from_bfd (abfd, SYMFILE_DEFER_BP_RESET, addrs,
+				    OBJF_NOT_MAPPED);
 	}
       xfree (oso->symbols);
       xfree (oso->offsets);
@@ -592,7 +594,7 @@ macho_symfile_read (struct objfile *objf
 	  oso_vector = NULL;
 
 	  /* Now recurse: read dwarf from dsym.  */
-	  symbol_file_add_from_bfd (dsym_bfd, 0, NULL, 0);
+	  symbol_file_add_from_bfd (dsym_bfd, 0, NULL, OBJF_NOT_MAPPED);
       
 	  /* Don't try to read dwarf2 from main file or shared libraries.  */
 	  return;

Re: [patch] Speed up find_pc_section

[Joel Brobecker]

> Given this subthread:
> 
>   http://sourceware.org/ml/gdb-patches/2009-09/msg00307.html
> 
> I don't believe OBJF_NOT_MAPPED is the right way to go.

Neither do I for the long term. I was seeing this as a temporary
work-around while the larger design issue is getting worked on.
The patch is relatively small and it's easy to get rid of that flag
once it becomes OBE.

> Given above, I don't think committing/testing the OBJF_NOT_MAPPED patch
> is the right way to go -- it's more of a hack then a solution.

Agreed. I'm curious as to what the other maintainers think of this
approach. I will probably use this patch in AdaCore's tree, but
thought that it might be useful to others as well. The complaint
is one part of it which I find to be an issue (they will drown any
other complaint that we might emit), but there there is a small
performance aspect where we skip scanning objfiles that do not need
to be scanned.

Anyway, let's see what the others think. I'll pursue this further
if there is interest.

-- 
Joel