From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14348 invoked by alias); 13 Nov 2008 23:51:10 -0000 Received: (qmail 14225 invoked by uid 22791); 13 Nov 2008 23:51:09 -0000 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 13 Nov 2008 23:50:32 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 7D4CF1EE8E5; Thu, 13 Nov 2008 18:50:30 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id V7b2KqaNFReK; Thu, 13 Nov 2008 18:50:30 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 0AAC91EE8DD; Thu, 13 Nov 2008 18:50:29 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 60496E7ACD; Thu, 13 Nov 2008 15:50:27 -0800 (PST) Date: Fri, 14 Nov 2008 11:32:00 -0000 From: Joel Brobecker To: Thiago Jung Bauermann Cc: gdb-patches ml Subject: Re: [rfc][1/2] add suport for 64-bit fpscr in powerpc linux native Message-ID: <20081113235027.GA12484@adacore.com> References: <1221436875.17278.4.camel@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1221436875.17278.4.camel@localhost.localdomain> User-Agent: Mutt/1.4.2.2i 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: 2008-11/txt/msg00314.txt.bz2 I will start by saying that I don't know the various powerpc variants very well, but your patch sat unreviewed for a long time, so I have to trust your judgement in the way you separated the ia205 variants from the rest. From what I can tell, this is pretty much the standard way of doing this, so this looks fine. > gdb/ > * ppc-linux-nat.c (ppc_register_u_addr): Add special case to return > offset for full 64-bit slot of FPSCR when in 32-bits. > (ppc_linux_read_description): Return target description with 64-bit > FPSCR when inferior is running on an ISA 2.05 or later processor. > * ppc-linux-tdep.c (_initialize_ppc_linux_tdep): Call > initialize_tdec_powerpc_isa205_32l, > initialize_tdec_powerpc_isa205_altivec32l, > initialize_tdec_powerpc_isa205_vsx32l, > initialize_tdec_powerpc_isa205_64l, > initialize_tdec_powerpc_isa205_altivec64l and > initialize_tdec_powerpc_isa205_vsx64l. > * ppc-linux-tdep.h: Add external declaration for > tdesc_powerpc_isa205_32l, tdesc_powerpc_isa205_altivec32l, > tdesc_powerpc_isa205_vsx32l, tdesc_powerpc_isa205_64l, > tdesc_powerpc_isa205_altivec64l and tdesc_powerpc_isa205_vsx64l. > * features/rs600/powerpc-fpu-isa205.xml: New file. > * features/rs600/powerpc-isa205-32l.xml: New file. > * features/rs600/powerpc-isa205-64l.xml: New file. > * features/rs600/powerpc-isa205-altivec32l.xml: New file. > * features/rs600/powerpc-isa205-altivec64l.xml: New file. > * features/rs600/powerpc-isa205-vsx32l.xml: New file. > * features/rs600/powerpc-isa205-vsx64l.xml: New file. > * features/rs600/powerpc-isa205-32l.c: Generate. > * features/rs600/powerpc-isa205-64l.c: Generate. > * features/rs600/powerpc-isa205-altivec32l.c: Generate. > * features/rs600/powerpc-isa205-altivec64l.c: Generate. > * features/rs600/powerpc-isa205-vsx32l.c: Generate. > * features/rs600/powerpc-isa205-vsx64l.c: Generate. > > gdb/testsuite/ > > * gdb.arch/ppc-dfp.exp: New file. > * gdb.arch/ppc-dfp.c: New file. > +#ifndef PPC_FEATURE_ARCH_2_05 > +#define PPC_FEATURE_ARCH_2_05 0x00001000 /* ISA 2.05 */ > +#endif How would this macro be defined? Does it come from one of the system includes? What does this macro contain? Perhaps a comment describing this macro would be useful. (a few minutes later, after I started looking at the change inside ppc_linux_read_description, I think I get it, now...) > + /* If the FPSCR is 64-bit wide, we need to fetch the whole 64-bit > + slot and not just its second word. The PT_FPSCR supplied in a > + 32-bit GDB compilation doesn't reflect this. */ 32-bit GDB compilation: Do you mean a 32-bit GDB? While you're modifying this part of the code, I noticed a typo in a comment just above: /* NOTE: cagney/2005-02-08: On some 64-bit GNU/Linux systems the kernel headers incorrectly contained the 32-bit definition of PT_FPSCR. For the 32-bit definition, floating-point registers occupy two 32-bit "slots", and the FPSCR lives in the secondhalf of such a slot-pair (hence +1). For 64-bit, ^^^^^^^^^^ missing space between second and half. Thanks! > +# Please email any bugs, comments, and/or additions to this file to: > +# bug-gdb@prep.ai.mit.edu Could you remove this part form your copyright header? > +gdb_test "print/t \$fpscr" " = 10100000000000000000000000000000000" "FPSCR for round to nearest, ties toward zero rounding mode" Just a casual remark, not a request for change, but I'm surprised that you might find /t more useful in this case than /x ;-). :REVIEWMAIL: -- Joel