This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch 6/6] gdbserver build-id attribute generator
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: Aleksandar Ristovski <aristovski at qnx dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Wed, 3 Apr 2013 21:32:58 +0200
- Subject: Re: [patch 6/6] gdbserver build-id attribute generator
- References: <51278984 dot 3070208 at qnx dot com> <20130310210843 dot GG21130 at host2 dot jankratochvil dot net> <514C56D4 dot 1060906 at qnx dot com> <20130326204157 dot GC12291 at host2 dot jankratochvil dot net> <51530465 dot 30503 at qnx dot com> <20130327145028 dot GA17905 at host2 dot jankratochvil dot net> <515353CF dot 40601 at qnx dot com> <5154ADD2 dot 9040206 at qnx dot com> <20130331174322 dot GB21374 at host2 dot jankratochvil dot net> <515B05D8 dot 1000003 at qnx dot com>
On Tue, 02 Apr 2013 18:22:48 +0200, Aleksandar Ristovski wrote:
> >>+static int
> >>+find_phdr_p (const void *const phdr, const int is_elf64,
> >>+ const void *const data)
> >
> >But I do not understand why this function exists - it could be integrated in
> >find_phdr as very every caller of find_phdr passse as find_phdr_p parameter
> >this find_phdr_p implementation.
>
>
> [AR] Yes, but I am eyeing solib-svr4.c loops over pheaders
> generalization - find_phdr could be moved to 'common' and possibly
> called 'iterate_phdrs' with the ability to pass in any function, not
> necessarily a predicate only. E.g. svr4_exec_displacement, etc...)
It is not relevant for this patch what do you plan. In the form as is it is
needlessly overcomplicated. That more thorough generalization would be
possibly good but it is not here and it may never happen. GDB will just
remain with needlessly complicated code.
> >You need to check here also the name content, it is "GNU\x00" (n_namesz == 4).
>
> [AR], can it be any other name if type is NT_GNU_BUILD_ID? Added the
> check but seems like an overkill to me.
NT_GNU_BUILD_ID is just number 3. I find very realistic for example
"SUNW Solaris" would use at least 3 note types (does not use it already?).
> >>+/* Linear reverse find starting from RBEGIN towards REND looking for
> >>+ the lowest vaddr mapping of the same inode and zero offset. */
> >>+
> >>+static mapping_entry_s *
> >>+lrfind_mapping_entry (mapping_entry_s *const rbegin,
> >>+ const mapping_entry_s *const rend)
> >>+{
> >>+ mapping_entry_s *p;
> >>+
> >>+ for (p = rbegin - 1; p >= rend && p->inode == rbegin->inode; --p)
> >>+ if (p->offset == 0)
> >>+ return p;
> >
> >I had here this layout:
> >7ffff7ddc000-7ffff7dfd000 r-xp 00000000 fd:01 51415762 /usr/lib64/ld-2.17.so
> >7ffff7ff9000-7ffff7ffa000 rw-p 00000000 00:00 0
> >7ffff7ffa000-7ffff7ffc000 r-xp 00000000 00:00 0 [vdso]
> >7ffff7ffc000-7ffff7ffe000 rw-p 00020000 fd:01 51415762 /usr/lib64/ld-2.17.so
> >
> >so it should rather be:
> > for (p = rbegin - 1; p >= rend; --p)
> > if (p->offset == 0 && p->inode == rbegin->inode)
> > return p;
> >
> >Fortunately it should not have any real performance impact.
>
> [AR] Ok, though we are not adding mappings with inode == 0. See
> 'find_memory_region_callback'.
Hmm, that's true, I do not see now why it failed for me. But that could be
even non-zero-inode mapping so a change was appropriate.
This is not yet a full review.
Thanks,
Jan