[PATCH 1/4] Split up s390-linux-tdep.c into two files

[Ulrich Weigand]

Philipp Rudo wrote:

> Currently all target dependent code for s390 is in one file
> (s390-linux-tdep.c).  This includes code general for the architecture as
> well as code specific for uses in GNU/Linux (user space).  Up until now
> this was ok as GNU/Linux was the only supported OS.  In preparation to
> support the new Linux kernel 'OS' split up the existing s390 code into a
> general s390-tdep and a GNU/Linux specific s390-linux-tdep.

Thanks for working on this!

A couple of comments on the particular split:


> -static void
> -s390_write_pc (struct regcache *regcache, CORE_ADDR pc)
> -{
> -  struct gdbarch *gdbarch = regcache->arch ();
> -  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> -
> -  regcache_cooked_write_unsigned (regcache, tdep->pc_regnum, pc);
> -
> -  /* Set special SYSTEM_CALL register to 0 to prevent the kernel from
> -     messing with the PC we just installed, if we happen to be within
> -     an interrupted system call that the kernel wants to restart.
> -
> -     Note that after we return from the dummy call, the SYSTEM_CALL and
> -     ORIG_R2 registers will be automatically restored, and the kernel
> -     continues to restart the system call at this point.  */
> -  if (register_size (gdbarch, S390_SYSTEM_CALL_REGNUM) > 0)
> -    regcache_cooked_write_unsigned (regcache, S390_SYSTEM_CALL_REGNUM, 0);
> -}

This seems a typical Linux issue.  Other OSes won't have the ORIG_R2
and SYSTEM_CALL registers and may not need this particular handling.

> -/* Maps for register sets.  */
> -
> -static const struct regcache_map_entry s390_gregmap[] =
> -  {
> -    { 1, S390_PSWM_REGNUM },
> -    { 1, S390_PSWA_REGNUM },
> -    { 16, S390_R0_REGNUM },
> -    { 16, S390_A0_REGNUM },
> -    { 1, S390_ORIG_R2_REGNUM },
> -    { 0 }
> -  };
> -
> -static const struct regcache_map_entry s390_fpregmap[] =
> -  {
> -    { 1, S390_FPC_REGNUM, 8 },
> -    { 16, S390_F0_REGNUM, 8 },
> -    { 0 }
> -  };
> -
>  static const struct regcache_map_entry s390_regmap_upper[] =
>    {
>      { 16, S390_R0_UPPER_REGNUM, 4 },

Not sure I understand your split here.  "ORIG_R2" is not a real register
and only exists on Linux, so it shouldn't move to s390-tdep.c.  On the
other hand, the split between upper halves and lower halves seems more
generic and could happen elsewhere ...

> +/* Set up GNU/Linux gdbarch.  Allocates struct gdbarch if needed.  */
>  
>  static struct gdbarch *
> -s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> +s390_linux_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  {

This is wrong.  The way the split setup between generic part and Linux
part is supposed to be working is this:

- The generic part (and only it) has a _initialize routine that calls
   register_gdbarch_init (bfd_arch_s390, s390_gdbarch_init);

- In the s390_gdbarch_init routine, the generic setup is done, and at
  the point where the platform-specific setup should begin, we call
   gdbarch_init_osabi (info, gdbarch);

- The OS-specific part also has a _initialize routine, which calls
  gdbarch_register_osabi (bfd_arch_s390, bfd_mach_s390_31, GDB_OSABI_LINUX,
                          s390_linux_init_abi);
  gdbarch_register_osabi (bfd_arch_s390, bfd_mach_s390_64, GDB_OSABI_LINUX,
                          s390_linux_init_abi);
  (may be the same or two different handlers for 31- vs 64-bit, whatever
  is more practical)

- The common gdbarch_init_osabi detects which OS handler to use and calls it.

This method is same across all Linux (and really all) targets; there is
no point for us to try to do it differently, that would just cause confusion.

> -      /* Optional GNU/Linux-specific "registers".  */
> -      feature = tdesc_find_feature (tdesc, "org.gnu.gdb.s390.linux");
> -      if (feature)
> -	{
> -	  tdesc_numbered_register (feature, tdesc_data,
> -				   S390_ORIG_R2_REGNUM, "orig_r2");
> -
> -	  if (tdesc_numbered_register (feature, tdesc_data,
> -				       S390_LAST_BREAK_REGNUM, "last_break"))
> -	    have_linux_v1 = 1;
> -
> -	  if (tdesc_numbered_register (feature, tdesc_data,
> -				       S390_SYSTEM_CALL_REGNUM, "system_call"))
> -	    have_linux_v2 = 1;
> -
> -	  if (have_linux_v2 > have_linux_v1)
> -	    valid_p = 0;
> -	}

The Linux specific feature should be handled here, the others in the 
generic part.

> -/* GNU/Linux-specific optional registers.  */
> -#define S390_ORIG_R2_REGNUM 67
> -#define S390_LAST_BREAK_REGNUM 68
> -#define S390_SYSTEM_CALL_REGNUM 69

Ideally, these should also remain here as Linux specific registers.
This can be done e.g. like ppc-linux-tdep.h does it.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com