This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v8 09/10] Validate symbol file using build-id
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: Doug Evans <dje at google dot com>
- Cc: gdb-patches <gdb-patches at sourceware dot org>, Aleksandar Ristovski <ARistovski at qnx dot com>
- Date: Tue, 23 Jun 2015 22:47:02 +0200
- Subject: Re: [PATCH v8 09/10] Validate symbol file using build-id
- Authentication-results: sourceware.org; auth=none
- References: <20150614192542 dot 18346 dot 87859 dot stgit at host1 dot jankratochvil dot net> <20150614192655 dot 18346 dot 17075 dot stgit at host1 dot jankratochvil dot net> <20150621101644 dot GA12733 at host1 dot jankratochvil dot net> <CADPb22TH5nfFSkCzW9cCiHuoJFAG+P5z=qZawgwGUgYQ5kfd5Q at mail dot gmail dot 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