Re: [rfa/6.0] Better handle unspecified CFI values

[Andrew Cagney <ac131313@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2839 bytes --]


> That's closer to the definitions used in the specification, so go
> ahead.  I used REG_UNSAVED since that's what GCC's unwinder uses, so
> perhaps you could add a comment that says this?

Ah!  That also explains why GCC has problems.  It's combined two 
different states :-/  I've added comments to that effect.


>> - if it detects an unspecified CFI entry it complains
>> It isn't perfect though - since it doesn't know the full range of valid 
>> debug info register numbers it can't check every entry.  Instead it 
>> checks the range provided by CFI for unspecified holes and then 
>> complains about that.  The reality is that GCC at least gets that bit 
>> right (but consistently forgets the SP).
> 
> 
> Sorry, GCC gets what right?

On the platforms I tested (amd64 & i386) all [0 .. max "column") were 
specified.  The problems were with numbers outside of that range.  I'll 
re-word the comment.

> 
> Anyway, the full range of valid debug info registers, is defined by
> the DWARF register mapping that some ABI's define (at least the x86-64
> ABI provides such a definition).  GDB sort of has this information in
> that we have the dwarf2_reg_to_regnum function in the architecture
> vector.

Yes.  There isn't a way of iterating over it.  Oh, and btw, it can also 
be sparse - I believe that the e500 (ppc variant) has has dwarf2 numbers 
 >1000.  Something to address in a later iteration.

>> I'd like to commit the patch as is for the 6.0 branch.  For the mainline 
>> though, I'd like to make the additional changes:
>> 
>> - delete the SP_REGNUM hack from the REG_UNDEFINED rule (it's no longer 
>> needed, I think)
> 
> 
> I guess that if the stack pointer is explicitly marked as being
> "undefined", all will be lost on most architectures.

Yes, bit of a loose-loose situtation.  If it's really "undefined" GDB 
can't use "CFI" as that would mislead the user into thinking that a 
bogus backtrace is valid.

