Re: [RFA] Fix amd64 dwarf register number mapping

[Joel Brobecker <brobecker at adacore dot com>]

hello Pierre,

On Wed, Oct 01, 2014 at 06:10:47PM +0200, Pierre Muller wrote:
> dwarf register number is defined in
> "System V Application Binary Interface
> AMD64 Architecture Processor Supplement
> Draft Version 0.99.6"
> The amd64_dwarf_regmap array 
> is missing the 8 MMX registers
> in Figure 3.36: DWARF Register Number Mapping page 57.
> This leads to a wrong value for the 
> registers past this point.
> 
> I don't know if there are already
> cases where this really fixes a problem,
> but it is nevertheless a valid correction.
> 
> Is this OK, or is there a valid reason for
> this "miss"?
> 
> Pierre Muller
> Pascal language maintainer of GDB
> 
> 
> 
> 2014-10-01  Pierre Muller  <muller@sourceware.org>
> 
> 	* amd64-tdep.c (amd64_dwarf_regmap array): Fix amd64 dwarf
> 	register numbering, by adding missing MMX registers.

I am so sorry for such a long delay in reviewing your patch.
I was also able to confirm that GCC also uses 41-48 for those
registers, so your patch is also improving compatibility with
GCC (my main concern was to make sure that GDB's numbering was
not made on purpose for the sake of supporting something invalid
in GCC).

The patch is essentially approved, with some minor comments.
For my penitence, I made the adjustments, and pushed your patch
after having re-tested it on x86_64-linux. Comments below:

> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 7c8575a..b98bc5d 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -201,7 +201,11 @@ static int amd64_dwarf_regmap[] =
>    AMD64_ST0_REGNUM + 4, AMD64_ST0_REGNUM + 5,
>    AMD64_ST0_REGNUM + 6, AMD64_ST0_REGNUM + 7,
> 
> -  /* Control and Status Flags Register.  */
> +  /* MMX Registers 0 - 7. Currently not handled,
> +     see "tdep->num_mmx_regs = 0;" below.  */
> +  -1, -1, -1, -1, -1, -1, -1, -1,
> +
> +   /* Control and Status Flags Register.  */

You were adding a space before the "Control and Status" comment,
which is probably unintentional. I removed it.

I changed the command to explain that those registers are in fact
handled (see the routines that convert between GDB and dwarf numbers).
They just need special handling because the associated numbers in
GDB depend on the target, and on whether those registers are in fact
available or not.

Attached is what I pushed for you.

gdb/ChangeLog:

        Pushed by Joel Brobecker  <brobecker@adacore.com>.
        * amd64-tdep.c (amd64_dwarf_regmap array): Add missing MMX
        registers.

Tested on x86_64-linux.

-- 
Joel

>From f7ca3fcfccd144c234370aa939e4f5f15f3b2a88 Mon Sep 17 00:00:00 2001
From: Pierre Muller <muller@sourceware.org>
Date: Fri, 28 Nov 2014 19:21:58 +0400
Subject: [PATCH] Fix amd64 dwarf register number mapping (MMX register and
 higher)

Dwarf register numbers are defined in "System V Application Binary
Interface AMD64 Architecture Processor Supplement Draft Version 0.99.6"

The amd64_dwarf_regmap array is missing the 8 MMX registers in Figure
3.36: DWARF Register Number Mapping page 57.  This leads to a wrong
value for the registers past this point.

gdb/ChangeLog:

        Pushed by Joel Brobecker  <brobecker@adacore.com>.
        * amd64-tdep.c (amd64_dwarf_regmap array): Add missing MMX
        registers.

Tested on x86_64-linux.
---
 gdb/ChangeLog    | 6 ++++++
 gdb/amd64-tdep.c | 8 +++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 6b5c02a..21e1a7e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2014-11-28  Pierre Muller  <muller@sourceware.org>
+
+	Pushed by Joel Brobecker  <brobecker@adacore.com>.
+	* amd64-tdep.c (amd64_dwarf_regmap array): Add missing MMX
+	registers.
+
 2014-11-28  Ulrich Weigand  <uweigand@de.ibm.com>
 
 	* config/ia64/linux.mh (NATDEPFILES): Remove core-regset.o.
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index e69da01..7bc4694 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -199,7 +199,13 @@ static int amd64_dwarf_regmap[] =
   AMD64_ST0_REGNUM + 2, AMD64_ST0_REGNUM + 3,
   AMD64_ST0_REGNUM + 4, AMD64_ST0_REGNUM + 5,
   AMD64_ST0_REGNUM + 6, AMD64_ST0_REGNUM + 7,
-  
+
+  /* MMX Registers 0 - 7.
+     We have to handle those registers specifically, as their register
+     number within GDB depends on the target (or they may even not be
+     available at all).  */
+  -1, -1, -1, -1, -1, -1, -1, -1,
+
   /* Control and Status Flags Register.  */
   AMD64_EFLAGS_REGNUM,
 
-- 
1.9.1