Re: New gdb 31 & 64 bit patches for S/390

[Andrew Cagney <ac131313 at cygnus dot com>]

The attached are comments on tm-s390.h.  As far as I can tell the file 
could be empty.  This is pretty significant since it means that you've 
managed to pure-multi-arch the s390 target.  Congratulations!

To summarise the comments, my main concern is that it appeared to 
contain many s390-nat.c specific declarations.

	Andrew
diff -u -r -N src.orig/gdb/config/s390/tm-s390.h src.new/gdb/config/s390/tm-s390.h
--- src.orig/gdb/config/s390/tm-s390.h	Thu Jan  1 01:00:00 1970
+++ src.new/gdb/config/s390/tm-s390.h	Tue Feb 27 16:47:03 2001
@@ -0,0 +1,390 @@
+/* Macro definitions for GDB on an S390.
+   Copyright (C) 1999-2001 Free Software Foundation, Inc.
+   Contributed by D.J. Barrow (djbarrow@de.ibm.com,barrow_dj@yahoo.com)
+   for IBM Deutschland Entwicklung GmbH, IBM Corporation.
+
+   This file is part of GDB.
+   
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
+   02111-1307, USA.  */
+
+#if !defined(TM_S390_H)
+#define TM_S390_H 1
+#include <asm/types.h>

Per previous comments, this is simply wrong.  The target code needs to
be host independant and cannot drag in host dependant headers.

+#if GDB_MULTI_ARCH
+#define GDB_TARGET_IS_ESAME (TARGET_ARCHITECTURE->mach == bfd_mach_s390_esame)

I would strongly recommend changing this to a function.  It can be
moved to s390-tdep.c.

+#else
....
+#endif /* GDB_MULTI_ARCH */
+
+#if defined(S390_TDEP) && GDB_MULTI_ARCH
+#define S390_REGISTER_SIZE REGISTER_SIZE
+#else
....
+#endif /* defined(S390_TDEP) && GDB_MULTI_ARCH */
+
+
+#define S390_STACK_FRAME_OVERHEAD  (GDB_TARGET_IS_ESAME ? 160:96)

Again, I would strongly recommend changing this to a function.  It can
be moved to s390-tdep.c or s390-tdep.h (if s390-nat uses it).

+#define S390_NUM_GPRS      (16)
+#define S390_GPR_SIZE      S390_REGISTER_SIZE
+#define S390_PSW_MASK_SIZE S390_REGISTER_SIZE
+#define S390_PSW_ADDR_SIZE S390_REGISTER_SIZE
+#define S390_NUM_FPRS      (16)
+#define S390_FPR_SIZE      (8)
+#define S390_FPC_SIZE      (4)
+#define S390_FPC_PAD_SIZE  (4)	/* gcc insists on aligning the fpregs */
+#define S390_NUM_CRS       (16)
+#define S390_CR_SIZE       S390_REGISTER_SIZE
+#define S390_NUM_ACRS      (16)
+#define S390_ACR_SIZE      (4)
+
+#define S390_ACR0_OFFSET ((S390_PSW_MASK_SIZE+S390_PSW_ADDR_SIZE)+(S390_GPR_SIZE*S390_NUM_GPRS))
+#define S390_CR0_OFFSET (S390_ACR0_OFFSET+(S390_ACR_SIZE+S390_NUM_ACRS))
+#define S390_FPC_OFFSET (S390_CR0_OFFSET+(S390_CR_SIZE*S390_NUM_CRS))
+#define S390_FP0_OFFSET (S390_FPC_OFFSET+(S390_FPC_SIZE+S390_FPC_PAD_SIZE))
+#define S390_GPR6_STACK_OFFSET (GDB_TARGET_IS_ESAME ? 48:24)

I suspect these can be moved to s390-tdep.c.

+
+typedef struct
+{
+  __u32 mask;
+  __u32 addr;
+} s390_psw_t __attribute__ ((aligned (8)));

This declaration doesn't belong in tm-s390.h.  The code
(s390-linux-nat.c?) that refers to it can possibly be greatly
simplified.

+typedef struct
+{
+  __u64 mask;
+  __u64 addr;
+} s390x_psw_t __attribute__ ((aligned (8)));

Again this doesn't belong here.

+typedef union
+{
+  struct
+  {
+    __u32 hi;
+    __u32 lo;
+  }
+  fp;
+  __u32 f;
+} s390_freg_t;

...

+typedef struct
+{
+  /* The compiler appears to like aligning freg_t on an 8 byte boundary
+     so I always access fpregs, this was causing fun when I was doing
+     coersions. */
+  __u32 fpc;
+  s390_freg_t fprs[S390_NUM_FPRS];
+} __s390_fp_regs;
+
+/* gdb structures & the kernel have this much always in common */
+#define __S390_REGS_COMMON     \
+s390_psw_t psw;                \
+__u32 gprs[S390_NUM_GPRS];     \
+__u32  acrs[S390_NUM_ACRS];
+
+#define __S390X_REGS_COMMON    \
+s390x_psw_t psw;               \
+__u64 gprs[S390_NUM_GPRS];     \
+__u32  acrs[S390_NUM_ACRS];
+
+
+typedef struct
+{
+__S390_REGS_COMMON
+} __s390_regs_common;
+
+typedef struct
+{
+__S390X_REGS_COMMON
+} __s390x_regs_common;

....

+/* Sequence of bytes for breakpoint illegal instruction.  */
+#define S390_BREAKPOINT {0x0,0x1}

