[rfa/6.0] Better handle unspecified CFI values

[rfa/6.0] Better handle unspecified CFI values

[Andrew Cagney]

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

Hello,

This patch is an attempt at improving GDB's behavior when GCC "treads 
the boundaries of the CFI specification".

It does the following:

- changes the rules REG_UNMODIFIED -> REG_SAME_VALUE and REG_UNSAVED -> 
REG_UNDEFINED so that they better match the corresponding CFI register 
states (I could commit this separatly).  The other names confused me :-)

- it adds a new register rule - REG_UNSPECIFIED - which is used to 
differentiate a register that is missing CFI info from a register that 
CFI specified as "undefined" (nee UNSAVED).

- when unwinding, it treats REG_UNSPECIFIED registers like 
REG_SAME_VALUE but with the additional hack to map an unspecified 
SP_REGNUM onto the CFA.

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

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)

- 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?
Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 9243 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.

Index: dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.12
diff -u -r1.12 dwarf2-frame.c
--- dwarf2-frame.c	26 Aug 2003 03:07:29 -0000	1.12
+++ dwarf2-frame.c	6 Sep 2003 00:15:28 -0000
@@ -97,6 +97,18 @@
 
 /* Structure describing a frame state.  */
 
+enum dwarf2_reg_rule
+{
+  /* Initializing using MEMSET.  Make certain that 0 maps onto the
+     correct enum value.  */
+  REG_UNSPECIFIED = 0,
+  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 +123,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 +360,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 +466,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 +540,64 @@
       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 cfinum;
+    for (cfinum = 0; cfinum < fs->regs.num_regs; cfinum++)
+      {
+	int regnum;
+	
+	/* Skip the return address column.  */
+	if (cfinum == 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 the destination index.  */
+	regnum = DWARF2_REG_TO_REGNUM (cfinum);
+
+	/* 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 - should the CFI fully specify a
+	   contigious subset of registers this check won't detect a
+	   problem.  Is an upper bound on the number of debug info
+	   registers available?  */
+	if (fs->regs.reg[cfinum].how == REG_UNSPECIFIED)
+	  complaint (&symfile_complaints,
+		     "Incomplete CFI data; unspecified registers at 0x%s",
+		     paddr (fs->pc));
+
+	cache->reg[regnum] = fs->regs.reg[cfinum];
+      }
+  }
 
   /* 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 +607,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 +646,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 +673,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 +730,53 @@
 	}
       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;

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

[Daniel Jacobowitz]

On Fri, Sep 05, 2003 at 08:57:57PM -0400, Andrew Cagney wrote:
> Hello,
> 
> This patch is an attempt at improving GDB's behavior when GCC "treads 
> the boundaries of the CFI specification".
> 
> It does the following:
> 
> - changes the rules REG_UNMODIFIED -> REG_SAME_VALUE and REG_UNSAVED -> 
> REG_UNDEFINED so that they better match the corresponding CFI register 
> states (I could commit this separatly).  The other names confused me :-)
> 
> - it adds a new register rule - REG_UNSPECIFIED - which is used to 
> differentiate a register that is missing CFI info from a register that 
> CFI specified as "undefined" (nee UNSAVED).
> 
> - when unwinding, it treats REG_UNSPECIFIED registers like 
> REG_SAME_VALUE but with the additional hack to map an unspecified 
> SP_REGNUM onto the CFA.
> 
> - 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).
> 
> 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)

Leaving the hack in REG_UNSPECIFIED?  Yes, I'm pretty sure you're
right.

> - add a check/complaint for the SP v CFA problem.

Could you hold off on the complaint until there's a valid way to
specify the SP in the unwind information?  Right now there isn't one,
as I described on the dwarf2 list three weeks ago.

Otherwise this looks good to me.

> @@ -611,7 +646,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.  */

"optimized"

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

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

[Mark Kettenis]

Andrew Cagney <ac131313@redhat.com> writes:

> This is a multi-part message in MIME format.
> --------------000900050201080102070403
> Content-Type: text/plain; charset=us-ascii; format=flowed
> Content-Transfer-Encoding: 7bit
> 
> Hello,
> 
> This patch is an attempt at improving GDB's behavior when GCC "treads 
> the boundaries of the CFI specification".
> 
> It does the following:
> 
> - changes the rules REG_UNMODIFIED -> REG_SAME_VALUE and REG_UNSAVED -> 
> REG_UNDEFINED so that they better match the corresponding CFI register 
> states (I could commit this separatly).  The other names confused me :-)

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?

