diff mbox

Make dwarf_expr_context::stack an std::vector

Message ID 1505404760-11844-1-git-send-email-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi Sept. 14, 2017, 3:59 p.m. UTC 
  Replace the manually managed array with a vector.  It is mostly
straightforward, except maybe one thing in execute_stack_op, in the
handling of DW_OP_fbreg.  When the code stumbles on that opcode while
evaluating an expression, it needs to evaluate a subexpression to find
where the fb reg has been saved.  Rather than creating a new context, it
reuses the current context.  It saves the size of the stack before and
restores the stack to that size after.

I think we can do a little bit better by saving the current stack
locally and installing a new empty stack.  This way, if the
subexpression is malformed and underflows, we'll get an exception.
Before, it would have overwitten the top elements of the top-level
expression.  The evaluation of the top-level expression would have then
resumed with the same stack size, but possibly some corrupted elements.

Regtested on the buildbot.

gdb/ChangeLog:

	* dwarf2expr.h (dwarf_stack_value): Add constructor.
	(dwarf_expr_context) <~dwarf_expr_context>: Define as default.
	<stack>: Change type to std::vector.
	<stack_len, stack_allocated>: Remove.
	<grow_stack>: Remove.
	* dwarf2expr.c (dwarf_expr_context::dwarf_expr_context): Adjust.
	(dwarf_expr_context::~dwarf_expr_context): Remove.
	(dwarf_expr_context::grow_stack): Remove.
	(dwarf_expr_context::push): Adjust.
	(dwarf_expr_context::pop): Adjust.
	(dwarf_expr_context::fetch): Adjust.
	(dwarf_expr_context::fetch_in_stack_memory): Adjust.
	(dwarf_expr_context::stack_empty_p): Adjust.
	(dwarf_expr_context::execute_stack_op): Adjust.
---
 gdb/dwarf2expr.c | 110 ++++++++++++++++++++-----------------------------------
 gdb/dwarf2expr.h |  13 +++----
 2 files changed, 46 insertions(+), 77 deletions(-)

Comments

Pedro Alves Sept. 14, 2017, 4:13 p.m. UTC | #1 
On 09/14/2017 04:59 PM, Simon Marchi wrote:
> Replace the manually managed array with a vector.  It is mostly
> straightforward, except maybe one thing in execute_stack_op, in the
> handling of DW_OP_fbreg.  When the code stumbles on that opcode while
> evaluating an expression, it needs to evaluate a subexpression to find
> where the fb reg has been saved.  Rather than creating a new context, it
> reuses the current context.  It saves the size of the stack before and
> restores the stack to that size after.
> 
> I think we can do a little bit better by saving the current stack
> locally and installing a new empty stack.  This way, if the
> subexpression is malformed and underflows, we'll get an exception.
> Before, it would have overwitten the top elements of the top-level

"overwritten"

> expression.  The evaluation of the top-level expression would have then
> resumed with the same stack size, but possibly some corrupted elements.

One difference this causes is that before we're reuse the
vector's internal memory buffer for the recursion, which may have
been reallocated sufficiently already and not require any further reallocation,
while with the patch, we must always heap-allocate the new vector's
internal buffer when recursion pushes a value, and release it when recursion
unwinds.  I assume that it doesn't cause an observable timing
difference, but, mentioning for completeness.

A couple nits and this is fine with me.

>    /* True if the piece is in memory and is known to be on the program's stack.
> @@ -114,7 +118,7 @@ struct dwarf_stack_value
>  struct dwarf_expr_context
>  {
>    dwarf_expr_context ();
> -  virtual ~dwarf_expr_context ();
> +  virtual ~dwarf_expr_context () = default;

Couldn't we just remove the dtor?

>  
>    void push_address (CORE_ADDR value, bool in_stack_memory);
>    void eval (const gdb_byte *addr, size_t len);
> @@ -123,11 +127,7 @@ struct dwarf_expr_context
>    bool fetch_in_stack_memory (int n);
>  
>    /* The stack of values, allocated with xmalloc.  */

"xmalloc" reference is stale.

Thanks,
Pedro Alves
Simon Marchi Sept. 14, 2017, 4:31 p.m. UTC | #2 
On 2017-09-14 18:13, Pedro Alves wrote:
> On 09/14/2017 04:59 PM, Simon Marchi wrote:
>> Replace the manually managed array with a vector.  It is mostly
>> straightforward, except maybe one thing in execute_stack_op, in the
>> handling of DW_OP_fbreg.  When the code stumbles on that opcode while
>> evaluating an expression, it needs to evaluate a subexpression to find
>> where the fb reg has been saved.  Rather than creating a new context, 
>> it
>> reuses the current context.  It saves the size of the stack before and
>> restores the stack to that size after.
>> 
>> I think we can do a little bit better by saving the current stack
>> locally and installing a new empty stack.  This way, if the
>> subexpression is malformed and underflows, we'll get an exception.
>> Before, it would have overwitten the top elements of the top-level
> 
> "overwritten"

