Re: [RFC] mips-tdep.c: Update mips_register_to_value(), et al...

Kevin Buettner
2010-12-11T00:04:28Z

On Thu, 9 Dec 2010 12:06:17 +0100 (CET)
Mark Kettenis <mark.kettenis@xs4all.nl> wrote:

> So what if we were adding a 32-bit unsigned integer to a 64-bit
> integer signed integer? Then it would presumably be using the 64-bit
> daddu instruction, and sign extending the 32-bit value when placing it
> into the register would yield the wrong result wouldn't it?

It turns out that the result is still correct in this case. I must
admit though that I thought that you were right at first. I even
began adjusting my patch to zero extend for unsigned types. But, as I
was making those adjustments, I became less and less certain about the
correctness of those revisions. (I think I had run into something
like this w/ mips64 in the distant past...)

I decided to look at the way that compiled code (using a recent gcc)
creates unsigned 32-bit values and how such values end up being
(possibly) converted when combined with signed 64-bit values. I
created a few small functions mostly cribbed from those in
gdb.base/store.c. The key function is add_uisl, which performs the
operation in your example:

long
add_uisl (register unsigned int u, register signed long v)
{
return u + v;
}

When I disassemble this with GDB, I see:

Dump of assembler code for function add_uisl:
32 {
0xffffffff800203bc <+0>: daddiu sp,sp,-8
0xffffffff800203c0 <+4>: sd s8,0(sp)
0xffffffff800203c4 <+8>: move s8,sp
0xffffffff800203c8 <+12>: move v1,a0
0xffffffff800203cc <+16>: move v0,a1

33 return u + v;
0xffffffff800203d0 <+20>: dsll32 v1,v1,0x0
0xffffffff800203d4 <+24>: dsrl32 v1,v1,0x0
=> 0xffffffff800203d8 <+28>: daddu v0,v1,v0

34 }
0xffffffff800203dc <+32>: move sp,s8
0xffffffff800203e0 <+36>: ld s8,0(sp)
0xffffffff800203e4 <+40>: daddiu sp,sp,8
0xffffffff800203e8 <+44>: jr ra
0xffffffff800203ec <+48>: nop

Note the pair of instructions, dsll32 and dsrl32, which precede the
daddu instruction. The dsll32 instruction is shifting v1 left by 32
bits and storing the result back into v1. The dsrl32 instruction is
doing the opposite (a right shift of 32 bits) where the top half of
the word is zeroed out. So, in effect, this pair of instructions
represent the type conversion operation from (unsigned int) to (signed
long).

When I compile it with -O2, the code ends up being a good deal smaller,
but the type conversion is still present:

32 {

33 return u + v;
0xffffffff80020318 <+0>: dsll32 v0,a0,0x0
0xffffffff8002031c <+4>: dsrl32 v0,v0,0x0

34 }
0xffffffff80020320 <+8>: jr ra
0xffffffff80020324 <+12>: daddu v0,a1,v0

Also of interest is the point at which a large unsigned 32-bit constant
is created:

276 wack_uisl ((unsigned int) 0x80000000, -2);
0xffffffff800211d8 <+64>: lui a0,0x8000
0xffffffff800211dc <+68>: li a1,-2
0xffffffff800211e0 <+72>: jal 0xffffffff80020654 <wack_uisl>
0xffffffff800211e4 <+76>: nop

The document, "MIPS64 Architecture For Programmers Volume II: The
MIPS64 Instruction Set", describes the LUI instruction as follows:

The 16-bit immediate is shifted left 16 bits and concatenated with
16 bits of low-order zeros. The 32-bit result is sign-extended
and placed into GPR rt.

This is what I see after executing that instruction in the debugger:

1: x/i $pc
=> 0xffffffff800211d8 <main+64>: lui a0,0x8000
(gdb) info reg a0
a0: 0xffffffffffffffff
(gdb) si
0xffffffff800211dc 276 wack_uisl ((unsigned int) 0x80000000, -2);
1: x/i $pc
=> 0xffffffff800211dc <main+68>: li a1,-2
(gdb) info reg a0
a0: 0xffffffff80000000

(I won't show it here, but the result is the same even when I zero out
a0 prior to stepping the lui instruction.)

Given that the compiler creates unsigned 32-bit constants as signed
values in 32-bit registers, I think that is what GDB should do as
well.

>
> > (Can anyone think of better names for the two new functions that I
> > introduced?)
>
> Yeah, they're pretty horrific.

I've adjusted those function names in the patch below. I ended up
using the names that I discussed with Yao:

mips_convert_register_float_case_p
mips_convert_register_gpreg_case_p

I've also added a few comments including one noting the fact that we
perform the sign extension on values shorter than 8-bytes without regard
for type.

Any further comments?

Kevin

