[PATCH] Fix DW_CFA_restore_extended parsing

[PATCH] Fix DW_CFA_restore_extended parsing

[Luis Machado]

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

Hi folks,

There appears to be a flaw during the execution of this instruction
(DW_CFA_restore_extended). Most of the time the registers are implicitly
defined to use an "unspecified" rule due to the lack of information (or
due to space optimization strategies) in the CIE's initial instructions.

Different from DW_CFA_restore, DW_CFA_restore_extended doesn't check if
the register rule in the current dwarf frame set's list of initialized
registers is valid prior to assigning the rule to it, so it might just
grab junk and fail eventually.

This is hard to reproduce as the extended restore instruction doesn't
show up very often, and you have to be lucky to grab the "wrong" kind of
junk for the rule, leading GDB to an internal error.

This simple patch fixes the issue. Any thoughts? Ok to commit?

Best regards,
-- 
Luis Machado
Software Engineer 
IBM Linux Technology Center

[-- Attachment #2: dwarf-fde-parse.diff --]
[-- Type: text/x-patch, Size: 1059 bytes --]

2007-12-20  Luis Machado  <luisgpm@br.ibm.com>

    * dwarf2-frame.c (execute_cfa_program): Check if a register's rule
    is explicitly defined in the CIE before assignment, else force the
    default rule.

Index: gdb/dwarf2-frame.c
===================================================================
--- gdb.orig/dwarf2-frame.c	2007-10-21 12:33:37.000000000 -0700
+++ gdb/dwarf2-frame.c	2007-12-20 11:19:56.000000000 -0800
@@ -382,7 +382,14 @@
 	      insn_ptr = read_uleb128 (insn_ptr, insn_end, &reg);
 	      reg = dwarf2_frame_adjust_regnum (gdbarch, reg, eh_frame_p);
 	      dwarf2_frame_state_alloc_regs (&fs->regs, reg + 1);
-	      fs->regs.reg[reg] = fs->initial.reg[reg];
+
+              /* Check if this register was explicitly initialized in the
+	      CIE initial instructions.  If not, default the rule to
+	      UNSPECIFIED.  */
+	      if (reg < fs->initial.num_regs)
+	        fs->regs.reg[reg] = fs->initial.reg[reg];
+	      else
+		fs->regs.reg[reg].how = DWARF2_FRAME_REG_UNSPECIFIED;
 	      break;
 
 	    case DW_CFA_undefined:

Re: [PATCH] Fix DW_CFA_restore_extended parsing

[Jim Blandy]

On Dec 20, 2007 12:22 PM, Luis Machado <luisgpm@linux.vnet.ibm.com> wrote:
> This simple patch fixes the issue. Any thoughts? Ok to commit?

The only difference between those two opcodes is in their encoding ---
the action to be performed is identical.  It seems silly to duplicate
the code.  Could you pull the common code out into its own function,
and then have both cases simply parse their arguments and call the
function?

Re: [PATCH] Fix DW_CFA_restore_extended parsing

[Luis Machado]

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


On Thu, 2007-12-20 at 14:03 -0800, Jim Blandy wrote:
> On Dec 20, 2007 12:22 PM, Luis Machado <luisgpm@linux.vnet.ibm.com> wrote:
> > This simple patch fixes the issue. Any thoughts? Ok to commit?
> 
> The only difference between those two opcodes is in their encoding ---
> the action to be performed is identical.  It seems silly to duplicate
> the code.  Could you pull the common code out into its own function,
> and then have both cases simply parse their arguments and call the
> function?

Like so?


-- 
Luis Machado
Software Engineer 
IBM Linux Technology Center

[-- Attachment #2: dwarf-fde-parse.diff --]
[-- Type: text/x-patch, Size: 3088 bytes --]

2007-12-21  Luis Machado  <luisgpm@br.ibm.com>

        * dwarf2-frame.c (execute_cfa_program): Call dwarf2_restore_rule
	function to handle required actions for the DW_CFA_restore and 
	DW_CFA_restore_extended instructions.
	(dwarf2_restore_rule): New function.

Index: gdb/dwarf2-frame.c
===================================================================
--- gdb.orig/dwarf2-frame.c	2007-10-21 12:33:37.000000000 -0700
+++ gdb/dwarf2-frame.c	2007-12-21 05:54:31.000000000 -0800
@@ -268,6 +268,36 @@
 		  _("Support for DW_OP_GNU_push_tls_address is unimplemented"));
 }
 
