Patchwork [v2,02/10] Change dwarf2_frame_state_reg_info::reg to be std::vector

login
register
mail settings
Submitter Tom Tromey
Date Oct. 2, 2018, 4:44 a.m.
Message ID <20181002044420.17628-3-tom@tromey.com>
Download mbox | patch
Permalink /patch/29610/
State New
Headers show

Comments

Tom Tromey - Oct. 2, 2018, 4:44 a.m.
This changes dwarf2_frame_state_reg_info::reg to be a std::vector.
This avoids undefined behavior in the copy constructor when the
original object does not have any registers.

gdb/ChangeLog
2018-10-01  Tom Tromey  <tom@tromey.com>

	* dwarf2-frame.h (dwarf2_frame_state_reg_info)
	<~dwarf2_frame_state_reg_info>: Update.
	<dwarf2_frame_state_reg_info>: Update.
	<alloc_regs>: Add assertion.  Update.
	<reg>: Now a std::vector.
	<num_regs>: Remove.
	<swap>: Update.
	* dwarf2-frame.c (dwarf2_restore_rule, execute_cfa_program)
	(execute_cfa_program_test, dwarf2_frame_cache): Update.
---
 gdb/ChangeLog      | 12 ++++++++++++
 gdb/dwarf2-frame.c | 28 ++++++++++++++--------------
 gdb/dwarf2-frame.h | 31 +++++++++----------------------
 3 files changed, 35 insertions(+), 36 deletions(-)
Pedro Alves - Oct. 3, 2018, 5:27 p.m.
On 10/02/2018 05:44 AM, Tom Tromey wrote:
> This changes dwarf2_frame_state_reg_info::reg to be a std::vector.
> This avoids undefined behavior in the copy constructor when the
> original object does not have any registers.

I don't have anything to say against the patch, I think it's a nice
cleanup regardless, but could you expand this log a little to point
out exactly what is triggering the undefined behavior.  I guess we're
passing NULL to the memcpy?  LGTM otherwise.

Thanks,
Pedro Alves
Tom Tromey - Oct. 3, 2018, 9:04 p.m.
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> On 10/02/2018 05:44 AM, Tom Tromey wrote:
>> This changes dwarf2_frame_state_reg_info::reg to be a std::vector.
>> This avoids undefined behavior in the copy constructor when the
>> original object does not have any registers.

Pedro> I don't have anything to say against the patch, I think it's a nice
Pedro> cleanup regardless, but could you expand this log a little to point
Pedro> out exactly what is triggering the undefined behavior.  I guess we're
Pedro> passing NULL to the memcpy?

Yes, that was the issue.  Simon asked for this cleanup IIRC.
I've added a note about this to the commit message.

Tom

Patch

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index f7dc820f4d..118bc11217 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -204,14 +204,13 @@  dwarf2_restore_rule (struct gdbarch *gdbarch, ULONGEST reg_num,
 {
   ULONGEST reg;
 
-  gdb_assert (fs->initial.reg);
   reg = dwarf2_frame_adjust_regnum (gdbarch, reg_num, eh_frame_p);
   fs->regs.alloc_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)
+  if (reg < fs->initial.reg.size ())
     fs->regs.reg[reg] = fs->initial.reg[reg];
   else
     fs->regs.reg[reg].how = DWARF2_FRAME_REG_UNSPECIFIED;
@@ -602,7 +601,7 @@  bad CFI data; mismatched DW_CFA_restore_state at %s"),
 	}
     }
 