* mips-tdep.c (mips_convert_register_float_case_p)
(mips_convert_register_gpreg_case_p): New functions.
(mips_convert_register_p): Invoke new functions above.
(mips_register_to_value): Add case for fetching value shorter
than 64 bits from a 64-bit register.
(mips_value_to_register): Add case for storing value shorter
than 64 bits into a 64-bit register.

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.508
diff -u -p -r1.508 mips-tdep.c
--- mips-tdep.c 28 Nov 2010 04:31:24 -0000 1.508
+++ mips-tdep.c 10 Dec 2010 22:36:48 -0000
@@ -625,8 +635,13 @@ set_mips64_transfers_32bit_regs (char *a

/* Convert to/from a register and the corresponding memory value. */

+/* This predicate tests for the case of an 8 byte floating point
+ value that is being transferred to or from a pair of floating point
+ registers each of which are (or are considered to be) only 4 bytes
+ wide. */
static int
-mips_convert_register_p (struct gdbarch *gdbarch, int regnum, struct type *type)
+mips_convert_register_float_case_p (struct gdbarch *gdbarch, int regnum,
+ struct type *type)
{
return (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG
&& register_size (gdbarch, regnum) == 4
@@ -637,20 +652,99 @@ mips_convert_register_p (struct gdbarch
&& TYPE_CODE (type) == TYPE_CODE_FLT && TYPE_LENGTH (type) == 8);
}

+/* This predicate tests for the case of a value of less than 8
+ bytes in width that is being transfered to or from an 8 byte
+ general purpose register. */
+static int
+mips_convert_register_gpreg_case_p (struct gdbarch *gdbarch, int regnum,
+ struct type *type)
+{
+ int num_regs = gdbarch_num_regs (gdbarch);
+
+ return (register_size (gdbarch, regnum) == 8
+ && regnum % num_regs > 0 && regnum % num_regs < 32
+ && TYPE_LENGTH (type) < 8);
+}
+
+static int
+mips_convert_register_p (struct gdbarch *gdbarch, int regnum, struct type *type)
+{
+ return mips_convert_register_float_case_p (gdbarch, regnum, type)
+ || mips_convert_register_gpreg_case_p (gdbarch, regnum, type);
+}
+
static void
mips_register_to_value (struct frame_info *frame, int regnum,
struct type *type, gdb_byte *to)
{
- get_frame_register (frame, regnum + 0, to + 4);
- get_frame_register (frame, regnum + 1, to + 0);
+ struct gdbarch *gdbarch = get_frame_arch (frame);
+
+ if (mips_convert_register_float_case_p (gdbarch, regnum, type))
+ {
+ get_frame_register (frame, regnum + 0, to + 4);
+ get_frame_register (frame, regnum + 1, to + 0);
+ }
+ else if (mips_convert_register_gpreg_case_p (gdbarch, regnum, type))
+ {
+ int len = TYPE_LENGTH (type);
+ if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
+ get_frame_register_bytes (frame, regnum, 8 - len, len, to);
+ else
+ get_frame_register_bytes (frame, regnum, 0, len, to);
+ }
+ else
+ {
+ internal_error (__FILE__, __LINE__,
+ _("mips_register_to_value: unrecognized case"));
+ }
}

static void
mips_value_to_register (struct frame_info *frame, int regnum,
struct type *type, const gdb_byte *from)
{
- put_frame_register (frame, regnum + 0, from + 4);
- put_frame_register (frame, regnum + 1, from + 0);
+ struct gdbarch *gdbarch = get_frame_arch (frame);
+
+ if (mips_convert_register_float_case_p (gdbarch, regnum, type))
+ {
+ put_frame_register (frame, regnum + 0, from + 4);
+ put_frame_register (frame, regnum + 1, from + 0);
+ }
+ else if (mips_convert_register_gpreg_case_p (gdbarch, regnum, type))
+ {
+ gdb_byte fill[8];
+ int len = TYPE_LENGTH (type);
+
+ /* Sign extend values, irrespective of type, that are stored to
+ a 64-bit general purpose register. (32-bit unsigned values
+ are stored as signed quantities within a 64-bit register.
+ When performing an operation, in compiled code, that combines
+ a 32-bit unsigned value with a signed 64-bit value, a type
+ conversion is first performed that zeroes out the high 32 bits.) */
+ if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
+ {
+ if (from[0] & 0x80)
+ store_signed_integer (fill, 8, BFD_ENDIAN_BIG, -1);
+ else
+ store_signed_integer (fill, 8, BFD_ENDIAN_BIG, 0);
+ put_frame_register_bytes (frame, regnum, 0, 8 - len, fill);
+ put_frame_register_bytes (frame, regnum, 8 - len, len, from);
+ }
+ else
+ {
+ if (from[len-1] & 0x80)
+ store_signed_integer (fill, 8, BFD_ENDIAN_LITTLE, -1);
+ else
+ store_signed_integer (fill, 8, BFD_ENDIAN_LITTLE, 0);
+ put_frame_register_bytes (frame, regnum, 0, len, from);
+ put_frame_register_bytes (frame, regnum, len, 8 - len, fill);
+ }
+ }
+ else
+ {
+ internal_error (__FILE__, __LINE__,
+ _("mips_value_to_register: unrecognized case"));
+ }
}

/* Return the GDB type object for the "standard" data type of data in