From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32549 invoked by alias); 17 Apr 2013 20:54:22 -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 32540 invoked by uid 89); 17 Apr 2013 20:54:22 -0000 X-Spam-SWARE-Status: No, score=-6.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 17 Apr 2013 20:54:21 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3HKsIjg022017 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 17 Apr 2013 16:54:18 -0400 Received: from host2.jankratochvil.net (ovpn-116-84.ams2.redhat.com [10.36.116.84]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r3HKsCDe007899 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 17 Apr 2013 16:54:15 -0400 Date: Thu, 18 Apr 2013 10:23:00 -0000 From: Jan Kratochvil To: Aleksandar Ristovski Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 7/8] Validate symbol file using build-id. Message-ID: <20130417205412.GF2090@host2.jankratochvil.net> References: <1365521265-28870-1-git-send-email-ARistovski@qnx.com> <1365521265-28870-8-git-send-email-ARistovski@qnx.com> <20130414141740.GE23227@host2.jankratochvil.net> <516D6AF4.1000708@qnx.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <516D6AF4.1000708@qnx.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2013-04/txt/msg00553.txt.bz2 On Tue, 16 Apr 2013 17:15:00 +0200, Aleksandar Ristovski wrote: > On 13-04-14 10:17 AM, Jan Kratochvil wrote: > >On Tue, 09 Apr 2013 17:27:44 +0200, Aleksandar Ristovski wrote: > >[...] > >>--- a/gdb/solib-svr4.c > >>+++ b/gdb/solib-svr4.c > >[...] [...] > >>+ if (target_read_memory (bfd_get_section_vma (so->abfd, asec) > >>+ + lm_addr_check (so, so->abfd), > > > >I see it is the most easy one how to get the target VMA of the options we were > >discussing. > > I do not parse this unambiguously... did you mean this is the > easiest way of getting target VMA? If so, I agree. Yes. > >>+ { > >>+ build_idsz > >>+ = extract_unsigned_integer ((gdb_byte *) note->descsz, > >>+ sizeof (note->descsz), > >>+ byte_order); > >>+ > >>+ if (build_idsz == elf_tdata (so->abfd)->build_id->size) > >>+ { > >>+ const char gnu[4] = "GNU\0"; > >>+ > >>+ if (memcmp (note->name, gnu, 4) == 0) > >>+ { > >>+ ULONGEST namesz > >>+ = extract_unsigned_integer ((gdb_byte *) note->namesz, > >>+ sizeof (note->namesz), > >>+ byte_order); > >>+ CORE_ADDR build_id_offs; > >>+ > >>+ /* Rounded to next sizeof (ElfXX_Word). */ > >>+ namesz = ((namesz + (sizeof (note->namesz) - 1)) > >>+ & ~((ULONGEST) (sizeof (note->namesz) - 1))); > > > >Like in the previous patch the rounding should be 4 bytes according to the > >standard. Not a sizeof of anything. > > Yes, I re-read it, you are right: it is 4 byte aligned. Somehow I > remembered it as Word aligned. But I do not see it fixed (to use literal constant 4) in solib-svr4.c. (Checking your GIT branch, not your posted patch, I hope they match.) > >>+ warning (_("Shared object \"%s\" could not be validated " > >>+ "and will be ignored."), so->so_name); > > > >Maybe the warning should by printed by svr4_validate stating the build-id does > >not match (even possibly printing the both build-ids). And then sure one can > >remove the warning here. > > I was thinking about that, but then decided that svr4_validate > should simply provide an answer, not print warnings. User of the > function should warn if appropriate (as is done in the patch). The problem is what end-user will do with a situation GDB prints Shared object \"%s\" could not be validated and will be ignored. and the symbols are missing? The user has to start investigating target and local shared library, it would be easier if GDB already printed both local and remote build-ids which it already knows. It may be easier for user to check it then. Or why don't you find useful to print both build-ids? Not a blocker, one can easily change the message in the future depending on how it will be useful in practice. > This is along the lines of (see below) using build-id for e.g. > finding separate debug info etc... When there are problems finding separate debug info it is already difficult to troubleshoot, I commonly use strace on gdb, that could be also improved (that is sure very unrelated to your current patchset). > >>@@ -551,6 +562,7 @@ free_so (struct so_list *so) > >> { > >> struct target_so_ops *ops = solib_ops (target_gdbarch ()); > >> > >>+ xfree (so->build_id); > >> free_so_symbols (so); > >> ops->free_so (so); > >> > > > >I already wrote: > > > >The problem is there is: > > xfree (so->build_id); > >in free_so() but it should be in free_so_symbols instead. free_so_symbols is > >called also from reload_shared_libraries_1 where so_list->abfd can change. > >Then obviously one should also set so->build_id = NULL there. > > > I'm not sure I follow this. In cases where so->build_id is > determined it is good for the lifetime of the 'so', not symbols. It > can not change for the given 'so'. OK, I take my comment back. You are right current svr4_validate() never stores the linux-nat-mode fetched build-id into so_list. so_list->build_id contains only build-id from gdbserver which is always right. Therefore I agree xfree in free_so is OK. > >I was thinking making the BUILD_ID data private so solib-svr4.c but that is > >currently not possible, lm_info is private for solib-svr4.c but that has > >lifetime of so_list, not the lifetime of so_list->abfd. > > > I was thinking slightly differently: build-id should be generalized > to represent "an ID". For svr4, it is build-id, for some other > system it might be something else, but if present, should represent > the connection between target binary, binary at hand and associated > optional separate debug info. Not comment "what if" but currently there is no solib-backend-specific subpart of so_list which is not trashed during reread_symbols (like so_list->lm_info is) so there isn't where else to place the gdbserver build_id field now. Thanks, Jan