+/* Execute the required actions for both the DW_CFA_restore and
+DW_CFA_restore_extended instructions.  */
+static void
+dwarf2_restore_rule (struct gdbarch *gdbarch, ULONGEST reg_num,
+		     struct dwarf2_frame_state *fs, int eh_frame_p)
+{
+  ULONGEST reg;
+
+  gdb_assert (fs->initial.reg);
+  reg = dwarf2_frame_adjust_regnum (gdbarch, reg_num, eh_frame_p);
+  dwarf2_frame_state_alloc_regs (&fs->regs, reg + 1);
+
+  /* Check if this register was explicitly initialized in the
+  CIE initial instructions.  If not, default the rule to
+  UNSPECIFIED.  */
+  if (reg < fs->initial.num_regs)
+    fs->regs.reg[reg] = fs->initial.reg[reg];
+  else
+    fs->regs.reg[reg].how = DWARF2_FRAME_REG_UNSPECIFIED;
+
+  if (fs->regs.reg[reg].how == DWARF2_FRAME_REG_UNSPECIFIED)
+    complaint (&symfile_complaints, _("\
+incomplete CFI data; DW_CFA_restore unspecified\n\
+register %s (#%d) at 0x%s"),
+		       gdbarch_register_name
+		       (gdbarch, gdbarch_dwarf2_reg_to_regnum (gdbarch, reg)),
+		       gdbarch_dwarf2_reg_to_regnum (gdbarch, reg),
+		       paddr (fs->pc));
+}
+
 static CORE_ADDR
 execute_stack_op (gdb_byte *exp, ULONGEST len,
 		  struct frame_info *next_frame, CORE_ADDR initial)
@@ -324,23 +354,8 @@
 	}
       else if ((insn & 0xc0) == DW_CFA_restore)
 	{
-	  gdb_assert (fs->initial.reg);
 	  reg = insn & 0x3f;
-	  reg = dwarf2_frame_adjust_regnum (gdbarch, reg, eh_frame_p);
-	  dwarf2_frame_state_alloc_regs (&fs->regs, reg + 1);
-	  if (reg < fs->initial.num_regs)
-	    fs->regs.reg[reg] = fs->initial.reg[reg];
-	  else 
-	    fs->regs.reg[reg].how = DWARF2_FRAME_REG_UNSPECIFIED;
-
-	  if (fs->regs.reg[reg].how == DWARF2_FRAME_REG_UNSPECIFIED)
-	    complaint (&symfile_complaints, _("\
-incomplete CFI data; DW_CFA_restore unspecified\n\
-register %s (#%d) at 0x%s"),
-		       gdbarch_register_name
-			 (gdbarch, gdbarch_dwarf2_reg_to_regnum (gdbarch, reg)),
-		       gdbarch_dwarf2_reg_to_regnum (gdbarch, reg),
-		       paddr (fs->pc));
+	  dwarf2_restore_rule (gdbarch, reg, fs, eh_frame_p);
 	}
       else
 	{
@@ -378,11 +393,8 @@
 	      break;
 
 	    case DW_CFA_restore_extended:
-	      gdb_assert (fs->initial.reg);
 	      insn_ptr = read_uleb128 (insn_ptr, insn_end, &reg);
-	      reg = dwarf2_frame_adjust_regnum (gdbarch, reg, eh_frame_p);
-	      dwarf2_frame_state_alloc_regs (&fs->regs, reg + 1);
-	      fs->regs.reg[reg] = fs->initial.reg[reg];
+	      dwarf2_restore_rule (gdbarch, reg, fs, eh_frame_p);
 	      break;
 
 	    case DW_CFA_undefined:

Re: [PATCH] Fix DW_CFA_restore_extended parsing

[Jim Blandy]

On Dec 21, 2007 5:59 AM, Luis Machado <luisgpm@linux.vnet.ibm.com> wrote:
> On Thu, 2007-12-20 at 14:03 -0800, Jim Blandy wrote:
> > The only difference between those two opcodes is in their encoding ---
> > the action to be performed is identical.  It seems silly to duplicate
> > the code.  Could you pull the common code out into its own function,
> > and then have both cases simply parse their arguments and call the
> > function?
>
> Like so?

Yes --- looks great.  Please commit.  Thanks!

Re: [PATCH] Fix DW_CFA_restore_extended parsing

[Luis Machado]

> > Like so?
> 
> Yes --- looks great.  Please commit.  Thanks!

Thanks for reviewing Jim! I've checked in this version.

Best regards,
-- 
Luis Machado
Software Engineer 
IBM Linux Technology Center