>> - add a check/complaint for the SP v CFA problem.
>> 
>> Anyway, the end result is that on x86-64 and i386 store.exp now passes.
>> 
>> ok to commit?
> 
> 
> Hmm, three "style" nits:
> 
> - The uppercase MEMSET in the comment on `enum dwarf2_reg_rule' is
>   wrong.  Uppercase is reserved for when we're talking about the value
>   of a symbol, not the symbol itself.  So please convert this into
>   lowercase.
> 
> - I find the name `cfinum' a bit cryptic.  I understand that you want
>   to avoid `reg', how about calling it `column'?  That's sort of CFI
>   speak for a register number.
> 
> - Please remove the extra blank line before `case REG_SAME_VALUE:'.
> 
> With those changes this is OK (or can't I approve changes to a file
> I've written in the first place?).  You can also leave it to me to
> make those changes ;-).

I think they are fixed.  I've attached the diff I'll commit.

Andrew


[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 9795 bytes --]

2003-09-05  Andrew Cagney  <cagney@redhat.com>

	* dwarf2-frame.c (enum dwarf2_reg_rule): New, replace anonymous
	enum.  Add REG_UNSPECIFIED, rename REG_UNSAVED to REG_UNDEFINED
	and REG_UNMODIFIED to REG_SAME_VALUE.
	(execute_cfa_program): Update.
	(dwarf2_frame_cache): Update.  Initialize table to
	REG_UNSPECIFIED, complain if CFI fails to specify a register's
	location.
	(dwarf2_frame_prev_register): Update.  Handle REG_UNSPECIFIED.

--- /home/scratch/GDB/src/gdb/dwarf2-frame.c	Fri Sep  5 15:55:04 2003
+++ dwarf2-frame.c	Mon Sep  8 19:37:24 2003
@@ -97,6 +97,28 @@
 
 /* Structure describing a frame state.  */
 
+enum dwarf2_reg_rule
+{
+  /* Make certain that 0 maps onto the correct enum value - the
+     corresponding structure is being initialized using memset zero.
+     This indicates that CFI didn't provide any information at all
+     about a register - leaving how to obtain it's value totally
+     unspecified.  */
+  REG_UNSPECIFIED = 0,
+  /* The term "undefined" comes from the DWARF2 CFI spec which this
+     code is moddeling - it indicates that the register's value is
+     "undefined".  */
+  /* NOTE: cagney/2003-09-08: GCC uses the less formal term "unsaved"
+     - it's definition is a combination of REG_UNDEFINED and
+     REG_UNSPECIFIED - the failure to differentiate the two helps
+     explain a few problems with the CFI GCC outputs.  */
+  REG_UNDEFINED,
+  REG_SAVED_OFFSET,
+  REG_SAVED_REG,
+  REG_SAVED_EXP,
+  REG_SAME_VALUE
+};
+
 struct dwarf2_frame_state
 {
   /* Each register save state can be described in terms of a CFA slot,
@@ -111,13 +133,7 @@
 	unsigned char *exp;
       } loc;
       ULONGEST exp_len;
-      enum {
-	REG_UNSAVED,
-	REG_SAVED_OFFSET,
-	REG_SAVED_REG,
-	REG_SAVED_EXP,
-	REG_UNMODIFIED
-      } how;
+      enum dwarf2_reg_rule how;
     } *reg;
     int num_regs;
 
@@ -354,13 +370,13 @@
 	    case DW_CFA_undefined:
 	      insn_ptr = read_uleb128 (insn_ptr, insn_end, &reg);
 	      dwarf2_frame_state_alloc_regs (&fs->regs, reg + 1);
-	      fs->regs.reg[reg].how = REG_UNSAVED;
+	      fs->regs.reg[reg].how = REG_UNDEFINED;
 	      break;
 
 	    case DW_CFA_same_value:
 	      insn_ptr = read_uleb128 (insn_ptr, insn_end, &reg);
 	      dwarf2_frame_state_alloc_regs (&fs->regs, reg + 1);
-	      fs->regs.reg[reg].how = REG_UNMODIFIED;
+	      fs->regs.reg[reg].how = REG_SAME_VALUE;
 	      break;
 
 	    case DW_CFA_register:
@@ -460,11 +476,10 @@
 dwarf2_frame_cache (struct frame_info *next_frame, void **this_cache)
 {
   struct cleanup *old_chain;
-  int num_regs = NUM_REGS + NUM_PSEUDO_REGS;
+  const int num_regs = NUM_REGS + NUM_PSEUDO_REGS;
   struct dwarf2_frame_cache *cache;
   struct dwarf2_frame_state *fs;
   struct dwarf2_fde *fde;
-  int reg;
 
   if (*this_cache)
     return *this_cache;
@@ -535,34 +550,65 @@
       internal_error (__FILE__, __LINE__, "Unknown CFA rule.");
     }
 
-  /* Save the register info in the cache.  */
-  for (reg = 0; reg < fs->regs.num_regs; reg++)
-    {
-      int regnum;
-
-      /* Skip the return address column.  */
-      if (reg == fs->retaddr_column)
-	/* NOTE: cagney/2003-06-07: Is this right?  What if the
-           RETADDR_COLUM corresponds to a real register (and, worse,
-           that isn't the PC_REGNUM)?  I'm guessing that the PC_REGNUM
-           further down is trying to handle this.  That can't be right
-           though - PC_REGNUM may not be valid (it can be -ve).  I
-           think, instead when RETADDR_COLUM isn't a real register, it
-           should map itself onto frame_pc_unwind.  */
-	continue;
-
-      /* Use the GDB register number as index.  */
-      regnum = DWARF2_REG_TO_REGNUM (reg);
+  /* Initialize things so that all registers are marked as
+     unspecified.  */
+  {
+    int regnum;
+    for (regnum = 0; regnum < num_regs; regnum++)
+      cache->reg[regnum].how = REG_UNSPECIFIED;
+  }
 
-      if (regnum >= 0 && regnum < num_regs)
-	cache->reg[regnum] = fs->regs.reg[reg];
-    }
+  /* Go through the DWARF2 CFI generated table and save its register
+     location information in the cache.  */
+  {
+    int column;		/* CFI speak for "register number".  */
+    for (column = 0; column < fs->regs.num_regs; column++)
+      {
+	int regnum;
+	
+	/* Skip the return address column.  */
+	if (column == fs->retaddr_column)
+	  /* NOTE: cagney/2003-06-07: Is this right?  What if
+	     RETADDR_COLUMN corresponds to a real register (and,
+	     worse, that isn't the PC_REGNUM)?  I'm guessing that the
+	     PC_REGNUM further down is trying to handle this.  That
+	     can't be right though - PC_REGNUM may not be valid (it
+	     can be -ve).  I think, instead when RETADDR_COLUM isn't a
+	     real register, it should map itself onto frame_pc_unwind.  */
+	  continue;
+
+	/* Use the GDB register number as the destination index.  */
+	regnum = DWARF2_REG_TO_REGNUM (column);
+
+	/* If there's no corresponding GDB register, ignore it.  */
+	if (regnum < 0 || regnum >= num_regs)
+	  continue;
+
+	/* NOTE: cagney/2003-09-05: CFI should specify the disposition
+	   of all debug info registers.  If it doesn't complain (but
+	   not too loudly).  It turns out that GCC, assumes that an
+	   unspecified register implies "same value" when CFI (draft
+	   7) specifies nothing at all.  Such a register could equally
+	   be interpreted as "undefined".  Also note that this check
+	   isn't sufficient - it only checks that all registers in the
+	   range [0 .. max column] are specified - and won't detect
+	   problems when a debug info register falls outside of the
+	   table.  Need a way of iterating through all the valid
+	   DWARF2 register numbers.  */
+	if (fs->regs.reg[column].how == REG_UNSPECIFIED)
+	  complaint (&symfile_complaints,
+		     "Incomplete CFI data; unspecified registers at 0x%s",
+		     paddr (fs->pc));
+
+	cache->reg[regnum] = fs->regs.reg[column];
+      }
+  }
 
   /* 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 (fs->retaddr_column < fs->regs.num_regs
-      && fs->regs.reg[fs->retaddr_column].how != REG_UNSAVED)
+      && fs->regs.reg[fs->retaddr_column].how != REG_UNDEFINED)
     {
       /* See comment above about a possibly -ve PC_REGNUM.  If this
          assertion fails, it's a problem with this code and not the
@@ -572,7 +618,7 @@
     }
   else
     {
-      reg = DWARF2_REG_TO_REGNUM (fs->retaddr_column);
+      int reg = DWARF2_REG_TO_REGNUM (fs->retaddr_column);
       if (reg != PC_REGNUM)
 	{
 	  /* See comment above about PC_REGNUM being -ve.  If this
@@ -611,7 +657,9 @@
 
   switch (cache->reg[regnum].how)
     {
-    case REG_UNSAVED:
+    case REG_UNDEFINED:
+      /* If CFI explicitly specified that the value isn't defined,
+	 mark it as optomized away - the value isn't available.  */
       *optimizedp = 1;
       *lvalp = not_lval;
       *addrp = 0;
@@ -636,6 +684,12 @@
              very real posibility that CFA is an offset from some
              other register, having nothing to do with the unwound SP
              value.  */
+	  /* FIXME: cagney/2003-09-05: I think I do now.  GDB was
+	     lumping the two states "unspecified" and "undefined"
+	     together.  Here SP_REGNUM was "unspecified", GCC assuming
+	     that in such a case CFA would be used.  This code should
+	     be deleted - this specific problem now handled as part of
+	     REG_UNSPECIFIED case further down.  */
 	  *optimizedp = 0;
 	  if (valuep)
 	    {
@@ -687,7 +741,52 @@
 	}
       break;
 
-    case REG_UNMODIFIED:
+    case REG_UNSPECIFIED:
+      /* GCC, in its infinite wisdom decided to not provide unwind
+	 information for registers that are "same value".  Since
+	 DWARF2 (3 draft 7) doesn't define such behavior, said
+	 registers are actually undefined (which is different to CFI
+	 "undefined").  Code above issues a complaint about this.
+	 Here just fudge the books, assume GCC, and that the value is
+	 more inner on the stack.  */
+      if (SP_REGNUM >= 0 && regnum == SP_REGNUM)
+	{
+	  /* Can things get worse?  Yep!  One of the registers GCC
+	     forgot to provide unwind information for was the stack
+	     pointer.  Outch!  GCC appears to assumes that the CFA
+	     address can be used - after all it points to the inner
+	     most address of the previous frame before the function
+	     call and that's always the same as the stack pointer on
+	     return, right?  Wrong.  See GCC's i386 STDCALL option for
+	     an ABI that has a different entry and return stack
+	     pointer.  */
+	  /* DWARF V3 Draft 7 p102: Typically, the CFA is defined to
+	     be the value of the stack pointer at the call site in the
+	     previous frame (which may be different from its value on
+	     entry to the current frame).  */
+	  /* DWARF V3 Draft 7 p103: The first column of the rules
+             defines the rule which computes the CFA value; it may be
+             either a register and a signed offset that are added
+             together or a DWARF expression that is evaluated.  */
+	  /* FIXME: cagney/2003-09-05: Complain about this via
+             complaint().  */
+	  *optimizedp = 0;
+	  *lvalp = not_lval;
+	  *addrp = 0;
+	  *realnump = -1;
+	  if (valuep)
+	    /* Store the value.  */
+	    store_typed_address (valuep, builtin_type_void_data_ptr,
+				 cache->cfa);
+	}
+      else
+	/* Assume that the register can be found in the next inner
+           most frame.  */
+	frame_register_unwind (next_frame, regnum,
+			       optimizedp, lvalp, addrp, realnump, valuep);
+      break;
+
+    case REG_SAME_VALUE:
       frame_register_unwind (next_frame, regnum,
 			     optimizedp, lvalp, addrp, realnump, valuep);
       break;