From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32592 invoked by alias); 3 Sep 2013 14:32:47 -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 32580 invoked by uid 89); 3 Sep 2013 14:32:46 -0000 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Sep 2013 14:32:46 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.8 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r83EWfma031408 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 3 Sep 2013 10:32:41 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r83EWahO017738; Tue, 3 Sep 2013 10:32:40 -0400 Message-ID: <5225F303.4050901@redhat.com> Date: Tue, 03 Sep 2013 14:32:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Ricard Wanderlof CC: "gdb-patches@sourceware.org" Subject: Re: [PATCH][CRISv32] Add support for threaded debugging References: <5224D319.1000704@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-09/txt/msg00076.txt.bz2 On 09/03/2013 01:35 PM, Ricard Wanderlof wrote: > > On Mon, 2 Sep 2013, Pedro Alves wrote: > >> (fixing subject) > > Thanks. > >>> 2013-08-30 Ricard Wanderlof >>> >>> * cris-tdep.c: Fix typedef for elf_greg_t. >>> (cris_gdbarch_init): Add call to >>> set_gdbarch_fetch_tls_load_module_address. >> >> Indentation doesn't look right. Should be indented >> with a single tab. > > Thanks for pointing it out. Will fix. > >>> Suggested-by: Edgar Iglisias >> >> (Note: surname/address don't match) > > Thanks for spotting that. Edgar used to work at the same company I do but > does not any more so I had to piece this together manually. > >>> 2013-08-30 Ricard Wanderlof >>> >>> * cris-tdep.c: Fix typedef for elf_greg_t. >> >> This seems to be unrelated to threaded debugging support. >> It just looks like a host-dependency fix. Please keep logically unrelated >> patches separate, and always send them as separate threads each with their >> own rationale. > > Sure. That was the intention, like I said the patch as such has been > applied locally for some time and I decided to submit it, not really > considering all the details. > > I will split this into two patches then. > >> (Note that "fix" is not "what" changed, but "why" you changed it, so >> it's not the proper ChangeLog description of the change.) > > Ok. > >> In this case, the fix should just go the extra mile and remove the >> reliance on host alignment from this type, that is representing an >> external structure. IOW, that typedef, if any, would better >> be: >> >> typedef gdb_byte cris_elf_greg_t[4]; >> >> See frv_linux_supply_gregset for example. > > Ok, I'll do something similar then. I see that MIPS has a similar > construct. > >>> (cris_gdbarch_init): Add call to >>> set_gdbarch_fetch_tls_load_module_address. >> >> This part looks OK, though it did raise some eyebrows to have >> GNU/Linux-specific code in cris-tdep.c, rather than in a cris-linux-tdep.c >> file. It seems there's no real support for cris bare-metal debugging? > > Virtually all CRIS systems run Linux, and GDB is mostly used for > user-space debugging. There is a GDB stub available for the Linux kernel, > which I guess would qualify as bare metal debugging, but as far as I know > it is virtually never used and the support in the kernel may even be > broken by know. Thanks for the clarification. > > So as you say the file should probably be split, but at this point in time > I don't really see the need for it. It doesn't seem like it would be a > huge effort (essentially the call to > set_gdbarch_fetch_tls_load_module_address and also > set_solib_svr4_fetch_link_map_offsets would be put in cris-linux-tdep.c), > on the other hand I can't really test that it works as expected, I'd be happy enough if the GNU/Linux port keeps working. > so perhaps the best solution for now is to add a FIXME, and to split the > files when the need comes up and the result can be tested properly? It's mostly about code/design/maintenance sanity. I won't really mind if the split isn't done, but note how the fact that there's a Linux port here is being missed often in regular maintenance (because people will look for *linux-tdep.*) files. cris-tdep.c doesn't call linux_init_abi anywhere AFAICT, for example, so the cris port lost the adjustment between v1 and v2 of the gdbarch_gdb_signal_{to,from}_target patches, just a few weeks back: http://sourceware.org/ml/gdb-patches/2013-07/msg00002.html http://sourceware.org/ml/gdb-patches/2013-07/msg00651.html Probably other across-the-board changes have been missed. > >>> >>> gdbserver >>> >>> * linux-crisv32-low.c (ps_get_thread_area): New function. >> >> This part is OK, though should mention also the addition of >> PTRACE_GET_THREAD_AREA, and, > > Ok, will add that. > >>> + *base = (void *) ((char *)*base - idx); >> >> missing space after '(char *) '. > > Ok, will fix that. > > I'll fix the issues and submit a new patch (two patches). Thanks. -- Pedro Alves