> 
> - it adds a new register rule - REG_UNSPECIFIED - which is used to 
> differentiate a register that is missing CFI info from a register that 
> CFI specified as "undefined" (nee UNSAVED).

Great!

> 
> - when unwinding, it treats REG_UNSPECIFIED registers like 
> REG_SAME_VALUE but with the additional hack to map an unspecified 
> SP_REGNUM onto the CFA.

OK!

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

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.

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

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

Mark

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

[Richard Henderson]

On Fri, Sep 05, 2003 at 08:57:57PM -0400, Andrew Cagney wrote:
> +	  /* 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?

Yes, gcc does assume the CFA can be used, except when there is
a CFI entry for the stack pointer.  See s390 for this case.

> 	     Wrong.  See GCC's i386 STDCALL option for
> +	     an ABI that has a different entry and return stack

Indeed, this is a problem.  Please file a gcc pr for this.  We
should be generating an entry for the stack pointer in this case.

We probably don't see the bug in gcc's unwinding for exception
handling because we also apply the fixup for DW_CFA_GNU_args_size,
since we're not unwinding to the call site exactly, but rather to
the handler within the function.


r~

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

[Andrew Cagney]

> On Fri, Sep 05, 2003 at 08:57:57PM -0400, Andrew Cagney wrote:
> 

>> - delete the SP_REGNUM hack from the REG_UNDEFINED rule (it's no longer 
>> needed, I think)
> 
> 
> Leaving the hack in REG_UNSPECIFIED?  Yes, I'm pretty sure you're
> right.

Yes, leaving the hack in REG_UNSPECIFIED - I know that one's needed :-)

>> - add a check/complaint for the SP v CFA problem.
> 
> 
> Could you hold off on the complaint until there's a valid way to
> specify the SP in the unwind information?  Right now there isn't one,
> as I described on the dwarf2 list three weeks ago.

Arrrrgh.  So "sp" should be specified as the same value as the "cfa" 
register?

> Otherwise this looks good to me.

m'kay

>> @@ -611,7 +646,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.  */
> 
> 
> "optimized"

Oops fixed (contrary to the patch I just posted).

Andrew

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

[Andrew Cagney]

