From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14078 invoked by alias); 4 Feb 2003 00:04:58 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 14059 invoked from network); 4 Feb 2003 00:04:56 -0000 Received: from unknown (HELO zenia.red-bean.com) (66.244.67.22) by 172.16.49.205 with SMTP; 4 Feb 2003 00:04:56 -0000 Received: from zenia.red-bean.com (localhost.localdomain [127.0.0.1]) by zenia.red-bean.com (8.12.5/8.12.5) with ESMTP id h13Ntn8A028297; Mon, 3 Feb 2003 18:55:49 -0500 Received: (from jimb@localhost) by zenia.red-bean.com (8.12.5/8.12.5/Submit) id h13NtlTw028293; Mon, 3 Feb 2003 18:55:47 -0500 To: Gerhard Tonn Cc: ton@de.ibm.com, gdb-patches@sources.redhat.com Subject: Re: [Patch] Fix ABI incompatibilities on s390x References: <03020212430000.00722@tau> From: Jim Blandy Date: Tue, 04 Feb 2003 00:04:00 -0000 In-Reply-To: <03020212430000.00722@tau> Message-ID: User-Agent: Gnus/5.09 (Gnus v5.9.0) Emacs/21.2.92 MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2003-02/txt/msg00097.txt.bz2 Okay, this looks good. You're testing on both the s390 and the s390x, right? Assuming you are, then I think we're ready to start the legal machinery. I don't know if this is a problem, but this patch doesn't apply to today's sources: the write_register_gen calls need to be changed to deprecated_write_register_gen calls, given Andrew's change from November: 2002-11-02 Andrew Cagney * regcache.h (deprecated_read_register_gen): Rename read_register_gen. (deprecated_write_register_gen): Rename write_register_gen. * i387-tdep.c: Update. * x86-64-linux-nat.c: Update * wince.c: Update. * thread-db.c: Update. * win32-nat.c: Update. * mips-tdep.c: Update. * d10v-tdep.c: Update. * cris-tdep.c: Update. * remote-sim.c: Update. * remote-rdi.c: Update. * remote-rdp.c: Update. * frame.c: Update. * target.c: Update. * blockframe.c: Update. * x86-64-tdep.c: Update. * xstormy16-tdep.c: Update. * sh-tdep.c: Update. * s390-tdep.c: Update. * rs6000-tdep.c: Update. * sparc-tdep.c: Update. * i386-tdep.c: Update. * dwarf2cfi.c: Update. * regcache.c: Update. If you're working with pre-November sources, you may want to update and re-test. Gerhard Tonn writes: > > I'm afraid I have the same concerns about this patch as I did about > > the first one. > > > It seems that there are two main differences between the s390 and > > s390x ABI's: > > - the s390 registers are larger. > > - the s390x does not have a special DOUBLE_ARG class of arguments that > > it handles specially. > > > Most of the first sort of difference you can account for using > > REGISTER_SIZE, but there is one case that cannot be handled this way. > > You handle this by testing GDB_TARGET_IS_ESAME. > > > Since there are several bits of code that know about the existence of > > DOUBLE_ARGs (i.e., we have to exclude DOUBLE_ARGs from the other > > classes of arguments), you handle each of those by testing > > GDB_TARGET_IS_ESAME. > > > Please handle the first case by defining a function: > > > static int > > is_power_of_two (unsigned int n) > > { > > return ((n & (n-1)) == 0); > > } > > > and then saying ! (is_power_of_two (length) && length <= REGISTER_SIZE) > > in pass_by_copy_ref. > > > Please handle the second case by explicitly calling is_double_arg from > > is_simple_arg, and then change is_double_arg as follows: > > > static int > > is_double_arg (struct type *type) > > { > > unsigned length = TYPE_LENGTH (type); > > > > /* The s390x ABI doesn't handle DOUBLE_ARGS specially. */ > > if (GDB_TARGET_IS_ESAME) > > return 0; > > > > return ((is_integer_like (type) > > || is_struct_like (type)) > > && length == 8); > > } > > Good idea. Thanks for your time and your help. > > BTW., I have run the test suite after each of my changes. The results look > pretty good. > > Gerhard > > > 2003-02-02 Gerhard Tonn > > * s390-tdep.c (is_simple_arg): Substitute hardcoded register size by > macro REGISTER_SIZE and check for double args. Long double checks > are dropped, since they are not supported by s390 and s390x. > (is_power_of_two): New. > (pass_by_copy_ref): Call is_power_of_two() instead of explicit checks of > length. Long double checks are dropped, since they are not supported > by s390 and s390x. > (is_double_arg): Return 0 for s390x architecture. > (s390_push_arguments): Substitute hardcoded register size by macro > REGISTER_SIZE. Consider ABI when returning values of type struct. > Substitute hardcoded number of floating point registers by macro > S390_NUM_FP_PARAMETER_REGISTERS. Substitute hardcoded > stack parameter alignment value by macro > S390_STACK_PARAMETER_ALIGNMENT. Substitute hardcoded stack frame > overhead value by macro S390_STACK_FRAME_OVERHEAD. > > --- s390-tdep.c.bak 2003-02-01 19:13:31.000000000 +0000 > +++ s390-tdep.c 2003-02-02 08:40:40.000000000 +0000 > @@ -94,7 +94,9 @@ > #define S390X_SIGREGS_FP0_OFFSET (216) > #define S390_UC_MCONTEXT_OFFSET (256) > #define S390X_UC_MCONTEXT_OFFSET (344) > -#define S390_STACK_FRAME_OVERHEAD (GDB_TARGET_IS_ESAME ? 160:96) > +#define S390_STACK_FRAME_OVERHEAD 16*REGISTER_SIZE+32 > +#define S390_STACK_PARAMETER_ALIGNMENT REGISTER_SIZE > +#define S390_NUM_FP_PARAMETER_REGISTERS (GDB_TARGET_IS_ESAME ? 4:2) > #define S390_SIGNAL_FRAMESIZE (GDB_TARGET_IS_ESAME ? 160:96) > #define s390_NR_sigreturn 119 > #define s390_NR_rt_sigreturn 173 > @@ -1369,13 +1371,18 @@ > > /* This is almost a direct translation of the ABI's language, except > that we have to exclude 8-byte structs; those are DOUBLE_ARGs. */ > - return ((is_integer_like (type) && length <= 4) > + return ((is_integer_like (type) && length <= REGISTER_SIZE) > || is_pointer_like (type) > - || (is_struct_like (type) && length != 8) > - || (is_float_like (type) && length == 16)); > + || (is_struct_like (type) && !is_double_arg (type))); > } > > > +static int > +is_power_of_two (unsigned int n) > +{ > + return ((n & (n - 1)) == 0); > +} > + > /* Return non-zero if TYPE should be passed as a pointer to a copy, > zero otherwise. TYPE must be a SIMPLE_ARG, as recognized by > `is_simple_arg'. */ > @@ -1384,8 +1391,8 @@ > { > unsigned length = TYPE_LENGTH (type); > > - return ((is_struct_like (type) && length != 1 && length != 2 && length != > 4) > - || (is_float_like (type) && length == 16)); > + return (is_struct_like (type) > + && !(is_power_of_two (length) && length <= REGISTER_SIZE)); > } > > > @@ -1417,6 +1424,10 @@ > { > unsigned length = TYPE_LENGTH (type); > > + /* The s390x ABI doesn't handle DOUBLE_ARGS specially. */ > + if (GDB_TARGET_IS_ESAME) > + return 0; > + > return ((is_integer_like (type) > || is_struct_like (type)) > && length == 8); > @@ -1542,9 +1553,9 @@ > > sp = round_down (sp, alignment_of (type)); > > - /* SIMPLE_ARG values get extended to 32 bits. Assume every > - argument is. */ > - if (length < 4) length = 4; > + /* SIMPLE_ARG values get extended to REGISTER_SIZE bytes. > + Assume every argument is. */ > + if (length < REGISTER_SIZE) length = REGISTER_SIZE; > sp -= length; > } > } > @@ -1565,13 +1576,17 @@ > int gr = 2; > CORE_ADDR starg = sp; > > + /* A struct is returned using general register 2 */ > + if (struct_return) > + gr++; > + > for (i = 0; i < nargs; i++) > { > struct value *arg = args[i]; > struct type *type = VALUE_TYPE (arg); > > if (is_double_or_float (type) > - && fr <= 2) > + && fr <= S390_NUM_FP_PARAMETER_REGISTERS * 2 - 2) > { > /* When we store a single-precision value in an FP register, > it occupies the leftmost bits. */ > @@ -1598,7 +1613,7 @@ > write_register_gen (S390_GP0_REGNUM + gr, > VALUE_CONTENTS (arg)); > write_register_gen (S390_GP0_REGNUM + gr + 1, > - VALUE_CONTENTS (arg) + 4); > + VALUE_CONTENTS (arg) + REGISTER_SIZE); > gr += 2; > } > else > @@ -1614,9 +1629,9 @@ > > if (is_simple_arg (type)) > { > - /* Simple args are always either extended to 32 bits, > - or pointers. */ > - starg = round_up (starg, 4); > + /* Simple args are always extended to > + REGISTER_SIZE bytes. */ > + starg = round_up (starg, REGISTER_SIZE); > > /* Do we need to pass a pointer to our copy of this > argument? */ > @@ -1624,18 +1639,19 @@ > write_memory_signed_integer (starg, pointer_size, > copy_addr[i]); > else > - /* Simple args are always extended to 32 bits. */ > - write_memory_signed_integer (starg, 4, > + /* Simple args are always extended to > + REGISTER_SIZE bytes. */ > + write_memory_signed_integer (starg, REGISTER_SIZE, > extend_simple_arg (arg)); > - starg += 4; > + starg += REGISTER_SIZE; > } > else > { > /* You'd think we should say: > starg = round_up (starg, alignment_of (type)); > Unfortunately, GCC seems to simply align the stack on > - a four-byte boundary, even when passing doubles. */ > - starg = round_up (starg, 4); > + a four/eight-byte boundary, even when passing doubles. */ > + starg = round_up (starg, S390_STACK_PARAMETER_ALIGNMENT); > write_memory (starg, VALUE_CONTENTS (arg), length); > starg += length; > } > @@ -1646,7 +1662,7 @@ > /* Allocate the standard frame areas: the register save area, the > word reserved for the compiler (which seems kind of meaningless), > and the back chain pointer. */ > - sp -= 96; > + sp -= S390_STACK_FRAME_OVERHEAD; > > /* Write the back chain pointer into the first word of the stack > frame. This will help us get backtraces from within functions