Done.

>> expression.  The evaluation of the top-level expression would have 
>> then
>> resumed with the same stack size, but possibly some corrupted 
>> elements.
> 
> One difference this causes is that before we're reuse the
> vector's internal memory buffer for the recursion, which may have
> been reallocated sufficiently already and not require any further 
> reallocation,
> while with the patch, we must always heap-allocate the new vector's
> internal buffer when recursion pushes a value, and release it when 
> recursion
> unwinds.  I assume that it doesn't cause an observable timing
> difference, but, mentioning for completeness.

Good point.  Though in this case, my opinion is that the correctness and 
safety is well-worth the cost.

> A couple nits and this is fine with me.
> 
>>    /* True if the piece is in memory and is known to be on the 
>> program's stack.
>> @@ -114,7 +118,7 @@ struct dwarf_stack_value
>>  struct dwarf_expr_context
>>  {
>>    dwarf_expr_context ();
>> -  virtual ~dwarf_expr_context ();
>> +  virtual ~dwarf_expr_context () = default;
> 
> Couldn't we just remove the dtor?

I thought that this was necessary because there are subclasses that 
might have destructors.  But now that I think about it more, I suppose 
it would only be relevant if we destroyed instances of subclasses 
through pointers to dwarf_expr_context?  All the instances of these 
classes are static.

>> 
>>    void push_address (CORE_ADDR value, bool in_stack_memory);
>>    void eval (const gdb_byte *addr, size_t len);
>> @@ -123,11 +127,7 @@ struct dwarf_expr_context
>>    bool fetch_in_stack_memory (int n);
>> 
>>    /* The stack of values, allocated with xmalloc.  */
> 
> "xmalloc" reference is stale.

Ok.

Thanks,

Simon
Pedro Alves Sept. 14, 2017, 4:43 p.m. UTC | #3 
On 09/14/2017 05:31 PM, Simon Marchi wrote:

>> A couple nits and this is fine with me.
>>
>>>    /* True if the piece is in memory and is known to be on the
>>> program's stack.
>>> @@ -114,7 +118,7 @@ struct dwarf_stack_value
>>>  struct dwarf_expr_context
>>>  {
>>>    dwarf_expr_context ();
>>> -  virtual ~dwarf_expr_context ();
>>> +  virtual ~dwarf_expr_context () = default;
>>
>> Couldn't we just remove the dtor?
> 
> I thought that this was necessary because there are subclasses that
> might have destructors.  But now that I think about it more, I suppose
> it would only be relevant if we destroyed instances of subclasses
> through pointers to dwarf_expr_context?  All the instances of these
> classes are static.

I actually didn't realize _this_ was the base class.  :-P
But yeah, if we never want to delete via base pointer, the dtor
could be non-virtual, and in which case it should be made
protected, to ensure that really no code outside subclasses tries
to call it directly.  (And in that case, you'd still use =default;)
Really not a big deal, I'm fine with what you had.

> 
>>>
>>>    void push_address (CORE_ADDR value, bool in_stack_memory);
>>>    void eval (const gdb_byte *addr, size_t len);
>>> @@ -123,11 +127,7 @@ struct dwarf_expr_context
>>>    bool fetch_in_stack_memory (int n);
>>>
>>>    /* The stack of values, allocated with xmalloc.  */
>>
>> "xmalloc" reference is stale.
> 
> Ok.
> 

Thanks,
Pedro Alves
Simon Marchi Sept. 14, 2017, 8:39 p.m. UTC | #4 
On 2017-09-14 18:43, Pedro Alves wrote:
> On 09/14/2017 05:31 PM, Simon Marchi wrote:
> 
>>> A couple nits and this is fine with me.
>>> 
>>>>    /* True if the piece is in memory and is known to be on the
>>>> program's stack.
>>>> @@ -114,7 +118,7 @@ struct dwarf_stack_value
>>>>  struct dwarf_expr_context
>>>>  {
>>>>    dwarf_expr_context ();
>>>> -  virtual ~dwarf_expr_context ();
>>>> +  virtual ~dwarf_expr_context () = default;
>>> 
>>> Couldn't we just remove the dtor?
>> 
>> I thought that this was necessary because there are subclasses that
>> might have destructors.  But now that I think about it more, I suppose
>> it would only be relevant if we destroyed instances of subclasses
>> through pointers to dwarf_expr_context?  All the instances of these
>> classes are static.
> 
> I actually didn't realize _this_ was the base class.  :-P
> But yeah, if we never want to delete via base pointer, the dtor
> could be non-virtual, and in which case it should be made
> protected, to ensure that really no code outside subclasses tries
> to call it directly.  (And in that case, you'd still use =default;)
> Really not a big deal, I'm fine with what you had.

Ok, well I've pushed and left the virtual dtor there just in case it's 
ever needed.  I don't think it changes anything at runtime.

Thanks,

Simon
diff mbox

Patch

diff --git a/gdb/dwarf2expr.c b/gdb/dwarf2expr.c
index 3a388ac..f5e0e4c 100644
--- a/gdb/dwarf2expr.c
+++ b/gdb/dwarf2expr.c
@@ -88,10 +88,7 @@  dwarf_expr_context::address_type () const
 /* Create a new context for the expression evaluator.  */
 
 dwarf_expr_context::dwarf_expr_context ()