-  if (fs->initial.reg == NULL)
+  if (fs->initial.reg.empty ())
     {
       /* Don't allow remember/restore between CIE and FDE programs.  */
       delete fs->regs.prev;
@@ -653,12 +652,12 @@  execute_cfa_program_test (struct gdbarch *gdbarch)
   auto r1 = dwarf2_frame_adjust_regnum (gdbarch, 1, fde.eh_frame_p);
   auto r2 = dwarf2_frame_adjust_regnum (gdbarch, 2, fde.eh_frame_p);
 
-  SELF_CHECK (fs.regs.num_regs == (std::max (r1, r2) + 1));
+  SELF_CHECK (fs.regs.reg.size () == (std::max (r1, r2) + 1));
 
   SELF_CHECK (fs.regs.reg[r2].how == DWARF2_FRAME_REG_SAVED_OFFSET);
   SELF_CHECK (fs.regs.reg[r2].loc.offset == -4);
 
-  for (auto i = 0; i < fs.regs.num_regs; i++)
+  for (auto i = 0; i < fs.regs.reg.size (); i++)
     if (i != r2)
       SELF_CHECK (fs.regs.reg[i].how == DWARF2_FRAME_REG_UNSPECIFIED);
 
@@ -1097,7 +1096,7 @@  dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
   {
     int column;		/* CFI speak for "register number".  */
 
-    for (column = 0; column < fs.regs.num_regs; column++)
+    for (column = 0; column < fs.regs.reg.size (); column++)
       {
 	/* Use the GDB register number as the destination index.  */
 	int regnum = dwarf_reg_to_regnum (gdbarch, column);
@@ -1140,8 +1139,9 @@  incomplete CFI data; unspecified registers (e.g., %s) at %s"),
 	if (cache->reg[regnum].how == DWARF2_FRAME_REG_RA
 	    || cache->reg[regnum].how == DWARF2_FRAME_REG_RA_OFFSET)
 	  {
-	    struct dwarf2_frame_state_reg *retaddr_reg =
-	      &fs.regs.reg[fs.retaddr_column];
+	    const std::vector<struct dwarf2_frame_state_reg> &regs
+	      = fs.regs.reg;
+	    ULONGEST retaddr_column = fs.retaddr_column;
 
 	    /* It seems rather bizarre to specify an "empty" column as
                the return adress column.  However, this is exactly
@@ -1150,14 +1150,14 @@  incomplete CFI data; unspecified registers (e.g., %s) at %s"),
                register corresponding to the return address column.
                Incidentally, that's how we should treat a return
                address column specifying "same value" too.  */
-	    if (fs.retaddr_column < fs.regs.num_regs
-		&& retaddr_reg->how != DWARF2_FRAME_REG_UNSPECIFIED
-		&& retaddr_reg->how != DWARF2_FRAME_REG_SAME_VALUE)
+	    if (fs.retaddr_column < fs.regs.reg.size ()
+		&& regs[retaddr_column].how != DWARF2_FRAME_REG_UNSPECIFIED
+		&& regs[retaddr_column].how != DWARF2_FRAME_REG_SAME_VALUE)
 	      {
 		if (cache->reg[regnum].how == DWARF2_FRAME_REG_RA)
-		  cache->reg[regnum] = *retaddr_reg;
+		  cache->reg[regnum] = regs[retaddr_column];
 		else
-		  cache->retaddr_reg = *retaddr_reg;
+		  cache->retaddr_reg = regs[retaddr_column];
 	      }
 	    else
 	      {
@@ -1176,7 +1176,7 @@  incomplete CFI data; unspecified registers (e.g., %s) at %s"),
       }
   }
 
-  if (fs.retaddr_column < fs.regs.num_regs
+  if (fs.retaddr_column < fs.regs.reg.size ()
       && fs.regs.reg[fs.retaddr_column].how == DWARF2_FRAME_REG_UNDEFINED)
     cache->undefined_retaddr = 1;
 
diff --git a/gdb/dwarf2-frame.h b/gdb/dwarf2-frame.h
index 52316e5e16..b89f931651 100644
--- a/gdb/dwarf2-frame.h
+++ b/gdb/dwarf2-frame.h
@@ -98,19 +98,14 @@  struct dwarf2_frame_state_reg_info
   ~dwarf2_frame_state_reg_info ()
   {
     delete prev;
-    xfree (reg);
   }
 
   /* Copy constructor.  */
   dwarf2_frame_state_reg_info (const dwarf2_frame_state_reg_info &src)
-    : num_regs (src.num_regs), cfa_offset (src.cfa_offset),
+    : reg (src.reg), cfa_offset (src.cfa_offset),
       cfa_reg (src.cfa_reg), cfa_how (src.cfa_how), cfa_exp (src.cfa_exp),
       prev (src.prev)
   {
-    size_t size = src.num_regs * sizeof (struct dwarf2_frame_state_reg);
-
-    reg = (struct dwarf2_frame_state_reg *) xmalloc (size);
-    memcpy (reg, src.reg, size);
   }
 
   /* Assignment operator for both move-assignment and copy-assignment.  */
@@ -123,33 +118,26 @@  struct dwarf2_frame_state_reg_info
 
   /* Move constructor.  */
   dwarf2_frame_state_reg_info (dwarf2_frame_state_reg_info &&rhs) noexcept
-    : reg (rhs.reg), num_regs (rhs.num_regs), cfa_offset (rhs.cfa_offset),
+    : reg (std::move (rhs.reg)), cfa_offset (rhs.cfa_offset),
       cfa_reg (rhs.cfa_reg), cfa_how (rhs.cfa_how), cfa_exp (rhs.cfa_exp),
       prev (rhs.prev)
   {
     rhs.prev = nullptr;
-    rhs.reg = nullptr;
   }
 
-/* Assert that the register set RS is large enough to store gdbarch_num_regs
-   columns.  If necessary, enlarge the register set.  */
+  /* If necessary, enlarge the register set to hold NUM_REGS_REQUESTED
+     registers.  */
   void alloc_regs (int num_regs_requested)
   {
-    if (num_regs_requested <= num_regs)
-      return;
+    gdb_assert (num_regs_requested > 0);
 
-    size_t size = sizeof (struct dwarf2_frame_state_reg);
-
-    reg = (struct dwarf2_frame_state_reg *)
-      xrealloc (reg, num_regs_requested * size);
+    if (num_regs_requested <= reg.size ())
+      return;
 
-    /* Initialize newly allocated registers.  */
-    memset (reg + num_regs, 0, (num_regs_requested - num_regs) * size);
-    num_regs = num_regs_requested;
+    reg.resize (num_regs_requested);
   }
 
-  struct dwarf2_frame_state_reg *reg = NULL;
-  int num_regs = 0;
+  std::vector<struct dwarf2_frame_state_reg> reg;
 
   LONGEST cfa_offset = 0;
   ULONGEST cfa_reg = 0;
@@ -166,7 +154,6 @@  private:
     using std::swap;
 
     swap (lhs.reg, rhs.reg);
-    swap (lhs.num_regs, rhs.num_regs);
     swap (lhs.cfa_offset, rhs.cfa_offset);
     swap (lhs.cfa_reg, rhs.cfa_reg);
     swap (lhs.cfa_how, rhs.cfa_how);