From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22713 invoked by alias); 11 Mar 2013 14:25:24 -0000 Received: (qmail 22704 invoked by uid 22791); 11 Mar 2013 14:25:23 -0000 X-SWARE-Spam-Status: No, hits=-4.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL X-Spam-Check-By: sourceware.org Received: from na3sys009aog109.obsmtp.com (HELO na3sys009aog109.obsmtp.com) (74.125.149.201) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 11 Mar 2013 14:25:16 +0000 Received: from mx20.qnx.com ([72.1.200.103]) (using TLSv1) by na3sys009aob109.postini.com ([74.125.148.12]) with SMTP ID DSNKUT3pS2LN2ery4k+Rmv0WHqNcgZyu4zH6@postini.com; Mon, 11 Mar 2013 07:25:16 PDT Received: by mx20.qnx.com (Postfix, from userid 500) id AA2C020E55; Mon, 11 Mar 2013 10:25:14 -0400 (EDT) Received: from exhts.ott.qnx.com (exch2 [10.222.2.136]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by mx20.qnx.com (Postfix) with ESMTPS id 37DDC1FA62; Mon, 11 Mar 2013 10:25:14 -0400 (EDT) Received: from [10.222.96.215] (10.222.2.5) by EXCH2.ott.qnx.com (10.222.2.136) with Microsoft SMTP Server (TLS) id 14.2.318.4; Mon, 11 Mar 2013 10:25:13 -0400 Message-ID: <513DE949.6030508@qnx.com> Date: Mon, 11 Mar 2013 14:25:00 -0000 From: Aleksandar Ristovski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130221 Thunderbird/17.0.3 MIME-Version: 1.0 To: Jan Kratochvil CC: "gdb-patches@sourceware.org" Subject: Re: [draft patch 0/6] Split FYI and some review notes References: <51278984.3070208@qnx.com> <20130310210734.GA21130@host2.jankratochvil.net> In-Reply-To: <20130310210734.GA21130@host2.jankratochvil.net> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit 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 X-SW-Source: 2013-03/txt/msg00463.txt.bz2 On 13-03-10 05:07 PM, Jan Kratochvil wrote: > Hello Aleksandar, Hello Jan, > > to send something here is some draft, there will be still some changes. > > I had first difficulties to apply it to FSF GDB HEAD as it has some conflicts > so I had to rebase it first. It applied cleanly when I posted it, I rebased it and made sure it applies to cleanly cvs checked out tree. Sorry if you had difficulties applying the patch, I could have rebased it for you. > > It is not a normal review as your patch has merged code moves + code > modifications which is unreviewable. Moves and modifications need to be in > separate patches so that one can see what has changed in the diff. I thought it is better to keep the whole change as a single blob so it can be easily reverted should there be any problems. But I can continue with the sequence of patches as you broked them down. > > fileio_read_stralloc you have reimplemented for gdbserver; I have rather > chosen to generalize the gdb/ variant and call the same common/ code from both > gdb and gdbserver. One may ask whether it is not more complicated than its > reimplementation, not sure. > > By moving linux_find_memory_regions_full you have dropped make_cleanup there. > Its use from GDB may now leak memory as its called FUNC may throw an > exception. I have put there 'void **memory_to_free_ptr' to make it reusable > from both gdb and gdbserver. > > I tried to make the patches similar to your code so that one can diff the > result against your patch. > > There were many code formatting little changes and also needless casts > (probably from C++). > > For the last [draft patch 6/6] - things not done there yet: > > get_dynamic already parses/scans PHDRs, you introduce new PHDR parser, they > should be merged. > > get_hex_build_id must not be based on soname, moreover playing with realname > on it, it is too fragile. It should be based on addresses, you know that l_ld > is absolute address inside the library. So find maps/smaps entry containing > l_ld, subtract its file offset from vaddr and you have the ELF header address. > > Do not run get_hex_build_id quadratically - currently for each library you > open and scan maps/smaps again. Probably the best approach is to scan all > l_ld addresses from r_debug first, then qsort them and then linearly match > them to the maps/smaps entries. I am not sure but I guess they still should > be returned to GDB in their original order from r_debug for some backward and > solib-svr4.c compatibility. Also Gary Benson is working on incremental shlib > transfers from gdbserver so that it does not clash too much (it will anyway). > While just quadratic computation would be in real world acceptable all the > open/read/close syscalls I find really needlessly expensive. I thought it would be an overkill to make it any more optimal. Ideal situation would be to send "library loaded"/ "library unloaded" events instead of sending the whole list each time. But sure, I'll do the qsort. > > PT_NOTE segment contains arbitrary number of notes, up to its size. You check > only the first entry there. Ok. > > On Fri, 22 Feb 2013 16:06:44 +0100, Aleksandar Ristovski wrote: >> As per Jan's request, this patch adds 'build-id' to the gdbserver >> response. The value is hex encoded string representing >> .note.gnu.build-id section of the corresponding shared library. > > gdbserver reports PT_NOTE segment, not any section. gdbserver is dependent on > runtime information (segments), it cannot rely on debug/link information > (sections). My references to the sections is to denote we are dealing with the contents of the section (which is mapped to the segment). > > BTW ping me (possibly off-list) whether you plan to work on it now yourself or > whether I should be doing more of the changes described above; I sure have > also enough other work... If you feel strongly, you can take it over. Otherwise, I will finish it. I plan some follow up changes to all this. If you want the patches to be attributed to you, that is also fine by me, let me know. > > > Thanks, > Jan > Thanks, Aleksandar