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

Philipp Rudo prudo@linux.vnet.ibm.com
Fri Nov 24 16:55:00 GMT 2017


Hi Uli,

thanks for the review.

I already feared you will find something. The short answer to most of your
comments is that the split isn't a 100% clean. The new s390-tdep isn't pure
architecture code but mixed with common Linux ELF abi code. If you wanted
it clean we had to split up s390-tdep even further. But I don't know is if
it is worth the work.

More details are below.


On Thu, 23 Nov 2017 21:26:03 +0100 (CET)
"Ulrich Weigand" <uweigand@de.ibm.com> wrote:

> 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.

You are right for the kernel we will have to treat it different. I moved it
back to s390-linux-tdep.
 
> > -/* 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 ...

This is a little more complicated.

For the upper/lower split you are right, it is architecture. However i see
it as legacy code and I don't plan to add any support for 31-bit systems on
64-bit machines. So i decided to leave the definition where it is used.

For orig_r2 you are right (again), it only exist in user space. However
(again) the kernel uses the same prstatus definitions like the user space,
including a field for orig_r2 (although it is not used). Thats why i moved it
to the common 'Linux ELF abi' s390-tdep.

 
> > +/* 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.

I played around with osabi and never got it to work the way I wanted it to.
My problem was that ELF doesn't distinguish the kernel and user space. So the
default ELF sniffer always used the GDB_OSABI_LINUX and never gave my
GDB_OSABI_LINUX_KERNEL a chance. I cannot recall all details and will give it
a second chance. Please give me some time.

 
> > -      /* 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.

My plan was to take care of this when we change to dynamically creating
the tdesc. Then we could number the registers directly when creating the tdesc
(at least I hope so). To be honest I haven't done it yet as with all those
have_* variables there are some nasty dependencies in the function...


> > -/* 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.

The ppc way is actually pretty nice. I will use it.

Thanks
Philipp



More information about the Gdb-patches mailing list