Re: [patch 6/6] gdbserver build-id attribute generator

[Jan Kratochvil <jan dot kratochvil at redhat 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