Re: [PATCH v8 09/10] Validate symbol file using build-id

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Tue, 23 Jun 2015 00:25:52 +0200, Doug Evans wrote:
> On Sun, Jun 21, 2015 at 5:16 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> >         * gdb.texinfo (Files): Add 'set solib-build-id-force'
> >         and 'show solib-build-id-force'.
> 
> IIUC this applies to symbol files (the main program) too, right?

No.  That is an extension I am working on as an add-on patchset, to be posted
in a week or two.

I expected this "solib" patchset will be already approved a long time ago so
the add-on patchset will make sense.  But given this patchset is still being
reviewed and the new patchset will change some parts of this one I am curious
whether I should not rather merge the second patchset into the first one and
start the review process from scratch.


> If so, having solib in the option name is confusing.
> 
> set build-id-force
> or
> set require-build-id-match
> or some such would be clearer.

The new patchset is being cooked as the last commits without ChangeLogs at:
	https://sourceware.org/git/?p=archer.git;a=log;h=refs/heads/jankratochvil/gdbserverbuildid
Particularly:
	https://sourceware.org/git/?p=archer.git;a=commitdiff;h=79c03cbb287878d3e5fcfb8104bdd21aa712f013
	-set solib-build-id-force (on|off)
	+set build-id-force (on|off)


> > +/* Validate SO by comparing build-id from the associated bfd and
> > +   corresponding build-id from target memory.  */
> 
> Please document that the result is an error message or NULL for success
> (including missing build id), and that the caller must free it.
> I realize you say so in the docs for the "validate" "method",
> but the comment here doesn't mention it is the validate method
> (which would be a fine alternative to repeating all the docs
> of the method).

I agree; although it gets reworked in the add-on patchset anyway.
	https://sourceware.org/git/?p=archer.git;a=commitdiff;h=6d40ae1db39bdabb415a05aa909178d61cb519ed

> > +
> > +static char *
> > +svr4_validate (const struct so_list *const so)


> > @@ -1146,6 +1215,25 @@ library_list_start_library (struct gdb_xml_parser *parser,
> >    strncpy (new_elem->so_name, name, sizeof (new_elem->so_name) - 1);
> >    new_elem->so_name[sizeof (new_elem->so_name) - 1] = 0;
> >    strcpy (new_elem->so_original_name, new_elem->so_name);
> > +  if (hex_build_id != NULL)
> > +    {
> > +      const size_t hex_build_id_len = strlen (hex_build_id);
> > +
> > +      if (hex_build_id_len > 0 && (hex_build_id_len & 1U) == 0)
> > +       {
> > +         const size_t build_idsz = hex_build_id_len / 2;
> > +
> > +         new_elem->build_id = xmalloc (build_idsz);
> > +         new_elem->build_idsz = hex2bin (hex_build_id, new_elem->build_id,
> > +                                         build_idsz);
> > +         if (new_elem->build_idsz != build_idsz)
> > +           {
> 
> This happens for a malformed build id, right?

Yes.

> A warning would be useful here.
> It'd also be nice to have a warning for an odd count.

OK.


> > --- a/gdb/solist.h
> > +++ b/gdb/solist.h
> > @@ -75,6 +75,22 @@ struct so_list
> >         There may not be just one (e.g. if two segments are relocated
> >         differently); but this is only used for "info sharedlibrary".  */
> >      CORE_ADDR addr_low, addr_high;
> > +
> > +    /* Build id in raw format, contains verbatim contents of
> > +       .note.gnu.build-id including note header.
> 
> Including the note header will be confusing to readers.
> Is there a reason to include it?

It seems to simplify all the code.  I will recheck how the code looks if it
parses the note.


Thanks,
Jan