-: stack (NULL),
-  stack_len (0),
-  stack_allocated (10),
-  gdbarch (NULL),
+: gdbarch (NULL),
   addr_size (0),
   ref_addr_size (0),
   offset (0),
@@ -102,29 +99,6 @@  dwarf_expr_context::dwarf_expr_context ()
   data (NULL),
   initialized (0)
 {
-  this->stack = XNEWVEC (struct dwarf_stack_value, this->stack_allocated);
-}
-
-/* Clean up a dwarf_expr_context.  */
-
-dwarf_expr_context::~dwarf_expr_context ()
-{
-  xfree (this->stack);
-}
-
-/* Expand the memory allocated stack to contain at least
-   NEED more elements than are currently used.  */
-
-void
-dwarf_expr_context::grow_stack (size_t need)
-{
-  if (this->stack_len + need > this->stack_allocated)
-    {
-      size_t newlen = this->stack_len + need + 10;
-
-      this->stack = XRESIZEVEC (struct dwarf_stack_value, this->stack, newlen);
-      this->stack_allocated = newlen;
-    }
 }
 
 /* Push VALUE onto the stack.  */
@@ -132,12 +106,7 @@  dwarf_expr_context::grow_stack (size_t need)
 void
 dwarf_expr_context::push (struct value *value, bool in_stack_memory)
 {
-  struct dwarf_stack_value *v;
-
-  grow_stack (1);
-  v = &this->stack[this->stack_len++];
-  v->value = value;
-  v->in_stack_memory = in_stack_memory;
+  stack.emplace_back (value, in_stack_memory);
 }
 
 /* Push VALUE onto the stack.  */
@@ -153,9 +122,10 @@  dwarf_expr_context::push_address (CORE_ADDR value, bool in_stack_memory)
 void
 dwarf_expr_context::pop ()
 {
-  if (this->stack_len <= 0)
+  if (stack.empty ())
     error (_("dwarf expression stack underflow"));
-  this->stack_len--;
+
+  stack.pop_back ();
 }
 
 /* Retrieve the N'th item on the stack.  */
@@ -163,11 +133,11 @@  dwarf_expr_context::pop ()
 struct value *
 dwarf_expr_context::fetch (int n)
 {
-  if (this->stack_len <= n)
+  if (stack.size () <= n)
      error (_("Asked for position %d of stack, "
-	      "stack only has %d elements on it."),
-	    n, this->stack_len);
-  return this->stack[this->stack_len - (1 + n)].value;
+	      "stack only has %zu elements on it."),
+	    n, stack.size ());
+  return stack[stack.size () - (1 + n)].value;
 }
 
 /* Require that TYPE be an integral type; throw an exception if not.  */
@@ -263,11 +233,11 @@  dwarf_expr_context::fetch_address (int n)
 bool
 dwarf_expr_context::fetch_in_stack_memory (int n)
 {
-  if (this->stack_len <= n)
+  if (stack.size () <= n)
      error (_("Asked for position %d of stack, "
-	      "stack only has %d elements on it."),
-	    n, this->stack_len);
-  return this->stack[this->stack_len - (1 + n)].in_stack_memory;
+	      "stack only has %zu elements on it."),
+	    n, stack.size ());
+  return stack[stack.size () - (1 + n)].in_stack_memory;
 }
 
 /* Return true if the expression stack is empty.  */
@@ -275,7 +245,7 @@  dwarf_expr_context::fetch_in_stack_memory (int n)
 bool
 dwarf_expr_context::stack_empty_p () const
 {
-  return this->stack_len == 0;
+  return stack.empty ();
 }
 
 /* Add a new piece to the dwarf_expr_context's piece list.  */