The code should be using breakpoint_from_pc so that this macro is
unnecessary.

+#define S390_BREAKPOINT_U16 ((__u16)0x0001)
+#define S390_SYSCALL_OPCODE ((__u16)0x0a00)
+#define S390_SYSCALL_SIZE   2
+
+#define S390_MAX_INSTR_SIZE 6
+#define S390_NUM_REGS      (2+S390_NUM_GPRS+S390_NUM_ACRS+S390_NUM_CRS+1+S390_NUM_FPRS)
+#define S390_FIRST_ACR     (2+S390_NUM_GPRS)
+#define S390_LAST_ACR      (S390_FIRST_ACR+S390_NUM_ACRS-1)
+#define S390_FIRST_CR      (S390_FIRST_ACR+S390_NUM_ACRS)
+#define S390_LAST_CR       (S390_FIRST_CR+S390_NUM_CRS-1)
+
+#define S390_PSWM_REGNUM    0
+#define S390_PC_REGNUM      1
+#define	S390_GP0_REGNUM     2	/* GPR register 0 */
+#define S390_GP_LAST_REGNUM (S390_GP0_REGNUM+S390_NUM_GPRS-1)
+/* Usually return address */
+#define S390_RETADDR_REGNUM (S390_GP0_REGNUM+14)
+/* Contains address of top of stack */
+#define S390_SP_REGNUM      (S390_GP0_REGNUM+15)
+/* needed in findvar.c still */	
+#define S390_FP_REGNUM     S390_SP_REGNUM	
+#define S390_FRAME_REGNUM  (S390_GP0_REGNUM+11)
+#define S390_FPC_REGNUM    (S390_GP0_REGNUM+S390_NUM_GPRS+S390_NUM_ACRS+S390_NUM_CRS)
+/* FPR (Floating point) register 0 */
+#define S390_FP0_REGNUM    (S390_FPC_REGNUM+1)
+/* Last floating point register */	
+#define S390_FPLAST_REGNUM (S390_FP0_REGNUM+S390_NUM_FPRS-1)	
+#define S390_LAST_REGNUM   S390_FPLAST_REGNUM
+
+/* The top of this structure is as similar as possible to a 
+   pt_regs structure to simplify code */
+typedef struct
+{
+  __S390_REGS_COMMON 
+  __u32 crs[S390_NUM_CRS];
+  __s390_fp_regs fp_regs;
+} s390_gdb_regs __attribute__ ((packed));

...

+
+typedef struct
+{
+  __S390X_REGS_COMMON 
+  __u64 crs[S390_NUM_CRS];
+  __s390_fp_regs fp_regs;
+} s390x_gdb_regs __attribute__ ((packed));

...

+
+
+#define S390_REGISTER_NAMES                                      \
+{                                                                \
+"pswm","pswa",                                                   \
+"gpr0","gpr1","gpr2","gpr3","gpr4","gpr5","gpr6","gpr7",         \
+"gpr8","gpr9","gpr10","gpr11","gpr12","gpr13","gpr14","gpr15",   \
+"acr0","acr1","acr2","acr3","acr4","acr5","acr6","acr7",         \
+"acr8","acr9","acr10","acr11","acr12","acr13","acr14","acr15",   \
+"cr0","cr1","cr2","cr3","cr4","cr5","cr6","cr7",                 \
+"cr8","cr9","cr10","cr11","cr12","cr13","cr14","cr15",           \
+"fpc",                                                           \
+"fpr0","fpr1","fpr2","fpr3","fpr4","fpr5","fpr6","fpr7",         \
+"fpr8","fpr9","fpr10","fpr11","fpr12","fpr13","fpr14","fpr15"    \
+}

The contents of this macro can be moved to s390-tdep.c.

+
+
+/* instruction sequence for call dummy is as follows
+   bras %r1,.+8      ; 0xA7150004   
+   long basraddr     ; 0x00000000
+   l    %r1,0(%r1)   ; 0x58101000
+   basr %r14,%r1     ; 0x0DE1
+   breakpoint        ; 0x0001 */
+#define S390_CALL_DUMMY {0xA7150004,0x00000000,0x58101000,0x0DE10000|S390_BREAKPOINT_U16}
+#define S390_CALL_DUMMY_LENGTH 16
+#define S390_CALL_ADDR_OFFSET  4

The contents of these macros can be moved to s390-tdep.c.

+
+
+/* instruction sequence for call dummy is as follows
+   bras %r1,.+12     ; 0xA7150006   
+   long basraddr     ; 0x0000000000000000
+   lg   %r1,0(%r1)   ; 0xE31010000004
+   basr %r14,%r1     ; 0x0DE1
+   breakpoint        ; 0x0001 */
+
+#define S390X_CALL_DUMMY {0xA715000600000000,0x00000000E3101000, \
+0x00040DE100000000|S390_BREAKPOINT_U16<<16}
+#define S390X_CALL_DUMMY_LENGTH 22

The contents of these macros can be moved to s390-tdep.c.

+#define S390_REGISTER_BYTES    \
+((S390_FP0_OFFSET)+(S390_FPR_SIZE*S390_NUM_FPRS))

This can be moved to s390-tdep.c or s390-tdep.h (if it is needed by
s390-nat.c).

+
+
+#if !GDB_MULTI_ARCH
...
+#endif /* !GDB_MULTI_ARCH */
+#endif /* ifndef TM_S390_H */
+
+
+