Re: [patch] Speed up find_pc_section

Joel Brobecker
2009-09-09T05:58:42Z

> 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