Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov <ppluzhnikov@google.com>]

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