From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20267 invoked by alias); 23 Jun 2015 20:47:10 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 20256 invoked by uid 89); 23 Jun 2015 20:47:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.0 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 23 Jun 2015 20:47:08 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 9FB7319F97B; Tue, 23 Jun 2015 20:47:07 +0000 (UTC) Received: from host1.jankratochvil.net (ovpn-116-41.ams2.redhat.com [10.36.116.41]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t5NKl2TI026322 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Tue, 23 Jun 2015 16:47:05 -0400 Date: Tue, 23 Jun 2015 20:47:00 -0000 From: Jan Kratochvil To: Doug Evans Cc: gdb-patches , Aleksandar Ristovski Subject: Re: [PATCH v8 09/10] Validate symbol file using build-id Message-ID: <20150623204702.GA2156@host1.jankratochvil.net> References: <20150614192542.18346.87859.stgit@host1.jankratochvil.net> <20150614192655.18346.17075.stgit@host1.jankratochvil.net> <20150621101644.GA12733@host1.jankratochvil.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-IsSubscribed: yes X-SW-Source: 2015-06/txt/msg00508.txt.bz2 On Tue, 23 Jun 2015 00:25:52 +0200, Doug Evans wrote: > On Sun, Jun 21, 2015 at 5:16 AM, Jan Kratochvil 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