Re: [PATCH] Make dwarf_expr_context::stack an std::vector

[Pedro Alves <palves@redhat.com>]

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