From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22335 invoked by alias); 3 Sep 2013 12:35:56 -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 22316 invoked by uid 89); 3 Sep 2013 12:35:56 -0000 Received: from anubis.se.axis.com (HELO anubis.se.axis.com) (195.60.68.12) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Sep 2013 12:35:56 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.2 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: anubis.se.axis.com Received: from localhost (localhost [127.0.0.1]) by anubis.se.axis.com (Postfix) with ESMTP id CFE8219DF6; Tue, 3 Sep 2013 14:35:52 +0200 (CEST) Received: from anubis.se.axis.com ([127.0.0.1]) by localhost (anubis.se.axis.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id JNosUktJgBT3; Tue, 3 Sep 2013 14:35:51 +0200 (CEST) Received: from boulder.se.axis.com (boulder.se.axis.com [10.0.2.104]) by anubis.se.axis.com (Postfix) with ESMTP id 31A0419DD9; Tue, 3 Sep 2013 14:35:47 +0200 (CEST) Received: from boulder.se.axis.com (localhost [127.0.0.1]) by postfix.imss71 (Postfix) with ESMTP id 8E17E56B; Tue, 3 Sep 2013 14:35:47 +0200 (CEST) Received: from thoth.se.axis.com (thoth.se.axis.com [10.0.2.173]) by boulder.se.axis.com (Postfix) with ESMTP id 7FE671D3; Tue, 3 Sep 2013 14:35:47 +0200 (CEST) Received: from xmail2.se.axis.com (xmail2.se.axis.com [10.0.5.74]) by thoth.se.axis.com (Postfix) with ESMTP id 7D0163404E; Tue, 3 Sep 2013 14:35:47 +0200 (CEST) Received: from lnxricardw.se.axis.com (10.88.7.1) by xmail2.se.axis.com (10.0.5.74) with Microsoft SMTP Server (TLS) id 8.2.255.0; Tue, 3 Sep 2013 14:35:47 +0200 Date: Tue, 03 Sep 2013 12:35:00 -0000 From: Ricard Wanderlof To: Pedro Alves CC: "gdb-patches@sourceware.org" Subject: Re: [PATCH][CRISv32] Add support for threaded debugging In-Reply-To: <5224D319.1000704@redhat.com> Message-ID: References: <5224D319.1000704@redhat.com> User-Agent: Alpine 2.00 (DEB 1167 2008-08-23) MIME-Version: 1.0 Content-Type: text/plain; format=flowed; charset="ISO-8859-15" Content-Transfer-Encoding: 8BIT X-SW-Source: 2013-09/txt/msg00069.txt.bz2 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. 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, 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? >> >> 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). /Ricard -- Ricard Wolf Wanderlöf ricardw(at)axis.com Axis Communications AB, Lund, Sweden www.axis.com Phone +46 46 272 2016 Fax +46 46 13 61 30