From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14918 invoked by alias); 3 Apr 2013 19:33:12 -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 14908 invoked by uid 89); 3 Apr 2013 19:33:12 -0000 X-Spam-SWARE-Status: No, score=-7.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,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, 03 Apr 2013 19:33:07 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r33JX3kP015524 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 3 Apr 2013 15:33:03 -0400 Received: from host2.jankratochvil.net (ovpn-116-44.ams2.redhat.com [10.36.116.44]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r33JWxQh018659 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 3 Apr 2013 15:33:02 -0400 Date: Thu, 04 Apr 2013 02:22:00 -0000 From: Jan Kratochvil To: Aleksandar Ristovski Cc: "gdb-patches@sourceware.org" Subject: Re: [patch 6/6] gdbserver build-id attribute generator Message-ID: <20130403193258.GA9853@host2.jankratochvil.net> References: <51278984.3070208@qnx.com> <20130310210843.GG21130@host2.jankratochvil.net> <514C56D4.1060906@qnx.com> <20130326204157.GC12291@host2.jankratochvil.net> <51530465.30503@qnx.com> <20130327145028.GA17905@host2.jankratochvil.net> <515353CF.40601@qnx.com> <5154ADD2.9040206@qnx.com> <20130331174322.GB21374@host2.jankratochvil.net> <515B05D8.1000003@qnx.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <515B05D8.1000003@qnx.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2013-04/txt/msg00090.txt.bz2 On Tue, 02 Apr 2013 18:22:48 +0200, Aleksandar Ristovski wrote: > >>+static int > >>+find_phdr_p (const void *const phdr, const int is_elf64, > >>+ const void *const data) > > > >But I do not understand why this function exists - it could be integrated in > >find_phdr as very every caller of find_phdr passse as find_phdr_p parameter > >this find_phdr_p implementation. > > > [AR] Yes, but I am eyeing solib-svr4.c loops over pheaders > generalization - find_phdr could be moved to 'common' and possibly > called 'iterate_phdrs' with the ability to pass in any function, not > necessarily a predicate only. E.g. svr4_exec_displacement, etc...) It is not relevant for this patch what do you plan. In the form as is it is needlessly overcomplicated. That more thorough generalization would be possibly good but it is not here and it may never happen. GDB will just remain with needlessly complicated code. > >You need to check here also the name content, it is "GNU\x00" (n_namesz == 4). > > [AR], can it be any other name if type is NT_GNU_BUILD_ID? Added the > check but seems like an overkill to me. NT_GNU_BUILD_ID is just number 3. I find very realistic for example "SUNW Solaris" would use at least 3 note types (does not use it already?). > >>+/* Linear reverse find starting from RBEGIN towards REND looking for > >>+ the lowest vaddr mapping of the same inode and zero offset. */ > >>+ > >>+static mapping_entry_s * > >>+lrfind_mapping_entry (mapping_entry_s *const rbegin, > >>+ const mapping_entry_s *const rend) > >>+{ > >>+ mapping_entry_s *p; > >>+ > >>+ for (p = rbegin - 1; p >= rend && p->inode == rbegin->inode; --p) > >>+ if (p->offset == 0) > >>+ return p; > > > >I had here this layout: > >7ffff7ddc000-7ffff7dfd000 r-xp 00000000 fd:01 51415762 /usr/lib64/ld-2.17.so > >7ffff7ff9000-7ffff7ffa000 rw-p 00000000 00:00 0 > >7ffff7ffa000-7ffff7ffc000 r-xp 00000000 00:00 0 [vdso] > >7ffff7ffc000-7ffff7ffe000 rw-p 00020000 fd:01 51415762 /usr/lib64/ld-2.17.so > > > >so it should rather be: > > for (p = rbegin - 1; p >= rend; --p) > > if (p->offset == 0 && p->inode == rbegin->inode) > > return p; > > > >Fortunately it should not have any real performance impact. > > [AR] Ok, though we are not adding mappings with inode == 0. See > 'find_memory_region_callback'. Hmm, that's true, I do not see now why it failed for me. But that could be even non-zero-inode mapping so a change was appropriate. This is not yet a full review. Thanks, Jan