Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov <ppluzhnikov at google dot com>]

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: gdb-find_pc_section-20090910.txt
Description: Text document