Re: [rfc][1/2] add suport for 64-bit fpscr in powerpc linux native

[Joel Brobecker <brobecker@adacore.com>]

[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