Re: [patchv3 7/8] Validate symbol file using build-id

[Pedro Alves <palves at redhat dot com>]

On 02/28/2014 08:20 PM, Jan Kratochvil wrote:
> On Fri, 28 Feb 2014 14:28:23 +0100, Pedro Alves wrote:
>> On 02/27/2014 09:33 PM, Jan Kratochvil wrote:
>>> +static int
>>> +svr4_validate (const struct so_list *const so)
>>> +{
>>> +  gdb_assert (so != NULL);
>>> +
>>> +  /* There is no way to safely fetch build-id from running inferior without OS
>>
>> What is "safely" referring to here?
> 
> The bug that was there from Aleksandar and my former wrong approval in the
> review.  The mail [patch 0/8] contains larger removed code which was trying to
> read build-id data from address computed from the on-disk BFD (symbol) file.
> 
> But that does not work for i386 files prelinked on disk and unprelinked in
> memory (or vice versa) as on this arch REL<->RELA conversion changes VMAs.
> Code/data symbols still match then but the build-id VMA does not, therefore
> GDB would read build-id from wrong address (=read a garbage).
> 
> gdbserver does it correctly as it reads ELF header, Program headers, PT_NOTE
> etc. from the memory, not from disk.

I see.  _This_ design rationale could be a comment somewhere, IMO.
I'd expect it near the definition of the build_id field.  And indeed
I see something like that is already there:

+    /* Build id in raw format, contains verbatim contents of
+       .note.gnu.build-id including note header.  This is actual
+       BUILD_ID which comes either from the remote target via qXfer
+       packet or via reading target memory.  Therefore, it may differ
+       from the build-id of the associated bfd.  In a normal
+       scenario, this so would soon lose its abfd due to failed
+       validation.  */
+    size_t build_idsz;
+    gdb_byte *build_id;
   };

But we could always extend it further, with a note on that prelink
issue, if necessary.

>>> +     specific code.  The code from get_hex_build_id from gdbserver/linux-low.c
>>> +     could be used for GNU/Linux NAT target.  */
>>
>> I'd rather not have comments that kind of suggest solib-svr4.c
>> is a Linux-specific file, when it is not.
>>
>> I'd prefer just saying:
>>
>>   +  /* Target doesn't support reporting the build ID.  */
> 
> Put it there.  But it is a useless text not describing the IMO unclear VMA
> change described above.

Well, the old comment didn't remotely suggest anything related
to VMAs and prelink to me either.  ;-)

-- 
Pedro Alves