Re: [PATCH] S/390 DWARF-2 CFI frame support

[Andrew Cagney <cagney@gnu.org>]

> Hello,
> 
> this patch adds support for DWARF-2 frame handling to the s390 backend.
> 
> However, the DWARF-2 frame common code makes a couple of assumptions
> that are not valid on s390:
> 
> - The return address column is not unwound into the specified register,
>   only into the PC.  On s390, it should be unwound into both.
> 
>   The patch below fixes this by unwinding into both register
>   and PC if the return column corresponds to a GDB register.

What does the CFI output look like here?  (I'm mainly curious but it 
always helps to have concrete examples).

For a variant of the PPC64 GCC I found:

- return column pointed at the FSCR
(which is stupid)

- return column same-value as "LR" was implied
(which is contrary to the DWARF2's example, and what you also encountered)

this means that we've now got the following initial states all mapping 
onto "unspecified":

- same value
- unknown
- (for SP_REGNUM) it's the CFA (but is it one frame out?)
- (for return column) it's the LR

I'm wondering if it would be easier, here to "give up".  Throw the whole 
problem back at the architecture vis:

- initialize unwind table using per OSABI (?) method

- update the unwind table using the CFI initialize info

- update the unwind table using the PC's CFI unwind info

- if after all this a register is still unspecified, we complain
(btw, the compaint is only ment to appear once but apparently appears 
repeatedly?).

Thoughts for the moment on the theory?  Mark?

It should also open the way for the elimination of SP_REGNUM and 
PC_REGNUM from this file (ya!).

As for an interface, the <ARCH>_gdbarch_init code could something like:

	dwarf2_frame_add_sniffer (gdbarch, (*<ARCH>_dwarf2_cfi_init) (struct 
cfi_table *table));

(but with better names :-).

Andrew


> - Unwinding the return address column had this line of code:
>  	  cache->reg[PC_REGNUM].loc.reg = reg;
>   which stores a GDB register number into a place where DWARF-2 
>   register numbers are expected.  This fails on s390 where the two
>   numbering schemes diverge.
> 
>   This is fixed below by using a DWARF-2 register number as appropriate.
> 
> 
> - Handling of unspecified registers made a couple of assumptions
>   peculiar to i386 (or amd64); in particular that an unspecified
>   stack pointer is supposed to be unwound to the CFA.  This is false
>   on s390, where the CFA is biased relative to the stack pointer.
>   Also, the presence of unspecified registers causes warnings to
>   be emitted, which is somewhat awkward, as this is just the way
>   GCC does it ...
> 
>   To fix this, I suggest the following.  What GCC assumes to happen
>   when it leaves a register unspecified in the CFI depends on whether
>   the register is call-saved or call-clobbered according to the 
>   target's ABI.  If it is call-saved (and unspecified), the function
>   doesn't save/restore it because it does not in fact ever modify it.
>   Thus, in this case the debugger should copy the value from the 
>   inner frame.  If it is call-clobbered (those will always be left
>   unspecified), it should be assumed undefined.
> 
>   Now, the GDB common code doesn't know which registers are call-saved
>   and which are call-clobbered.  Thus, I've extended the register
>   group mechanism by providing two new system reggroups,
>   call_saved_reggroup and call_clobbered_reggroup, which the target
>   can define according to the platform ABI (which the target code 
>   can certainly know).  The patch below uses those two groups to
>   handle REG_UNSPECIFIED registers as either REG_SAME_VALUE or
>   REG_UNDEFINED.
> 
>   I'm using two reggroups instead of one to leave the present 
>   behaviour unchanged for existing targets, and also to allow
>   a target to, say, place the stack pointer register neither in
>   call_saved_reggroup nor call_clobbered_reggroup, in which case
>   the current sp = CFA special case still triggers.
> 
>   I've also simply removed the 'complaint' about unspecified 
>   registers because this is now no longer supposed to be a problem.
> 
> 
> Tested on s390-ibm-linux and s390x-ibm-linux with no new regressions.
> (In fact, even with prologue-based frame analysis switched off,
> there are no new regressions, with the exception of the asm-source
> case because the assembler test case doesn't provide CFI data.  
> I've still left prologue analysis on for backward-compatibility 
> with older systems that don't yet have DWARF-2 CFIs everywhere.)

>   	if (regnum < 0 || regnum >= num_regs)
>   	  continue;
>   
>   	cache->reg[regnum] = fs->regs.reg[column];
>         }
>     }
>   
> +   /* Among the registers the CFI generated by GCC leaves unspecified,
> +      those that are call-saved according to the target's ABI are presumed
> +      to inherit their value from the next frame, while those that are
> +      call-clobbered should be considered undefined.  */
> +   {
> +     int regnum;
> +     for (regnum = 0; regnum < num_regs; regnum++)
> +       if (cache->reg[regnum].how == REG_UNSPECIFIED)
> + 	{
> + 	  if (gdbarch_register_reggroup_p (gdbarch, regnum, 
> + 					   call_saved_reggroup))
> + 	    cache->reg[regnum].how = REG_SAME_VALUE;
> + 
> + 	  else if (gdbarch_register_reggroup_p (gdbarch, regnum, 
> + 						call_clobbered_reggroup))
> + 	    cache->reg[regnum].how = REG_UNDEFINED;
> + 	}
> +   }
> + 
>     /* Store the location of the return addess.  If the return address
> !      column (adjusted) is not the same as gdb's PC_REGNUM, then this
> !      implies a copy from the ra column register.  */
> !   if (PC_REGNUM >= 0)
>       {
>         int reg = DWARF2_REG_TO_REGNUM (fs->retaddr_column);
> !       if (reg != PC_REGNUM && reg >= 0 && reg < num_regs)
>   	{
> ! 	  cache->reg[PC_REGNUM] = cache->reg[reg];
> ! 
> ! 	  /* 'Same value' in this case refers to the return address
> ! 	     register, not the PC register.  */
> ! 	  if (cache->reg[PC_REGNUM].how == REG_SAME_VALUE)
> ! 	    {
> ! 	      cache->reg[PC_REGNUM].loc.reg = fs->retaddr_column;
> ! 	      cache->reg[PC_REGNUM].how = REG_SAVED_REG;
> ! 	    }
>   	}
>       }
>   
> diff -c -p -r gdb-head/gdb/reggroups.c gdb-head-new/gdb/reggroups.c
> *** gdb-head/gdb/reggroups.c	Wed Dec  3 14:50:26 2003
> --- gdb-head-new/gdb/reggroups.c	Wed Dec  3 14:50:31 2003
> *************** static struct reggroup vector_group = { 
> *** 254,259 ****
> --- 254,261 ----
>   static struct reggroup all_group = { "all", USER_REGGROUP };
>   static struct reggroup save_group = { "save", INTERNAL_REGGROUP };
>   static struct reggroup restore_group = { "restore", INTERNAL_REGGROUP };
> + static struct reggroup call_saved_group = { "call-saved", INTERNAL_REGGROUP };
> + static struct reggroup call_clobbered_group = { "call-clobbered", INTERNAL_REGGROUP };
>   
>   struct reggroup *const general_reggroup = &general_group;
>   struct reggroup *const float_reggroup = &float_group;
> *************** struct reggroup *const vector_reggroup =
> *** 262,267 ****
> --- 264,271 ----
>   struct reggroup *const all_reggroup = &all_group;
>   struct reggroup *const save_reggroup = &save_group;
>   struct reggroup *const restore_reggroup = &restore_group;
> + struct reggroup *const call_saved_reggroup = &call_saved_group;
> + struct reggroup *const call_clobbered_reggroup = &call_clobbered_group;
>   
>   extern initialize_file_ftype _initialize_reggroup; /* -Wmissing-prototypes */
>   
> *************** _initialize_reggroup (void)
> *** 278,283 ****
> --- 282,289 ----
>     add_group (&default_groups, all_reggroup, XMALLOC (struct reggroup_el));
>     add_group (&default_groups, save_reggroup, XMALLOC (struct reggroup_el));
>     add_group (&default_groups, restore_reggroup, XMALLOC (struct reggroup_el));
> +   add_group (&default_groups, call_saved_reggroup, XMALLOC (struct reggroup_el));
> +   add_group (&default_groups, call_clobbered_reggroup, XMALLOC (struct reggroup_el));
>   
>     add_cmd ("reggroups", class_maintenance,
>   	   maintenance_print_reggroups, "\
> diff -c -p -r gdb-head/gdb/reggroups.h gdb-head-new/gdb/reggroups.h
> *** gdb-head/gdb/reggroups.h	Wed Dec  3 14:50:26 2003
> --- gdb-head-new/gdb/reggroups.h	Wed Dec  3 14:50:31 2003
> *************** extern struct reggroup *const all_reggro
> *** 39,44 ****
> --- 39,46 ----
>   /* Pre-defined, internal, register groups.  */
>   extern struct reggroup *const save_reggroup;
>   extern struct reggroup *const restore_reggroup;
> + extern struct reggroup *const call_saved_reggroup;
> + extern struct reggroup *const call_clobbered_reggroup;
>   
>   /* Create a new local register group.  */
>   extern struct reggroup *reggroup_new (const char *name,
> diff -c -p -r gdb-head/gdb/s390-tdep.c gdb-head-new/gdb/s390-tdep.c
> *** gdb-head/gdb/s390-tdep.c	Wed Dec  3 14:50:26 2003
> --- gdb-head-new/gdb/s390-tdep.c	Wed Dec  3 14:50:31 2003
> ***************
> *** 39,44 ****
> --- 39,45 ----
>   #include "trad-frame.h"
>   #include "frame-base.h"
>   #include "frame-unwind.h"
> + #include "dwarf2-frame.h"
>   #include "reggroups.h"
>   #include "regset.h"
>   #include "value.h"
> *************** s390_register_reggroup_p (struct gdbarch
> *** 435,440 ****
> --- 436,469 ----
>     if (group == save_reggroup || group == restore_reggroup)
>       return regnum != S390_PSWM_REGNUM && regnum != S390_PSWA_REGNUM;
>   
> +   /* Call-saved registers.  */
> +   if (group == call_saved_reggroup)
> +     switch (tdep->abi)
> +       {
> +       case ABI_LINUX_S390:
> + 	return (regnum >= S390_R6_REGNUM && regnum <= S390_R15_REGNUM)
> + 	       || regnum == S390_F4_REGNUM
> + 	       || regnum == S390_F6_REGNUM;
> + 
> +       case ABI_LINUX_ZSERIES:
> + 	return (regnum >= S390_R6_REGNUM && regnum <= S390_R15_REGNUM)
> + 	       || (regnum >= S390_F8_REGNUM && regnum <= S390_F15_REGNUM);
> +       }
> + 
> +   /* Call-clobbered registers.  */
> +   if (group == call_clobbered_reggroup)
> +     switch (tdep->abi)
> +       {
> +       case ABI_LINUX_S390:
> + 	return (regnum >= S390_R0_REGNUM && regnum <= S390_R5_REGNUM)
> + 	       || (regnum >= S390_F0_REGNUM && regnum <= S390_F15_REGNUM
> + 		   && regnum != S390_F4_REGNUM && regnum != S390_F6_REGNUM);
> + 
> +       case ABI_LINUX_ZSERIES:
> + 	return (regnum >= S390_R0_REGNUM && regnum <= S390_R5_REGNUM)
> + 	       || (regnum >= S390_F0_REGNUM && regnum <= S390_F7_REGNUM);
> +       }
> + 
>     return default_register_reggroup_p (gdbarch, regnum, group);
>   }
>