This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 04/11] Little-endian fixes: VSX tests and pseudo-regs
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Ulrich Weigand <uweigand at de dot ibm dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Wed, 29 Jan 2014 08:33:20 +0400
- Subject: Re: [PATCH 04/11] Little-endian fixes: VSX tests and pseudo-regs
- Authentication-results: sourceware.org; auth=none
- References: <201401281727 dot s0SHRShk018211 at d06av02 dot portsmouth dot uk dot ibm dot com>
> ChangeLog:
>
> * rs6000-tdep.c (efpr_pseudo_register_read): Use correct offset
> of the overlapped FP register within the VSX register on little-
> endian platforms.
> (efpr_pseudo_register_write): Likewise.
>
> testsuite/ChangeLog:
>
> * gdb.arch/vsx-regs.exp: Check target endianness. Provide variants
> of the test patterns for use on little-endian systems.
I only have small comments on this patch...
>
>
> Index: binutils-gdb/gdb/rs6000-tdep.c
> ===================================================================
> --- binutils-gdb.orig/gdb/rs6000-tdep.c
> +++ binutils-gdb/gdb/rs6000-tdep.c
> @@ -2781,7 +2781,8 @@ efpr_pseudo_register_read (struct gdbarc
> int reg_index = reg_nr - tdep->ppc_efpr0_regnum;
>
> /* Read the portion that overlaps the VMX register. */
> - return regcache_raw_read_part (regcache, tdep->ppc_vr0_regnum + reg_index, 0,
> + int offset = gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG ? 0 : 8;
> + return regcache_raw_read_part (regcache, tdep->ppc_vr0_regnum + reg_index, offset,
> register_size (gdbarch, reg_nr), buffer);
Can you add an empty line after the local variable declaration?
Also, the return statement now has a line that's too long, and so
needs to be broken up.
> @@ -2794,7 +2795,8 @@ efpr_pseudo_register_write (struct gdbar
> int reg_index = reg_nr - tdep->ppc_efpr0_regnum;
>
> /* Write the portion that overlaps the VMX register. */
> - regcache_raw_write_part (regcache, tdep->ppc_vr0_regnum + reg_index, 0,
> + int offset = gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG ? 0 : 8;
> + regcache_raw_write_part (regcache, tdep->ppc_vr0_regnum + reg_index, offset,
> register_size (gdbarch, reg_nr), buffer);
Same here.
> Index: binutils-gdb/gdb/testsuite/gdb.arch/vsx-regs.exp
> ===================================================================
> --- binutils-gdb.orig/gdb/testsuite/gdb.arch/vsx-regs.exp
> +++ binutils-gdb/gdb/testsuite/gdb.arch/vsx-regs.exp
> @@ -58,19 +58,46 @@ if ![runto_main] then {
> gdb_suppress_tests
> }
>
> +send_gdb "show endian\n"
> +set endianness ""
> +gdb_expect {
> + -re "(The target endianness is set automatically .currently )(big|little)( endian.*)$gdb_prompt $" {
> + pass "endianness"
> + set endianness $expect_out(2,string)
> + }
> + -re ".*$gdb_prompt $" {
> + fail "couldn't get endianness"
> + }
> + timeout { fail "(timeout) endianness" }
Can you use test_gdb_multiple instead? That should allow you to simplify
this a little by not having to handle the timeout. Generally speaking,
we now prefer not to use send_gdb+gdb_expect unless we absolutely have to.
--
Joel