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

Denis Joseph Barrow
2001-07-05T09:15:00Z

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 51066 bytes --]

Hi Andrew,
+
+#undef offsetof
+#define offsetof(type, member) ((CORE_ADDR) & ((type*)0) -> member )
+
+#define s390_offsetof(esame_type,esa_type,member) \
+(GDB_TARGET_IS_ESAME ? offsetof(esame_type,member) :
offsetof(esa_type,member))
+
+#define s390_sizeof(esame_type,esa_type) \
+(GDB_TARGET_IS_ESAME ? sizeof(esame_type) : sizeof(esa_type))
+
+#define SIZEOF_S390_GDB_REGS s390_sizeof(s390x_gdb_regs,s390_gdb_regs)

I don't know where you get the idea these are unportable,
they use the definitons u32 u16 etc from the host asm/types.h & if
asm/types is defined
this stuff is portable.

GDB_TARGET_IS_ESAME macro is fine & if not please explain why you think it
isn't.

D.J. Barrow Gnu/Linux for S/390 kernel developer
eMail: djbarrow@de.ibm.com,barrow_dj@yahoo.com
Phone: +49-(0)7031-16-2583
IBM Germany Lab, SchÃ¶naicherstr. 220, 71032 BÃ¶blingen

Andrew Cagney <ac131313@cygnus.com> on 05.07.2001 05:07:52

Please respond to Andrew Cagney <ac131313@cygnus.com>

To: Denis Joseph Barrow/Germany/Contr/IBM@IBMDE
cc: gdb-patches@sourceware.cygnus.com, s390-patches@gnu.org, Martin
Schwidefsky/Germany/IBM@IBMDE, Christoph Arenz/Germany/IBM@IBMDE
Subject: Re: New gdb 31 & 64 bit patches for S/390

DJB,

I'm trying to build an s390 cross debugger. Actually I'm trying to build:

./configure --target=s390-elf

s390-elf being a a fairly generic s390 target.

The reason for doing this is to ensure that the non linix specific parts
of the s390 target really are native independant. If you check the
MAINTAINERS file you'll notice that all targets (with the exception of
the HP/PA, which has no maintainer) can be built as a cross debugger.

Any way, I'm not having much luck.

The configury changes needed for this are very straight forward.
Unfortunatly the tweeks to s390-tdep.c are not. Sigh. For the record,
the problems are not new.

As I suggested last time, I can think of two possible courses of action:

o we try to fix the s390 problems
and then get an IBM copyright transfer

o IBM transfer copyright for these files
to the FSF and then they be included in
GDB as a patch. This would free up the
code so that others can address the problems.

A very detailed commentry on the file s390-tdep.c is attached.

Andrew

--- /dev/null Tue Jun 1 11:25:19 1999
+++ src.new/gdb/s390-tdep.c Tue Feb 27 15:10:47 2001
@@ -0,0 +1,1308 @@
+/* Target-dependent code for GDB, the GNU debugger.
+ Copyright 1999 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. */
+
+#define S390_TDEP /* for special macros in tm-s390.h */

This is wrong. tm-s390.h shouldn't be affected by s390-tdep.c.

+#include <defs.h>
+#include "arch-utils.h"
+#include "frame.h"
+#include "inferior.h"
+#include "symtab.h"
+#include "target.h"
+#include "gdbcore.h"
+#include "gdbcmd.h"
+#include "symfile.h"
+#include "objfiles.h"
+#include "tm.h"

Recent changes to GDB may mean that "tm.h" is no longer be needed for
native targets. For the moment I'll assume it is.

Thinking about this, I suspect what you really need is ``s390-tdep.h''
(included by s390-tdep.c and s390-nat.c). That could contain all the
constants that need to be shared between the s390-* files with out
needing to define them globally.

+#include "../bfd/bfd.h"

That should be #include "bfd.h"

+#include "floatformat.h"
+
+#ifndef TRUE
+#define TRUE 1
+#endif
+#ifndef FALSE
+#define FALSE 0
+#endif

See earlier post. Your code contains constructs such as:

++ if (good_prologue == TRUE)
++ for (second_pass = FALSE; second_pass <= TRUE; second_pass++)
++ if (init_extra_info || fextra_info->initialised == FALSE)
++ if (frame_pointer_found == FALSE && regidx == 0xb)
++ && fextra_info.good_prologue == FALSE);

such expressions should be written as:

if (good_prologue)
if (!fextra_info.good_prologue)

With that in mind (and per previous e-mail), i'd fix the conditionals
and replace ``TRUE'' with 1, ``FALSE'' with 0.

+
+#undef offsetof
+#define offsetof(type, member) ((CORE_ADDR) & ((type*)0) -> member )
+
+#define s390_offsetof(esame_type,esa_type,member) \
+(GDB_TARGET_IS_ESAME ? offsetof(esame_type,member) :
offsetof(esa_type,member))
+
+#define s390_sizeof(esame_type,esa_type) \
+(GDB_TARGET_IS_ESAME ? sizeof(esame_type) : sizeof(esa_type))
+
+#define SIZEOF_S390_GDB_REGS s390_sizeof(s390x_gdb_regs,s390_gdb_regs)

All of these macro's are very unportable. They assume HOST=TARGET.
The gdbint document goes into some detail on how important it is to
not write code that assumes this.

In the past it has been suggested that types like _u32, _u8 etc would
allow the user to exactly describe structures and hence allow the
programmer to exactly describe structure layout. Unfortunatly, that
won't work since allignment (and hence packing) also varies between
hosts.

If you need to read structures from memory you can use functions like:

LONGEST read_memory_integer (CORE_ADDR memaddr, int len);
ULONGEST read_memory_unsigned_integer (CORE_ADDR memaddr, int len);

If you already have the memory local then:

LONGEST extract_signed_integer (void *, int);
ULONGEST extract_unsigned_integer (void *, int);

will do the trick.

+struct frame_extra_info
+{
+ int initialised;
+ int good_prologue;
+ CORE_ADDR function_start;
+ CORE_ADDR skip_prologue_function_start;
+ CORE_ADDR saved_pc_valid;
+ CORE_ADDR saved_pc;
+ CORE_ADDR sig_fixed_saved_pc_valid;
+ CORE_ADDR sig_fixed_saved_pc;
+ __u32 stack_bought; /* amount we decrement the stack pointer by
*/

See comment above. This is either going to be small (int) or large
(LONGEST).

+ int has_frame_pointer; /* frame pointer needed for alloca */
+};
+
+
+static CORE_ADDR s390_frame_saved_pc_nofix (struct frame_info *fi);
+
+int
+s390_readinstruction (__u8 instr[], CORE_ADDR at,
+ struct disassemble_info *info)

The function name should be s390_read_instruction (see GNU coding
standard).

The first parameter should be just ``char *insr'' or ``char insr[]''.
Don't use __u8. Yes, strictly speaking this means that GDB assumes
tha sizeof(char) == one eight bit byte. However, per the comment in
findvar.c:

#if TARGET_CHAR_BIT != 8 || HOST_CHAR_BIT != 8
/* 8 bit characters are a pretty safe assumption these days, so we
assume it throughout all these swapping routines. If we had to deal
with
9 bit characters, we would need to make len be in bits and would have
to re-write these routines... */
you lose
#endif

The changes below will make the third parameter redundant.

