From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23291 invoked by alias); 13 Nov 2008 23:55:20 -0000 Received: (qmail 23163 invoked by uid 22791); 13 Nov 2008 23:55:19 -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:54:07 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 8A4551EE8EA; Thu, 13 Nov 2008 18:54:05 -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 ToAEGqCHvyyw; Thu, 13 Nov 2008 18:54:05 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 273E71EE8E5; Thu, 13 Nov 2008 18:54:05 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id DC049E7ACD; Thu, 13 Nov 2008 15:54:02 -0800 (PST) Date: Fri, 14 Nov 2008 11:36: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: <20081113235402.GA12802@adacore.com> References: <1221436875.17278.4.camel@localhost.localdomain> <20081113235027.GA12484@adacore.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20081113235027.GA12484@adacore.com> 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/msg00315.txt.bz2 [humpf, I ended up being interrupted while reviewing and forgot to write the end of the email before sending it. Here is, hopefully, the complete version] 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 ;-). Overall, the patch looks sane to me. Can you just wait a few more days, say until Monday to give the real experts a chance to yell at me ;-), and then commit? :REVIEWMAIL: -- Joel