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

[Simon Marchi <simon.marchi@polymtl.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