+{
+ int instrlen;
+
+ static int s390_instrlen[] = {
+ 2,
+ 4,
+ 4,
+ 6
+ };
+ if ((*info->read_memory_func) (at, &instr[0], 2, info))

Just use the function:

target_read_memory (at, insr, 2);

see corefile.c:read_memory() for an example of how to check the return
value.

+ return -1;
+ instrlen = s390_instrlen[instr[0] >> 6];
+ if ((*info->read_memory_func) (at + 2, &instr[2], instrlen - 2, info))
+ return -1;

Again, just use:

target_read_memory (....)

+ return instrlen;
+}
+
+static void
+s390_memset_extra_info (struct frame_extra_info *fextra_info)
+{
+ memset (fextra_info, 0, sizeof (struct frame_extra_info));
+}
+
+
+
+char *
+s390_register_name (int reg_nr)
+{
+ static char *register_names[] = S390_REGISTER_NAMES;

The macro S390_REGISTER_NAMES is not needed. Instead just move the
macro contents to here.

+
+ if (reg_nr >= S390_LAST_REGNUM)
+ return NULL;
+ return register_names[reg_nr];
+}
+
+
+
+int
+s390_cannot_fetch_register (int regno)
+{
+ return (regno >= S390_FIRST_CR && regno < (S390_FIRST_CR + 9)) ||
+ (regno >= (S390_FIRST_CR + 12) && regno <= S390_LAST_CR);

See the GNU coding standard. This expression should be written with
``||'' at the start of a line and indented / parenthesized something
like:

return ((regno >= S390_FIRST_CR && regno < (S390_FIRST_CR + 9))
|| ((regno >= (S390_FIRST_CR + 12) && regno <=
S390_LAST_CR)));

+}
+
+int
+s390_stab_reg_to_regnum (int regno)
+{
+ return regno >= 64 ? S390_PSWM_REGNUM - 64 :
+ regno >= 48 ? S390_FIRST_ACR - 48 :
+ regno >= 32 ? S390_FIRST_CR - 32 :
+ regno <= 15 ? (regno + 2) :
+ S390_FP0_REGNUM + ((regno - 16) & 8) + (((regno - 16) & 3) << 1) +
+ (((regno - 16) & 4) >> 2);

Please rewrite this as:

if (regno >= 64)
return S390_PSWM_REGNUM - 64;
else if (regno >= 48)
return S390_FIRST_ACR - 48;
....

+}
+
+
+
+/* s390_get_frame_info based on Hartmuts
+ prologue definition in
+ gcc-2.8.1/config/l390/linux.c
+
+ It reads one instruction at a time & based on whether
+ it looks like prologue code or not it makes a decision on
+ whether the prologue is over, there are various state machines
+ in the code to determine if the prologue code is possilby valid.
+
+ This is done to hopefully allow the code survive minor revs of
+ calling conventions.
+
+ fextra_info must be already set up to come in with saved_regs not NULL
*/
+
+int
+s390_get_frame_info (CORE_ADDR pc, struct frame_extra_info *fextra_info,
+ struct frame_info *fi, int init_extra_info)
+{
+#define CONST_POOL_REGIDX 13
+#define GOT_REGIDX 12

These should be enums.

+ __u8 instr[S390_MAX_INSTR_SIZE];

Per above. char is sufficient.

+ CORE_ADDR test_pc = pc, test_pc2;

While I have a personal preference for one variable declaraton per
line, one assignment per line, the GNU coding standard does tolerate
this.

+ CORE_ADDR orig_sp = 0, save_reg_addr = 0, *saved_regs = NULL;
+ int valid_prologue, good_prologue = FALSE;
+ int gprs_saved[S390_NUM_GPRS];
+ int fprs_saved[S390_NUM_FPRS];
+ int regidx, instrlen;
+ int save_link_regidx, subtract_sp_regidx;
+ int const_pool_state, save_link_state, got_state;

These state variables need to be implemented as enums and explicit
states. At the end of the code the expression:

+ good_prologue = (((got_state == 0) || (got_state == 2)) &&
+ ((const_pool_state == 0) || (const_pool_state == 2)) &&
+ ((save_link_state == 0) || (save_link_state == 4)) &&
+ ((varargs_state == 0) || (varargs_state == 2)));

put simply, I have no idea what this does (yet I should).

+ int frame_pointer_found, varargs_state;
+ int loop_cnt, gdb_gpr_store, gdb_fpr_store;
+ int frame_pointer_regidx = 0xf;
+ int offset, expected_offset;
+ int err = 0;
+ disassemble_info info;
+ const_pool_state = save_link_state = got_state = varargs_state = 0;
+ frame_pointer_found = FALSE;
+ memset (gprs_saved, 0, sizeof (gprs_saved));
+ memset (fprs_saved, 0, sizeof (fprs_saved));
+ info.read_memory_func = dis_asm_read_memory;

Per comments above, ``info'' isn't needed.

+
+ save_link_regidx = subtract_sp_regidx = 0;
+ if (fi)
+ {
+ saved_regs = fi->saved_regs;
+ orig_sp = fi->frame + fextra_info->stack_bought;
+ }
+ if (fextra_info)
+ {
+ if (init_extra_info || fextra_info->initialised == FALSE)
+ {

Take care with the coding standard - indent two spaces vis:

if (...)
{
s390_....

As I suggested earlier, the easiest way to fix this is to put the code
through GNU indent.

+ s390_memset_extra_info (fextra_info);
+ fextra_info->function_start = pc;
+ fextra_info->initialised = TRUE;
+ }
+ }
+ instrlen = 0;
+ do
+ {
+ valid_prologue = FALSE;
+ test_pc += instrlen;
+ /* add the previous instruction len */
+ instrlen = s390_readinstruction (instr, test_pc, &info);
+ if (instrlen < 0)
+ {
+ good_prologue = FALSE;
+ err = -1;
+ break;
+ }
+ if (save_link_state == 0)
+ {
+ /* check for a stack relative STMG or STM */
+ if (((GDB_TARGET_IS_ESAME &&

This doesn't need to be a macro. A function like:

target_is_s390_esame()

or even the test:

if (TARGET_ARCHITECTURE->mach == bfd_mach_s390_esame)

are sufficient.

+ ((instr[0] == 0xeb) && (instr[5] == 0x24))) ||
+ (instr[0] == 0x90)) && ((instr[2] >> 4) == 0xf))

Again see above about how to write expressions.

+ {
+ regidx = (instr[1] >> 4);
+ if (regidx < 6)
+ varargs_state = 1;
+ offset = ((*((__u16 *) & instr[2])) & 0xfff);

This isn't portable.

+ expected_offset =
+ S390_GPR6_STACK_OFFSET + (S390_GPR_SIZE * (regidx - 6));
+ if (offset != expected_offset)
+ {
+ good_prologue = FALSE;
+ break;
+ }
+ if (saved_regs)
+ save_reg_addr = orig_sp + offset;
+ for (; regidx <= (instr[1] & 0xf); regidx++)
+ {
+ if (gprs_saved[regidx])
+ {
+ good_prologue = FALSE;
+ break;
+ }
+ good_prologue = TRUE;
+ gprs_saved[regidx] = TRUE;
+ if (saved_regs)
+ {
+ saved_regs[S390_GP0_REGNUM + regidx] = save_reg_addr;
+ save_reg_addr += S390_GPR_SIZE;
+ }
+ }
+ valid_prologue = TRUE;
+ continue;
+ }
+ }
+ /* check for a stack relative STG or ST */
+ if ((save_link_state == 0 || save_link_state == 3) &&
+ ((GDB_TARGET_IS_ESAME &&
+ ((instr[0] == 0xe3) && (instr[5] == 0x24))) ||
+ (instr[0] == 0x50)) && ((instr[2] >> 4) == 0xf))

Again expressions style.

+ {
+ regidx = instr[1] >> 4;
+ offset = ((*((__u16 *) & instr[2])) & 0xfff);
+ if (offset == 0)
+ {
+ if (save_link_state == 3 && regidx == save_link_regidx)
+ {
+ save_link_state = 4;
+ valid_prologue = TRUE;
+ continue;
+ }
+ else
+ break;
+ }
+ if (regidx < 6)
+ varargs_state = 1;
+ expected_offset =
+ S390_GPR6_STACK_OFFSET + (S390_GPR_SIZE * (regidx - 6));
+ if (offset != expected_offset)
+ {
+ good_prologue = FALSE;
+ break;
+ }
+ if (gprs_saved[regidx])
+ {
+ good_prologue = FALSE;
+ break;
+ }
+ good_prologue = TRUE;
+ gprs_saved[regidx] = TRUE;
+ if (saved_regs)
+ {
+ save_reg_addr = orig_sp + offset;
+ saved_regs[S390_GP0_REGNUM + regidx] = save_reg_addr;
+ }
+ valid_prologue = TRUE;
+ continue;
+ }
+
+ /* check for STD */
+ if (instr[0] == 0x60 && (instr[2] >> 4) == 0xf)
+ {
+ regidx = instr[1] >> 4;
+ if (regidx == 0 || regidx == 2)
+ varargs_state = 1;
+ if (fprs_saved[regidx])
+ {
+ good_prologue = FALSE;
+ break;
+ }
+ fprs_saved[regidx] = TRUE;
+ if (saved_regs)
+ {
+ save_reg_addr = orig_sp + ((*((__u16 *) & instr[2])) & 0xfff);

Again, portability, probably use:

extract_unsigned_integer (...);

+ saved_regs[S390_FP0_REGNUM + regidx] = save_reg_addr;
+ }
+ valid_prologue = TRUE;
+ continue;
+ }
+
+
+ if (const_pool_state == 0)
+ {
+ /* Check for BASR gpr13,gpr0 used to load constant pool pointer to
r13 in old compiler */
+ if (GDB_TARGET_IS_ESAME && instr[0] == 0xd && (instr[1] & 0xf) == 0
+ && ((instr[1] >> 4) == CONST_POOL_REGIDX))
+ {
+ const_pool_state = 1;
+ valid_prologue = TRUE;
+ continue;
+ }
+ /* Check for new fangled bras %r13,newpc to load new constant pool
*/
+ /* embedded in code */
+ if ((instr[0] == 0xa7) && ((instr[1] & 0xf) == 0x5) &&
+ ((instr[1] >> 4) == CONST_POOL_REGIDX)
+ && ((instr[2] & 0x80) == 0))

Style.

+ {
+ const_pool_state = 2;
+ test_pc += ((*((__u16 *) & instr[2]) << 1) - instrlen);

Remember, portability.

+ valid_prologue = TRUE;
+ continue;
+ }
+ }
+ /* Check for AGHI or AHI CONST_POOL_REGIDX,val */
+ if (const_pool_state == 1 && (instr[0] == 0xa7) &&
+ ((GDB_TARGET_IS_ESAME &&
+ (instr[1] == ((CONST_POOL_REGIDX << 4) | 0xb))) ||
+ (instr[1] == ((CONST_POOL_REGIDX << 4) | 0xa))))

Style.

+ {
+ const_pool_state = 2;
+ valid_prologue = TRUE;
+ continue;
+ }
+ /* Check for LGR or LR gprx,15 */
+ if ((GDB_TARGET_IS_ESAME &&
+ instr[0] == 0xb9 && instr[1] == 0x04 && (instr[3] & 0xf) == 0xf) ||
+ (instr[0] == 0x18 && (instr[1] & 0xf) == 0xf))

Style.

+ {
+ if (GDB_TARGET_IS_ESAME)
+ regidx = instr[3] >> 4;
+ else
+ regidx = instr[1] >> 4;
+ if (save_link_state == 0 && regidx != 0xb)
+ {
+ /* Almost defintely code for
+ decrementing the stack pointer
+ ( i.e. a non leaf function
+ or else leaf with locals ) */
+ save_link_regidx = regidx;
+ save_link_state = 1;
+ valid_prologue = TRUE;
+ continue;
+ }
+ /* We use this frame pointer for alloca
+ unfortunately we need to assume its gpr11
+ otherwise we would need a smarter prologue
+ walker. */
+ if (frame_pointer_found == FALSE && regidx == 0xb)
+ {
+ frame_pointer_regidx = 0xb;
+ frame_pointer_found = TRUE;
+ if (fextra_info)
+ fextra_info->has_frame_pointer = TRUE;
+ valid_prologue = TRUE;
+ continue;
+ }
+ }
+ /* Check for AHI or AGHI gpr15,val */
+ if (save_link_state == 1 && (instr[0] == 0xa7) &&
+ ((GDB_TARGET_IS_ESAME && (instr[1] == 0xfb)) || (instr[1] == 0xfa)))
+ {
+ if (fextra_info)
+ fextra_info->stack_bought = -(*((__s16 *) & instr[2]));
+ save_link_state = 3;
+ valid_prologue = TRUE;
+ continue;
+ }
+ /* Alternatively check for the complex construction for
+ buying more than 32k of stack
+ BRAS gprx,.+8
+ long vals %r15,0(%gprx) gprx currently r1 */
+ if ((save_link_state == 1) && (instr[0] == 0xa7)
+ && ((instr[1] & 0xf) == 0x5) && (*((__u16 *) & instr[2]) == 0x4)
+ && ((instr[1] >> 4) != CONST_POOL_REGIDX))

Style.

+ {
+ subtract_sp_regidx = instr[1] >> 4;
+ save_link_state = 2;
+ if (fextra_info)
+ target_read_memory (test_pc + instrlen,
+ (char *) &fextra_info->stack_bought,

The cast isn't needed.

+ sizeof (fextra_info->stack_bought));
+ test_pc += 4;
+ valid_prologue = TRUE;
+ continue;
+ }
+ if (save_link_state == 2 && (*((__u16 *) & instr[0]) == 0x5bf0) &&
+ (*((__u16 *) & instr[2]) == subtract_sp_regidx << 12))

Style

+ {
+ save_link_state = 3;
+ valid_prologue = TRUE;
+ continue;
+ }
+ /* check for LA gprx,offset(15) used for varargs */
+ if (varargs_state == 1 &&
+ instr[0] == 0x41 && ((instr[1] & 0xf) == 0)
+ && ((instr[2] >> 4) == 0xf))
+ {
+ varargs_state = 2;
+ valid_prologue = TRUE;
+ continue;
+ }
+ /*
+ Check for a GOT load
+ we might be able to get info like whether we
+ are compiled -fpic to check whether this is valid
+ prologue */

Comments should be formatted vis:

/* Check for a GOT load. We might be able to get info like whether
we
are compiled -fpic to check whether this is valid prologue. */

and don't forget complete sentences and ``. '' like I do :-)

+ if (got_state == 0 && const_pool_state == 2 && instr[0] == 0x58
+ && (instr[2] == (CONST_POOL_REGIDX << 4))
+ && ((instr[1] >> 4) == GOT_REGIDX))

style

+ {
+ got_state == 1;
+ valid_prologue = TRUE;
+ continue;
+ }
+ /* Check for subsequent ar got_regidx,basr_regidx */
+ if (got_state == 1 && instr[0] == 0x1a &&
+ instr[1] == ((GOT_REGIDX << 4) | CONST_POOL_REGIDX))
+ {
+ got_state = 2;
+ valid_prologue = TRUE;
+ continue;
+ }
+ }
+ while (valid_prologue && good_prologue);
+ if (good_prologue == TRUE)
+ {
+ good_prologue = (((got_state == 0) || (got_state == 2)) &&
+ ((const_pool_state == 0) || (const_pool_state == 2)) &&
+ ((save_link_state == 0) || (save_link_state == 4)) &&
+ ((varargs_state == 0) || (varargs_state == 2)));
+ }
+ if (fextra_info)
+ {
+ fextra_info->good_prologue = good_prologue;
+ fextra_info->skip_prologue_function_start =
+ (good_prologue ? test_pc : pc);
+ }
+ return err;
+}
+
+
+static CORE_ADDR
+s390_sniff_pc_function_start (CORE_ADDR pc)
+{
+ CORE_ADDR function_start, test_function_start, ret_function_start,
loop_dec;
+ int loop_cnt, err;
+ struct frame_extra_info fextra_info;
+ function_start = get_pc_function_start (pc);
+
+ if (function_start == 0)
+ {
+ test_function_start = pc;
+ if (test_function_start & 1)
+ loop_dec = 1; /* This is a suspicious address */
+ else
+ loop_dec = 2;

Indentation. Other cases occure below.

+ loop_cnt = 0;
+ do
+ {
+ err =
+ s390_get_frame_info (test_function_start, &fextra_info, NULL,
+ TRUE);
+ loop_cnt++;
+ test_function_start -= loop_dec;
+ }
+ while (err == 0 && loop_cnt < 4096
+ && fextra_info.good_prologue == FALSE);

This is better.

+ if (fextra_info.good_prologue)
+ function_start = fextra_info.function_start;
+ }
+ return function_start;
+}
+
+
+
+CORE_ADDR s390_function_start (struct frame_info * fi)

The function name should appear at the start of the line vis:

CORE_ADDR
s390_function_start (...)

I suspect it can also be made static like most of the functions in
this file. Other cases of this occure below.

+{
+ CORE_ADDR function_start = 0;
+
+ if (fi->extra_info && fi->extra_info->initialised)
+ function_start = fi->extra_info->function_start;
+ else if (fi->pc)
+ function_start = get_pc_function_start (fi->pc);
+ return function_start;
+}
+
+
+
+
+int
+s390_frameless_function_invocation (struct frame_info *fi)
+{
+ struct frame_extra_info fextra_info, *fextra_info_ptr;
+ int frameless = 0;
+
+ if (fi->next == NULL) /* no may be frameless */
+ {
+ if (fi->extra_info)
+ fextra_info_ptr = fi->extra_info;
+ else
+ {

Indentation.

+ fextra_info_ptr = &fextra_info;
+ s390_get_frame_info (s390_sniff_pc_function_start (fi->pc),
+ fextra_info_ptr, NULL, TRUE);
+ }
+ frameless = ((fextra_info_ptr->good_prologue)
+ && (fextra_info_ptr->stack_bought == 0));
+ }
+ return frameless;
+
+}
+
+
+static int
+s390_is_sigreturn (CORE_ADDR pc, struct frame_info *sighandler_fi,
+ CORE_ADDR * sregs, CORE_ADDR * sigcaller_pc)

Style: ``CORE_ADDR *sregs'' not ``CORE_ADDR * sregs''.

+{
+ __u8 instr[S390_MAX_INSTR_SIZE];

char

+ disassemble_info info;
+ int instrlen;
+ CORE_ADDR scontext;
+ int retval = FALSE;
+ CORE_ADDR orig_sp;
+ CORE_ADDR temp_sregs;
+
+ scontext = temp_sregs = 0;
+
+ info.read_memory_func = dis_asm_read_memory;
+ instrlen = s390_readinstruction (instr, pc, &info);
+ if (sigcaller_pc)
+ *sigcaller_pc = 0;
+ if ((instrlen == S390_SYSCALL_SIZE) &&
+ ((((S390_SYSCALL_OPCODE | s390_NR_sigreturn)) == *(__u16 *) instr)
||
+ (((S390_SYSCALL_OPCODE | s390_NR_rt_sigreturn)) == *(__u16 *)
instr)))
+ {
+ if (sighandler_fi)
+ {
+ if (s390_frameless_function_invocation (sighandler_fi))
+ orig_sp = sighandler_fi->frame;
+ else
+ orig_sp = ADDR_BITS_REMOVE ((CORE_ADDR)
+ read_memory_integer (sighandler_fi->frame,
+ S390_GPR_SIZE));
+ if (orig_sp && sigcaller_pc)
+ {
+ scontext = orig_sp + S390_SIGNAL_FRAMESIZE;
+ temp_sregs = ADDR_BITS_REMOVE((CORE_ADDR)
+ read_memory_integer (scontext +
+
s390_offsetof(s390x_sigcontext,s390_sigcontext,sregs),
+ S390_GPR_SIZE));
+ *sigcaller_pc =
+ ADDR_BITS_REMOVE ((CORE_ADDR)
+ read_memory_integer (temp_sregs +
+
s390_offsetof(s390x_sigregs,s390_sigregs,regs.psw.addr),
+ S390_PSW_ADDR_SIZE));
+ }
+ }
+ retval = TRUE;
+ }
+ if (sregs)
+ *sregs = temp_sregs;
+ return retval;
+}
+
+/*
+ We need to do something better here but this will keep us out of trouble
+ for the moment.
+ For some reason the blockframe.c calls us with fi->next->fromleaf
+ so this seems of little use to us. */
+void
+s390_init_frame_pc_first (int next_fromleaf, struct frame_info *fi)
+{
+ CORE_ADDR sigcaller_pc;
+
+ fi->pc = 0;
+ if (next_fromleaf)
+ {
+ fi->pc = ADDR_BITS_REMOVE (read_register (S390_RETADDR_REGNUM));
+ /* fix signal handlers */
+ }
+ else if (fi->next && fi->next->pc)
+ fi->pc = s390_frame_saved_pc_nofix (fi->next);
+ if (fi->pc && fi->next && fi->next->frame &&
+ s390_is_sigreturn (fi->pc, fi->next, NULL, &sigcaller_pc))

Style.

+ {
+ fi->pc = sigcaller_pc;
+ }
+
+}
+
+void
+s390_init_extra_frame_info (int fromleaf, struct frame_info *fi)
+{
+ fi->extra_info = frame_obstack_alloc (sizeof (struct frame_extra_info));
+ if (fi->pc)
+ s390_get_frame_info (s390_sniff_pc_function_start (fi->pc),
+ fi->extra_info, NULL, TRUE);
+ else
+ s390_memset_extra_info (fi->extra_info);
+}
+
+/* If saved registers of frame FI are not known yet, read and cache them.
+ &FEXTRA_INFOP contains struct frame_extra_info; TDATAP can be NULL,
+ in which case the framedata are read. */
+
+void
+s390_frame_init_saved_regs (struct frame_info *fi)
+{
+
+ if (fi->saved_regs == NULL)
+ {
+
+ frame_saved_regs_zalloc (fi);
+ if (fi->extra_info)
+ {
+ if (!fi->extra_info->initialised && fi->pc)
+ s390_get_frame_info (s390_sniff_pc_function_start (fi->pc),
+ fi->extra_info, fi, TRUE);
+ if (fi->extra_info->good_prologue)
+ s390_get_frame_info (fi->extra_info->function_start,
+ fi->extra_info, fi, FALSE);
+ }
+ }
+}
+
+
+
+CORE_ADDR s390_frame_args_address (struct frame_info *fi)
+{
+
+ /* Apparently gdb already knows gdb_args_offset itself */
+ return fi->frame;
+}
+
+
+static CORE_ADDR
+s390_frame_saved_pc_nofix (struct frame_info *fi)
+{
+ if (fi->extra_info && fi->extra_info->saved_pc_valid)
+ return fi->extra_info->saved_pc;
+ s390_frame_init_saved_regs (fi);
+ if (fi->extra_info)
+ {
+ fi->extra_info->saved_pc_valid = TRUE;
+ if (fi->extra_info->good_prologue)
+ {
+ return (fi->extra_info->saved_pc =
+
ADDR_BITS_REMOVE(read_memory_integer(fi->saved_regs[S390_RETADDR_REGNUM],
+ S390_GPR_SIZE)));
+ }
+ }
+ return 0;
+}
+
+CORE_ADDR s390_frame_saved_pc (struct frame_info * fi)
+{
+ CORE_ADDR saved_pc = 0, sig_pc;
+
+ if (fi->extra_info && fi->extra_info->sig_fixed_saved_pc_valid)
+ return fi->extra_info->sig_fixed_saved_pc;
+ saved_pc = s390_frame_saved_pc_nofix (fi);
+
+ if (fi->extra_info)
+ {
+ fi->extra_info->sig_fixed_saved_pc_valid = TRUE;
+ if (saved_pc)
+ {
+ if (s390_is_sigreturn (saved_pc, fi, NULL, &sig_pc))
+ saved_pc = sig_pc;
+ }
+ fi->extra_info->sig_fixed_saved_pc = saved_pc;
+ }
+ return saved_pc;
+}
+
+
+
+
+/* We want backtraces out of signal handlers so we don't
+ set thisframe->signal_handler_caller to 1 */
+
+CORE_ADDR s390_frame_chain (struct frame_info * thisframe)
+{
+
+ CORE_ADDR prev_fp = 0;
+ struct frame_extra_info fextra_info;
+ CORE_ADDR saved_pc, sig_pc;
+ int regno;
+ CORE_ADDR sregs;
+ int sigreturn;
+
+ if (thisframe->prev && thisframe->prev->frame)
+ prev_fp = thisframe->prev->frame;
+ else
+ {
+ /* We have to do some work */
+ if (!thisframe->pc)
+ return 0;
+ saved_pc = s390_frame_saved_pc_nofix (thisframe);
+ if (!saved_pc)
+ return 0;
+ if (
+ (sigreturn =
+ s390_is_sigreturn (saved_pc, thisframe, &sregs, &sig_pc)))
+ saved_pc = sig_pc;
+ s390_get_frame_info (s390_sniff_pc_function_start (saved_pc),
+ &fextra_info, NULL, TRUE);
+ if (!fextra_info.good_prologue)
+ return 0;
+ if (sigreturn)
+ prev_fp =
+ ADDR_BITS_REMOVE ((CORE_ADDR)
+ read_memory_integer (sregs +
+ s390_offsetof (s390x_sigregs,
+ s390_sigregs,
+ regs.
+ gprs
+ [fextra_info.
+ has_frame_pointer
+ ? 11 : 15]),
+ S390_GPR_SIZE));
+ else
+ {
+ regno =
+ (fextra_info.
+ has_frame_pointer ? S390_FRAME_REGNUM : S390_SP_REGNUM);

Conditional expressions are normally indented as:

(fextra_info.has_frame_pointer
? S390_FRAME_REGNUM
: S390_SP_REGNUM)

however typically it is better to just use an IF statement vis:

+ /* if it wasn't saved we must be in the innermost frame */
+ prev_fp = ADDR_BITS_REMOVE (thisframe->saved_regs[regno] ?
+ read_memory_integer (thisframe->
+ saved_regs[regno],
+ S390_GPR_SIZE) :
+ read_register (regno));
+ }
+
+
+ }
+ return prev_fp;
+}
+
+/*
+ Whether struct frame_extra_info is actually needed I'll have to figure
+ out as our frames are similar to rs6000 there is a possibility
+ i386 dosen't need it. */

see above on how to write comments.

+
+
+
+/* a given return value in `regbuf' with a type `valtype', extract and
copy its
+ value into `valbuf' */
+void
+s390_extract_return_value (struct type *valtype, char *regbuf, char
*valbuf)
+{
+ /* floats and doubles are returned in fpr0. fpr's have a size of 8
bytes.
+ We need to truncate the return value into float size (4 byte) if
+ necessary. */
+ int len = TYPE_LENGTH (valtype);
+
+ if (TYPE_CODE (valtype) == TYPE_CODE_FLT)
+ {
+ if (len > (TARGET_FLOAT_BIT>>3))
+ memcpy (valbuf,&regbuf[REGISTER_BYTE (S390_FP0_REGNUM)],len);

Remember, space after commas.

+ else
+ {
+ /* float */
+ DOUBLEST val;
+
+ floatformat_to_doublest (&floatformat_ieee_double_big,
+ &regbuf[REGISTER_BYTE (S390_FP0_REGNUM)], &val);
+ store_floating (valbuf,len, val);
+ }
+ }
+ else
+ {
+ int offset = 0;
+ /* return value is copied starting from r2. */
+ if (TYPE_LENGTH (valtype) < S390_GPR_SIZE)
+ offset = S390_GPR_SIZE - TYPE_LENGTH (valtype);
+ memcpy (valbuf,
+ regbuf + REGISTER_BYTE (S390_GP0_REGNUM + 2) + offset,
+ TYPE_LENGTH (valtype));
+ }
+}
+
+
+static char *
+s390_promote_integer_argument(struct type *valtype,char *valbuf,char
*reg_buff,int *arglen)
+{
+ char *value=valbuf;
+ int len=TYPE_LENGTH (valtype);
+
+ if (len < S390_GPR_SIZE)
+ {
+ /* We need to upgrade this value to a register to pass it correctly
*/
+ int idx,diff=S390_GPR_SIZE-len,negative = (!TYPE_UNSIGNED (valtype)
&&value[0]&0x80);

Again, expression indentation. Here, separating the two (er three!)
declarations would also be a good idea.

+ for(idx=0;idx<S390_GPR_SIZE;idx++)
+ {
+ reg_buff[idx] = (idx<diff ? (negative ? 0xff:0x0) :
+ value[idx-diff] );

indentation

+ }
+ value=reg_buff;
+ *arglen = S390_GPR_SIZE;

Indentation `` = ''.

+ }
+ else
+ {
+ if (len & (S390_GPR_SIZE - 1))
+ {
+ fprintf_unfiltered (gdb_stderr,
+ "s390_promote_integer_argument detected an argument
not "
+ "a multiple of S390_GPR_SIZE & greater than
S390_GPR_SIZE "
+ "we might not deal with this correctly.\n");
+ }
+ *arglen = len;
+ }
+
+ return(value);

Indentation:
return value;
or return (value);

+}
+
+void
+s390_store_return_value (struct type *valtype, char *valbuf)
+{
+ int arglen;
+ char *reg_buff=alloca(max(S390_FPR_SIZE,S390_REGISTER_SIZE)),*value;
+
+ if (TYPE_CODE (valtype) == TYPE_CODE_FLT)
+ {
+ DOUBLEST tempfloat = extract_floating (valbuf,TYPE_LENGTH
(valtype));
+
+ floatformat_from_doublest ( &floatformat_ieee_double_big,
&tempfloat, reg_buff);
+ write_register_bytes (REGISTER_BYTE
(S390_FP0_REGNUM),reg_buff,S390_FPR_SIZE);
+ }
+ else
+ {
+ value=s390_promote_integer_argument(valtype,valbuf,reg_buff,
&arglen);

Indentation.

+ /* Everything else is returned in GPR2 and up. */
+ write_register_bytes (REGISTER_BYTE (S390_GP0_REGNUM + 2), value,
arglen);
+ }
+}
+static int
+gdb_print_insn_s390 (bfd_vma memaddr, disassemble_info * info)
+{
+ __u8 instrbuff[S390_MAX_INSTR_SIZE];

char

+ int instrlen, cnt;
+
+ instrlen = s390_readinstruction (instrbuff, (CORE_ADDR) memaddr, info);
+ if (instrlen < 0)
+ {
+ (*info->memory_error_func) (instrlen, memaddr, info);
+ return -1;
+ }
+ for (cnt = 0; cnt < instrlen; cnt++)
+ info->fprintf_func (info->stream, "%02X ", instrbuff[cnt]);
+ for (cnt = instrlen; cnt < S390_MAX_INSTR_SIZE; cnt++)
+ info->fprintf_func (info->stream, " ");
+ instrlen = print_insn_s390 (memaddr, info);
+ return instrlen;
+}
+
+
+
+/* Not the most efficent code in the world */
+int
+s390_fp_regnum ()
+{
+ int regno = S390_SP_REGNUM;
+ struct frame_extra_info fextra_info;
+ s390_get_frame_info (s390_sniff_pc_function_start
+ (read_register (S390_PC_REGNUM)), &fextra_info, NULL,
+ TRUE);
+ if (fextra_info.good_prologue && fextra_info.has_frame_pointer)
+ regno = S390_FRAME_REGNUM;
+ return regno;
+}
+
+CORE_ADDR s390_read_fp ()
+{
+ return read_register (s390_fp_regnum ());
+}
+
+
+void
+s390_write_fp (CORE_ADDR val)
+{
+ write_register (s390_fp_regnum (), val);
+}
+
+
+void
+s390_push_dummy_frame ()
+{
+ CORE_ADDR orig_sp = read_register (S390_SP_REGNUM), new_sp;
+ void *saved_regs=alloca(SIZEOF_S390_GDB_REGS);
+
+ new_sp = (orig_sp - (SIZEOF_S390_GDB_REGS + S390_GPR_SIZE));
+ read_register_bytes (0, (char *)saved_regs,SIZEOF_S390_GDB_REGS);
+ /* Use saved copy instead of orig_sp as this will have the correct
endianness */
+ write_memory (new_sp, (char *)saved_regs+
+ s390_offsetof(s390x_gdb_regs,s390_gdb_regs,gprs[S390_SP_REGNUM]),
+ S390_GPR_SIZE);
+ write_memory (new_sp + S390_GPR_SIZE, (char *) &saved_regs,
+ SIZEOF_S390_GDB_REGS);
+ write_register (S390_SP_REGNUM, new_sp);
+}
+
+/* pop the innermost frame, go back to the caller.
+ Used in `call_function_by_hand' to remove an artificial stack
+ frame. */

Comment style.

+void
+s390_pop_frame ()
+{
+ CORE_ADDR new_sp = read_register (S390_SP_REGNUM), orig_sp;
+ void *saved_regs=alloca(SIZEOF_S390_GDB_REGS);
+
+
+ read_memory (new_sp + S390_GPR_SIZE, (char *)saved_regs,
+ s390_sizeof(s390x_gdb_regs,s390_gdb_regs));
+ write_register_bytes (0, (char *) &saved_regs,SIZEOF_S390_GDB_REGS);
+}
+
+/* used by call function by hand
+ struct_return indicates that this function returns a structure &
+ therefore gpr2 stores a pointer to the structure to be returned as
+ opposed to the first argument.
+ Currently I haven't seen a TYPE_CODE_INT whose size wasn't 2^n or less
+ than S390_GPR_SIZE this is good because I don't seem to have to worry
+ about sign extending pushed arguments (i.e. a signed char currently
+ comes into this code with a size of 4 ). */

Comment style. Suggest a blank line be tween paragraphs.

+
+CORE_ADDR
+s390_push_arguments (int nargs, value_ptr * args, CORE_ADDR sp,
+ int struct_return, CORE_ADDR struct_addr)
+{
+ int num_float_args, num_gpr_args, orig_num_gpr_args, argno;
+ int second_pass, len, arglen, gprs_required;
+ CORE_ADDR outgoing_args_ptr, outgoing_args_space;
+ value_ptr arg;
+ struct type *type;
+ int max_num_gpr_args = 5 - (struct_return ? 1 : 0);
+ int arg0_regnum = S390_GP0_REGNUM + 2 + (struct_return ? 1 : 0);
+ char *reg_buff=alloca(max(S390_FPR_SIZE,S390_REGISTER_SIZE)),*value;

style.

+
+ for (second_pass = FALSE; second_pass <= TRUE; second_pass++)
+ {
+ if (second_pass)
+ outgoing_args_ptr = sp + S390_STACK_FRAME_OVERHEAD;
+ else
+ outgoing_args_ptr = 0;

Indentation.

+ num_float_args = 0;
+ num_gpr_args = 0;
+ for (argno = 0; argno < nargs; argno++)
+ {
+ arg = args[argno];
+ type = check_typedef (VALUE_TYPE (arg));
+ len = TYPE_LENGTH (type);
+ if (TYPE_CODE (type) == TYPE_CODE_FLT)
+ {
+ int all_float_registers_used=num_float_args >
(GDB_TARGET_IS_ESAME ? 3 : 1);
+
+ if(second_pass)
+ {
+ DOUBLEST tempfloat = extract_floating
(VALUE_CONTENTS(arg),len);
+
+
+ floatformat_from_doublest ( all_float_registers_used &&
+ len == (TARGET_FLOAT_BIT>>3)
+ ? &floatformat_ieee_single_big
+ : &floatformat_ieee_double_big,
+ &tempfloat, reg_buff);

Indentation and expressons. No `` '' after ``(''.

+ if(all_float_registers_used)
+ write_memory (outgoing_args_ptr,reg_buff, len);
+ else
+ write_register_bytes (REGISTER_BYTE((S390_FP0_REGNUM)
+ + (2 * num_float_args)),reg_buff,
+ S390_FPR_SIZE);
+ }
+ if(all_float_registers_used)
+ outgoing_args_ptr += len;
+ num_float_args++;
+ }
+ else
+ {
+ gprs_required = ((len + (S390_GPR_SIZE - 1)) / S390_GPR_SIZE);
+
+
value=s390_promote_integer_argument(type,VALUE_CONTENTS(arg),reg_buff,
&arglen);
+
+ orig_num_gpr_args = num_gpr_args;
+ num_gpr_args += gprs_required;
+ if (num_gpr_args > max_num_gpr_args)
+ {
+ if (second_pass)
+ write_memory (outgoing_args_ptr, value, arglen);
+ outgoing_args_ptr += arglen;
+ }
+ else
+ {
+ if (second_pass)
+ write_register_bytes (REGISTER_BYTE (arg0_regnum)
+ + (orig_num_gpr_args * S390_GPR_SIZE),
+ value,arglen);
+ }
+ }
+ }
+ if (!second_pass)
+ {
+ outgoing_args_space = outgoing_args_ptr;
+ /* Align to 16 bytes because because I like alignment &
+ some of the kernel code requires 8 byte stack alignment at least.
*/
+ sp = (sp - (S390_STACK_FRAME_OVERHEAD + outgoing_args_ptr)) & (-16);
+ }
+
+ }
+ return sp;
+
+}
+
+
+#if defined(GDB_MULTI_ARCH) || !defined(CONFIG_ARCH_S390X)

This #if is not needed. This code should always be compiled.

+void
+s390_fix_call_dummy (char *dummy, CORE_ADDR pc, CORE_ADDR fun, int nargs,
+ struct value **args, struct type *value_type,
+ int using_gcc)
+{
+ *(__u32 *) ((char *) &dummy[4]) = fun;

This isn't portable. Probably need to use store_...().

+}
+/* Number of bytes of storage in the actual machine representation
+ for register N.
+ Note that the unsigned cast here forces the result of the
+ subtraction to very high positive values if N < S390_FP0_REGNUM */
+int
+s390_register_raw_size (int reg_nr)
+{
+ return ((unsigned) reg_nr - S390_FP0_REGNUM) <
+ S390_NUM_FPRS ? S390_FPR_SIZE : 4;

expression style.

+}
+
+/* Return the GDB type object for the "standard" data type
+ of data in register N. */
+struct type *
+s390_register_virtual_type (int regno)
+{
+ return ((unsigned) regno - S390_FPC_REGNUM) <
+ S390_NUM_FPRS ? builtin_type_double : builtin_type_int;

expression style

+}
+
+#endif
+
+#if defined(GDB_MULTI_ARCH) || defined(CONFIG_ARCH_S390X)

The #if should not be present, this code should always be compiled.

+void
+s390x_fix_call_dummy (char *dummy, CORE_ADDR pc, CORE_ADDR fun, int nargs,
+ struct value **args, struct type *value_type,
+ int using_gcc)
+{
+ *(__u64 *) ((char *) &dummy[4]) = fun;

this isn't portable

+}
+
+int
+s390x_register_raw_size (int reg_nr)
+{
+ return (reg_nr == S390_FPC_REGNUM)
+ || (reg_nr >= S390_FIRST_ACR && reg_nr <= S390_LAST_ACR) ? 4 : 8 ;

expression style. I'd change this to a simpler if().

+}
+
+struct type *
+s390x_register_virtual_type (int regno)
+{
+ return (regno == S390_FPC_REGNUM) ||
+ (regno >= S390_FIRST_ACR && regno <= S390_LAST_ACR) ? builtin_type_int
:
+ (regno >= S390_FP0_REGNUM) ? builtin_type_double : builtin_type_long;

expression style I'd change this to a simpler if().

+}
+
+#endif
+
+
+void
+s390_store_struct_return (CORE_ADDR addr, CORE_ADDR sp)
+{
+ write_register (S390_GP0_REGNUM + 2, addr);
+}
+
+
+
+static unsigned char *
+s390_breakpoint_from_pc (CORE_ADDR * pcptr, int *lenptr)
+{
+ static unsigned char breakpoint[] = S390_BREAKPOINT;
+
+ *lenptr = sizeof (breakpoint);
+ return breakpoint;
+}
+
+/* Advance PC across any function entry prologue instructions to reach
some
+ "real" code. */
+CORE_ADDR
+s390_skip_prologue (CORE_ADDR pc)
+{
+ struct frame_extra_info fextra_info;
+
+ s390_get_frame_info (pc, &fextra_info, NULL, TRUE);
+ return fextra_info.skip_prologue_function_start;
+}
+
+/* pc_in_call_dummy_on stack may work for us must test this */
+int
+s390_pc_in_call_dummy (CORE_ADDR pc, CORE_ADDR sp, CORE_ADDR
frame_address)
+{
+ return pc > sp && pc < (sp + 4096);
+}
+
+/* Immediately after a function call, return the saved pc.
+ Can't go through the frames for this because on some machines
+ the new frame is not set up until the new function executes
+ some instructions. */
+CORE_ADDR s390_saved_pc_after_call (struct frame_info * frame)

Don't forget, function at start of a line.

+{
+ return ADDR_BITS_REMOVE (read_register (S390_RETADDR_REGNUM));
+}
+
+#if GDB_MULTI_ARCH

The #if isn't needed.

+static CORE_ADDR
+s390_addr_bits_remove (CORE_ADDR addr)
+{
+ return (addr) & 0x7fffffff;
+}
+#endif
+
+int
+s390_register_byte (int reg_nr)
+{
+ return (((reg_nr) <= S390_GP_LAST_REGNUM ? (reg_nr) * S390_GPR_SIZE :
+ (reg_nr) <=
+ S390_LAST_ACR ? (S390_ACR0_OFFSET +
+ (((reg_nr) - S390_FIRST_ACR) *
+ S390_ACR_SIZE)) : (reg_nr) <=
+ S390_LAST_CR ? (S390_CR0_OFFSET +
+ (((reg_nr) - S390_FIRST_CR) *
+ S390_CR_SIZE)) : (reg_nr) ==
+ S390_FPC_REGNUM ? S390_FPC_OFFSET : (S390_FP0_OFFSET +
+ (((reg_nr) - S390_FP0_REGNUM)
+ * S390_FPR_SIZE))));

Please rewrite this as an if() else if() chain.

+}
+
+#if GDB_MULTI_ARCH

The #if isn't needed.

+struct gdbarch *
+s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
+{
+ static LONGEST s390_call_dummy_words[] = S390_CALL_DUMMY;
+ static LONGEST s390x_call_dummy_words[] = S390X_CALL_DUMMY;
+ struct gdbarch *gdbarch;
+ struct gdbarch_tdep *tdep;
+ int elf_flags;
+
+ /* First see if there is already a gdbarch that can satisfy the request.
*/
+ arches = gdbarch_list_lookup_by_info (arches, &info);
+ if (arches != NULL)
+ return arches->gdbarch;
+
+ /* None found: is the request for a s390 architecture? */
+ if (info.bfd_architecture != bfd_arch_s390)
+ return NULL; /* No; then it's not for us. */
+
+ /* Yes: create a new gdbarch for the specified machine type. */
+ gdbarch = gdbarch_alloc (&info, NULL);
+
+ set_gdbarch_believe_pcc_promotion (gdbarch, 0);
+
+ /* We don't define set_gdbarch_call_dummy_breakpoint_offset
+ as we already have a breakpoint inserted. */
+ set_gdbarch_use_generic_dummy_frames (gdbarch, 0);
+
+ set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
+ set_gdbarch_call_dummy_start_offset (gdbarch, 0);
+ set_gdbarch_pc_in_call_dummy (gdbarch, s390_pc_in_call_dummy);
+ set_gdbarch_frame_args_skip (gdbarch, 0);
+ set_gdbarch_frame_args_address (gdbarch, s390_frame_args_address);
+ set_gdbarch_frame_chain (gdbarch, s390_frame_chain);
+ set_gdbarch_frame_init_saved_regs (gdbarch, s390_frame_init_saved_regs);
+ set_gdbarch_frame_locals_address (gdbarch, s390_frame_args_address);
+ /* We can't do this */
+ set_gdbarch_frame_num_args (gdbarch, frame_num_args_unknown);
+ set_gdbarch_store_struct_return (gdbarch, s390_store_struct_return);
+ set_gdbarch_extract_return_value (gdbarch, s390_extract_return_value);
+ set_gdbarch_store_return_value (gdbarch, s390_store_return_value);
+ /* Amount PC must be decremented by after a breakpoint.
+ This is often the number of bytes in BREAKPOINT
+ but not always. */
+ set_gdbarch_decr_pc_after_break (gdbarch, 2);
+ set_gdbarch_pop_frame (gdbarch, s390_pop_frame);
+ set_gdbarch_push_dummy_frame (gdbarch, s390_push_dummy_frame);
+ set_gdbarch_push_arguments (gdbarch, s390_push_arguments);
+ set_gdbarch_ieee_float (gdbarch, 1);
+ /* Stack grows downward. */
+ set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
+ /* Offset from address of function to start of its code.
+ Zero on most machines. */
+ set_gdbarch_function_start_offset (gdbarch, 0);
+ set_gdbarch_max_register_raw_size (gdbarch, 8);
+ set_gdbarch_max_register_virtual_size (gdbarch, 8);
+ set_gdbarch_breakpoint_from_pc (gdbarch, s390_breakpoint_from_pc);
+ set_gdbarch_skip_prologue (gdbarch, s390_skip_prologue);
+ set_gdbarch_init_extra_frame_info (gdbarch, s390_init_extra_frame_info);
+ set_gdbarch_init_frame_pc_first (gdbarch, s390_init_frame_pc_first);
+ set_gdbarch_read_fp (gdbarch, s390_read_fp);
+ set_gdbarch_write_fp (gdbarch, s390_write_fp);
+ /* This function that tells us whether the function invocation
represented
+ by FI does not have a frame on the stack associated with it. If it
+ does not, FRAMELESS is set to 1, else 0. */
+ set_gdbarch_frameless_function_invocation (gdbarch,
+ s390_frameless_function_invocation);
+ /* Return saved PC from a frame */
+ set_gdbarch_frame_saved_pc (gdbarch, s390_frame_saved_pc);
+ /* FRAME_CHAIN takes a frame's nominal address
+ and produces the frame's chain-pointer. */
+ set_gdbarch_frame_chain (gdbarch, s390_frame_chain);
+ set_gdbarch_saved_pc_after_call (gdbarch, s390_saved_pc_after_call);
+ set_gdbarch_register_byte (gdbarch, s390_register_byte);
+ set_gdbarch_pc_regnum (gdbarch, S390_PC_REGNUM);
+ set_gdbarch_sp_regnum (gdbarch, S390_SP_REGNUM);
+ set_gdbarch_fp_regnum (gdbarch, S390_FP_REGNUM);
+ set_gdbarch_fp0_regnum (gdbarch, S390_FP0_REGNUM);
+ set_gdbarch_num_regs (gdbarch, S390_NUM_REGS);
+ set_gdbarch_cannot_fetch_register (gdbarch, s390_cannot_fetch_register);
+ set_gdbarch_cannot_store_register (gdbarch, s390_cannot_fetch_register);
+ set_gdbarch_get_saved_register (gdbarch, generic_get_saved_register);
+ set_gdbarch_use_struct_convention (gdbarch,
generic_use_struct_convention);
+ set_gdbarch_frame_chain_valid (gdbarch, file_frame_chain_valid);
+ set_gdbarch_register_name (gdbarch, s390_register_name);
+ set_gdbarch_stab_reg_to_regnum (gdbarch, s390_stab_reg_to_regnum);
+ set_gdbarch_dwarf_reg_to_regnum (gdbarch, s390_stab_reg_to_regnum);
+ set_gdbarch_dwarf2_reg_to_regnum (gdbarch, s390_stab_reg_to_regnum);
+
+
+ /* Stuff below here wouldn't be required if gdbarch.sh was a little */
+ /* more intelligent */
+ set_gdbarch_call_dummy_breakpoint_offset_p (gdbarch, 0);
+ set_gdbarch_call_dummy_p (gdbarch, 1);
+ set_gdbarch_call_dummy_stack_adjust_p (gdbarch, 0);
+ set_gdbarch_extract_struct_value_address_p (gdbarch, 0);
+ switch (info.bfd_arch_info->mach)
+ {
+ case bfd_mach_s390_esa:
+ set_gdbarch_register_size (gdbarch, 4);
+ set_gdbarch_call_dummy_length (gdbarch, S390_CALL_DUMMY_LENGTH);
+ set_gdbarch_register_raw_size (gdbarch, s390_register_raw_size);
+ set_gdbarch_register_virtual_size (gdbarch, s390_register_raw_size);
+ set_gdbarch_register_virtual_type (gdbarch,
s390_register_virtual_type);
+
+ set_gdbarch_addr_bits_remove (gdbarch, s390_addr_bits_remove);
+ set_gdbarch_fix_call_dummy (gdbarch, s390_fix_call_dummy);
+ set_gdbarch_sizeof_call_dummy_words (gdbarch,
+ sizeof (s390_call_dummy_words));
+ set_gdbarch_call_dummy_words (gdbarch, s390_call_dummy_words);
+ break;
+ case bfd_mach_s390_esame:
+ set_gdbarch_register_size (gdbarch, 8);
+ set_gdbarch_call_dummy_length (gdbarch, S390X_CALL_DUMMY_LENGTH);
+ set_gdbarch_register_raw_size (gdbarch, s390x_register_raw_size);
+ set_gdbarch_register_virtual_size (gdbarch,
s390x_register_raw_size);
+ set_gdbarch_register_virtual_type (gdbarch,
+ s390x_register_virtual_type);
+
+ set_gdbarch_long_bit (gdbarch, 64);
+ set_gdbarch_long_long_bit (gdbarch, 64);
+ set_gdbarch_ptr_bit (gdbarch, 64);
+ set_gdbarch_fix_call_dummy (gdbarch, s390x_fix_call_dummy);
+ set_gdbarch_sizeof_call_dummy_words (gdbarch,
+ sizeof (s390x_call_dummy_words));
+ set_gdbarch_call_dummy_words (gdbarch, s390x_call_dummy_words);
+ break;
+ }
+ /* REGISTER_SIZE is set up so this is correct here */
+ set_gdbarch_register_bytes (gdbarch, S390_REGISTER_BYTES);
+ return gdbarch;
+}
+#endif
+
+
+void
+_initialize_s390_tdep ()
+{
+ const bfd_arch_info_type *s390_arch_ptr =
+ bfd_lookup_arch (bfd_arch_s390, 0);
+
+#if GDB_MULTI_ARCH

The #if isn't needed.

+ /* Hook us into the gdbarch mechanism. */
+ register_gdbarch_init (bfd_arch_s390, s390_gdbarch_init);
+#endif
+ if (!tm_print_insn) /* Someone may have already set it */
+ tm_print_insn = gdb_print_insn_s390;
+ if (s390_arch_ptr)
+ tm_print_insn_info.mach = s390_arch_ptr->mach;
+ else
+ internal_error (__FILE__, __LINE__,
+ "_initialize_s390_tdep: bfd_lookup_arch failed for
bfd_arch_s390 recompile.\n");

I don't understand what this is doing.

+}