Re: [draft patch 0/6] Split FYI and some review notes

[Aleksandar Ristovski <aristovski@qnx.com>]

On 13-03-10 05:07 PM, Jan Kratochvil wrote:
> Hello Aleksandar,

Hello Jan,

>
> to send something here is some draft, there will be still some changes.
>
> I had first difficulties to apply it to FSF GDB HEAD as it has some conflicts
> so I had to rebase it first.

It applied cleanly when I posted it, I rebased it and made sure it 
applies to cleanly cvs checked out tree. Sorry if you had difficulties 
applying the patch, I could have rebased it for you.

>
> It is not a normal review as your patch has merged code moves + code
> modifications which is unreviewable.  Moves and modifications need to be in
> separate patches so that one can see what has changed in the diff.

I thought it is better to keep the whole change as a single blob so it 
can be easily reverted should there be any problems. But I can continue 
with the sequence of patches as you broked them down.

>
> fileio_read_stralloc you have reimplemented for gdbserver; I have rather
> chosen to generalize the gdb/ variant and call the same common/ code from both
> gdb and gdbserver.  One may ask whether it is not more complicated than its
> reimplementation, not sure.
>
> By moving linux_find_memory_regions_full you have dropped make_cleanup there.
> Its use from GDB may now leak memory as its called FUNC may throw an
> exception.  I have put there 'void **memory_to_free_ptr' to make it reusable
> from both gdb and gdbserver.
>
> I tried to make the patches similar to your code so that one can diff the
> result against your patch.
>
> There were many code formatting little changes and also needless casts
> (probably from C++).
>
> For the last [draft patch 6/6] - things not done there yet:
>
> get_dynamic already parses/scans PHDRs, you introduce new PHDR parser, they
> should be merged.
>
> get_hex_build_id must not be based on soname, moreover playing with realname
> on it, it is too fragile.  It should be based on addresses, you know that l_ld
> is absolute address inside the library.  So find maps/smaps entry containing
> l_ld, subtract its file offset from vaddr and you have the ELF header address.
>
> Do not run get_hex_build_id quadratically - currently for each library you
> open and scan maps/smaps again.  Probably the best approach is to scan all
> l_ld addresses from r_debug first, then qsort them and then linearly match
> them to the maps/smaps entries.  I am not sure but I guess they still should
> be returned to GDB in their original order from r_debug for some backward and
> solib-svr4.c compatibility.  Also Gary Benson is working on incremental shlib
> transfers from gdbserver so that it does not clash too much (it will anyway).
> While just quadratic computation would be in real world acceptable all the
> open/read/close syscalls I find really needlessly expensive.

I thought it would be an overkill to make it any more optimal. Ideal 
situation would be to send "library loaded"/ "library unloaded" events 
instead of sending the whole list each time.

But sure, I'll do the qsort.


>
> PT_NOTE segment contains arbitrary number of notes, up to its size.  You check
> only the first entry there.

Ok.

>
> On Fri, 22 Feb 2013 16:06:44 +0100, Aleksandar Ristovski wrote:
>> As per Jan's request, this patch adds 'build-id' to the gdbserver
>> response. The value is hex encoded string representing
>> .note.gnu.build-id section of the corresponding shared library.
>
> gdbserver reports PT_NOTE segment, not any section.  gdbserver is dependent on
> runtime information (segments), it cannot rely on debug/link information
> (sections).

My references to the sections is to denote we are dealing with the 
contents of the section (which is mapped to the segment).

>
> BTW ping me (possibly off-list) whether you plan to work on it now yourself or
> whether I should be doing more of the changes described above; I sure have
> also enough other work...

If you feel strongly, you can take it over. Otherwise, I will finish it. 
I plan some follow up changes to all this.

If you want the patches to be attributed to you, that is also fine by 
me, let me know.

>
>
> Thanks,
> Jan
>

Thanks,

Aleksandar