@@ -875,14 +845,16 @@  dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
 	  {
 	    const gdb_byte *datastart;
 	    size_t datalen;
-	    unsigned int before_stack_len;
 
 	    op_ptr = safe_read_sleb128 (op_ptr, op_end, &offset);
+
 	    /* Rather than create a whole new context, we simply
-	       record the stack length before execution, then reset it
-	       afterwards, effectively erasing whatever the recursive
-	       call put there.  */
-	    before_stack_len = this->stack_len;
+	       backup the current stack locally and install a new empty stack,
+	       then reset it afterwards, effectively erasing whatever the
+	       recursive call put there.  */
+	    std::vector<dwarf_stack_value> saved_stack = std::move (stack);
+	    stack.clear ();
+
 	    /* FIXME: cagney/2003-03-26: This code should be using
                get_frame_base_address(), and then implement a dwarf2
                specific this_base method.  */
@@ -898,7 +870,10 @@  dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
 	    result = result + offset;
 	    result_val = value_from_ulongest (address_type, result);
 	    in_stack_memory = true;
-	    this->stack_len = before_stack_len;
+
+	    /* Restore the content of the original stack.  */
+	    stack = std::move (saved_stack);
+
 	    this->location = DWARF_VALUE_MEMORY;
 	  }
 	  break;
@@ -920,16 +895,14 @@  dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
 	  
 	case DW_OP_swap:
 	  {
-	    struct dwarf_stack_value t1, t2;
-
-	    if (this->stack_len < 2)
+	    if (stack.size () < 2)
 	       error (_("Not enough elements for "
-			"DW_OP_swap.  Need 2, have %d."),
-		      this->stack_len);
-	    t1 = this->stack[this->stack_len - 1];
-	    t2 = this->stack[this->stack_len - 2];
-	    this->stack[this->stack_len - 1] = t2;
-	    this->stack[this->stack_len - 2] = t1;
+			"DW_OP_swap.  Need 2, have %zu."),
+		      stack.size ());
+
+	    dwarf_stack_value &t1 = stack[stack.size () - 1];
+	    dwarf_stack_value &t2 = stack[stack.size () - 2];
+	    std::swap (t1, t2);
 	    goto no_push;
 	  }
 
@@ -940,18 +913,15 @@  dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
 
 	case DW_OP_rot:
 	  {
-	    struct dwarf_stack_value t1, t2, t3;
-
-	    if (this->stack_len < 3)
+	    if (stack.size () < 3)
 	       error (_("Not enough elements for "
-			"DW_OP_rot.  Need 3, have %d."),
-		      this->stack_len);
-	    t1 = this->stack[this->stack_len - 1];
-	    t2 = this->stack[this->stack_len - 2];
-	    t3 = this->stack[this->stack_len - 3];
-	    this->stack[this->stack_len - 1] = t2;
-	    this->stack[this->stack_len - 2] = t3;
-	    this->stack[this->stack_len - 3] = t1;
+			"DW_OP_rot.  Need 3, have %zu."),
+		      stack.size ());
+
+	    dwarf_stack_value temp = stack[stack.size () - 1];
+	    stack[stack.size () - 1] = stack[stack.size () - 2];
+	    stack[stack.size () - 2] = stack[stack.size () - 3];
+	    stack[stack.size () - 3] = temp;
 	    goto no_push;
 	  }
 
diff --git a/gdb/dwarf2expr.h b/gdb/dwarf2expr.h
index 0a57bee..94056ae 100644
--- a/gdb/dwarf2expr.h
+++ b/gdb/dwarf2expr.h
@@ -100,6 +100,10 @@  struct dwarf_expr_piece
 
 struct dwarf_stack_value
 {
+  dwarf_stack_value (struct value *value_, int in_stack_memory_)
+  : value (value_), in_stack_memory (in_stack_memory_)
+  {}
+
   struct value *value;
 
   /* True if the piece is in memory and is known to be on the program's stack.
@@ -114,7 +118,7 @@  struct dwarf_stack_value
 struct dwarf_expr_context
 {
   dwarf_expr_context ();
-  virtual ~dwarf_expr_context ();
+  virtual ~dwarf_expr_context () = default;
 
   void push_address (CORE_ADDR value, bool in_stack_memory);
   void eval (const gdb_byte *addr, size_t len);
@@ -123,11 +127,7 @@  struct dwarf_expr_context
   bool fetch_in_stack_memory (int n);
 
   /* The stack of values, allocated with xmalloc.  */
-  struct dwarf_stack_value *stack;
-
-  /* The number of values currently pushed on the stack, and the
-     number of elements allocated to the stack.  */
-  int stack_len, stack_allocated;
+  std::vector<dwarf_stack_value> stack;
 
   /* Target architecture to use for address operations.  */
   struct gdbarch *gdbarch;
@@ -249,7 +249,6 @@  struct dwarf_expr_context
 private:
 
   struct type *address_type () const;
-  void grow_stack (size_t need);
   void push (struct value *value, bool in_stack_memory);
   bool stack_empty_p () const;
   void add_piece (ULONGEST size, ULONGEST offset);