Re: [patch] Speed up find_pc_section

[Joel Brobecker <brobecker@adacore.com>]

> 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