From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28083 invoked by alias); 11 Dec 2010 00:04:28 -0000 Received: (qmail 28075 invoked by uid 22791); 11 Dec 2010 00:04:26 -0000 X-SWARE-Spam-Status: No, hits=-6.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_SR,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 11 Dec 2010 00:04:18 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id oBB04HwM005235 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 10 Dec 2010 19:04:17 -0500 Received: from mesquite.lan ([10.3.113.8]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id oBB04GFU007018 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 10 Dec 2010 19:04:17 -0500 Date: Sat, 11 Dec 2010 00:04:00 -0000 From: Kevin Buettner To: gdb-patches@sourceware.org Cc: Mark Kettenis Subject: Re: [RFC] mips-tdep.c: Update mips_register_to_value(), et al... Message-ID: <20101210170416.64586a1c@mesquite.lan> In-Reply-To: <201012091106.oB9B6HvF003270@glazunov.sibelius.xs4all.nl> References: <20101208164657.7d9ce88e@mesquite.lan> <201012091106.oB9B6HvF003270@glazunov.sibelius.xs4all.nl> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-IsSubscribed: yes 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: 2010-12/txt/msg00152.txt.bz2 On Thu, 9 Dec 2010 12:06:17 +0100 (CET) Mark Kettenis 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 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 : lui a0,0x8000 (gdb) info reg a0 a0: 0xffffffffffffffff (gdb) si 0xffffffff800211dc 276 wack_uisl ((unsigned int) 0x80000000, -2); 1: x/i $pc => 0xffffffff800211dc : 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