[-- 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;

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

[Daniel Jacobowitz]

On Mon, Sep 08, 2003 at 08:12:23PM -0400, Andrew Cagney wrote:
> >On Fri, Sep 05, 2003 at 08:57:57PM -0400, Andrew Cagney wrote:
> >
> 
> >>- delete the SP_REGNUM hack from the REG_UNDEFINED rule (it's no longer 
> >>needed, I think)
> >
> >
> >Leaving the hack in REG_UNSPECIFIED?  Yes, I'm pretty sure you're
> >right.
> 
> Yes, leaving the hack in REG_UNSPECIFIED - I know that one's needed :-)
> 
> >>- add a check/complaint for the SP v CFA problem.
> >
> >
> >Could you hold off on the complaint until there's a valid way to
> >specify the SP in the unwind information?  Right now there isn't one,
> >as I described on the dwarf2 list three weeks ago.
> 
> Arrrrgh.  So "sp" should be specified as the same value as the "cfa" 
> register?

Yes - normally.  On S/390, stdcall, et cetera (anywhere where the hack
would be wrong) it gets even worse.  We can only compute expressions
describing a memory location where the register is saved, not computed
values.  For stack pointers (and maybe frame pointers on some
architectures?) this isn't good enough.

Thanks for addressing this!

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

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

[Jim Blandy]

Daniel Jacobowitz <drow@mvista.com> writes:
> Yes - normally.  On S/390, stdcall, et cetera (anywhere where the hack
> would be wrong) it gets even worse.  We can only compute expressions
> describing a memory location where the register is saved, not computed
> values.  For stack pointers (and maybe frame pointers on some
> architectures?) this isn't good enough.

Not to pursue unimportant tangents, but why would the hack be wrong on
the S/390?  Its frames are normally FP-free, but aside from that,
what's unusual about it?

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

[Daniel Jacobowitz]

On Tue, Sep 09, 2003 at 12:23:27PM -0500, Jim Blandy wrote:
> 
> Daniel Jacobowitz <drow@mvista.com> writes:
> > Yes - normally.  On S/390, stdcall, et cetera (anywhere where the hack
> > would be wrong) it gets even worse.  We can only compute expressions
> > describing a memory location where the register is saved, not computed
> > values.  For stack pointers (and maybe frame pointers on some
> > architectures?) this isn't good enough.
> 
> Not to pursue unimportant tangents, but why would the hack be wrong on
> the S/390?  Its frames are normally FP-free, but aside from that,
> what's unusual about it?

I'm wrong about the S/390 being the problem - in fact it was the
_opposite_ of the problem.  From:

  http://gcc.gnu.org/ml/gcc-patches/2003-05/msg00904.html

    From the previous discussions, it would appear to me that
    the real difference between s390 and other platforms here
    is that on s390, the stack pointer register is saved and
    restored from the call stack frame, like any other call-
    saved register.  This means that uw_install_context can
    install the target stack pointer simply by writing it
    into the current stack frame, just like any other call-
    saved register.

    If this is possible, there is no need for the unwinder
    routines to perform any special handling w.r.t. the
    stack pointer register at all.  This means that there
    should be no need to define EH_RETURN_STACKADJ_RTX,
    and everything related to this should be omitted by
    the library.  This also means that the special-cased
    code for 'simulating' the stack pointer register in
    uw_update_context (which is what caused the breakage
    on s390) is just superfluous.

GCC has a local hack in it, essentially, which handles adjusting the
stack pointer for a frame back far enough.  At least it seems that way
to me.  This hack is equivalent to the problem that GDB has for
figuring out the saved SP.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

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

[Richard Henderson]

On Mon, Sep 08, 2003 at 11:30:53PM -0400, Daniel Jacobowitz wrote:
> We can only compute expressions
> describing a memory location where the register is saved, not computed
> values.

False.  See DW_CFA_expression.


r~

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

[Daniel Jacobowitz]

On Wed, Sep 10, 2003 at 12:47:54PM -0700, Richard Henderson wrote:
> On Mon, Sep 08, 2003 at 11:30:53PM -0400, Daniel Jacobowitz wrote:
> > We can only compute expressions
> > describing a memory location where the register is saved, not computed
> > values.
> 
> False.  See DW_CFA_expression.

True.  See DW_CFA_expression.

20. DW_CFA_expression

    The DW_CFA_expression instruction takes two operands: an unsigned
    LEB128 value representing a register number, and a DW_FORM_block
    value representing a DWARF expression. The required action is to
    establish the DWARF expression as the means by which the address in
    which the given register contents are found may be computed. The
    value of the CFA is pushed on the DWARF evaluation stack prior to
    execution of the DWARF expression.

    The DW_OP_call2, DW_OP_call4, DW_OP_call_ref and
    DW_OP_push_object_address DWARF operators (see Section 2.4.1)
    cannot be used in such a DWARF expression.

It computes "the address in which the given register contents are
found".  That's not ambiguous, and that's precisely how GCC uses it:
          _Unwind_SetGRPtr (context, i, (void *) val);


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

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

[Richard Henderson]

On Wed, Sep 10, 2003 at 03:53:06PM -0400, Daniel Jacobowitz wrote:
> It computes "the address in which the given register contents are
> found".  That's not ambiguous, and that's precisely how GCC uses it:
>           _Unwind_SetGRPtr (context, i, (void *) val);

Huh.  I coulda sworn there was a way to do values.  Oh well.


r~