This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] Speed up find_pc_section
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Paul Pluzhnikov <ppluzhnikov at google dot com>
- Cc: Ulrich Weigand <uweigand at de dot ibm dot com>, Ulrich Weigand <Ulrich dot Weigand at de dot ibm dot com>, gdb-patches ml <gdb-patches at sourceware dot org>, Tom Tromey <tromey at redhat dot com>, Jan Kratochvil <jan dot kratochvil at redhat dot com>
- Date: Tue, 8 Sep 2009 22:58:24 -0700
- Subject: Re: [patch] Speed up find_pc_section
- References: <8ac60eac0908201340k6b759eb5o9bb73c8f473d8785@mail.gmail.com> <200908211130.n7LBUCJc011108@d12av02.megacenter.de.ibm.com> <8ac60eac0908231548x135edf2doa04fa59a49455bcd@mail.gmail.com> <8ac60eac0908260020l4200cf84v2686a76b5858d13@mail.gmail.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