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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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