Re: [PATCH 03/43] Move frame context info to dwarf_expr_context

[Zoran Zaric via Gdb-patches <gdb-patches@sourceware.org>]

On 4/27/21 3:19 AM, Simon Marchi wrote:
>>
>> This patch starts the merging proccess by moving the frame context
> 
> proccess -> process

Thanks, I will fix it in the next iteration.

> 
>> @@ -56,6 +57,31 @@ dwarf_gdbarch_types_init (struct gdbarch *gdbarch)
>>     return types;
>>   }
>>
>> +/* Ensure that a FRAME is defined, throw an exception otherwise.
>> +
>> +   Throwing NOT_AVAILABLE_ERROR error so that a client can chose
>> +   to react differently if the evaluation ended because there
>> +   was a missing context information.  */
>> +
>> +static void
>> +ensure_have_frame (struct frame_info *frame, const char *op_name)
>> +{
>> +  if (frame == nullptr)
>> +    throw_error (NOT_AVAILABLE_ERROR,
>> +              _("%s evaluation requires a frame."), op_name);
>> +}
> 
> I don't remember if we discussed about that or not, so I'll ask:
> "available" in GDB terminology usually refers to the tracing
> functionality.  While tracing, you can choose which registers and what
> part of the memory to collect when hitting a tracepoint.  During later
> analysis, if you try for example to print the value of a variable that
> requires some information (register or memory) that you didn't collect,
> we'll say that this register or memory (and therefore the variable's
> value) is not available / unavailable.
> 
> It is also used with the "record" command / concept, where some things
> are recorded during execution to be able to step backwards in time,
> that's pretty much the same as tracing.  If you step backwards in time
> and try to access an information that wasn't recorded and an exception
> is thrown as a result, it will be NOT_AVAILABLE_ERROR.
> 
> So I am a bit worried that by using NOT_AVAILABLE_ERROR to say "to
> evaluate this expression, you need a frame, but there is no frame in the
> current context", we are overloading this exception type / return code.
> 
> I can imagine that I could be inspecting a trace, I try to print a
> variable whose location expression needs to be evaluated.  Evaluating
> the location expression requires accessing some memory or register that
> wasn't recorded in my trace, and that results in throwing
> NOT_AVAILABLE_ERROR.  That would be a different error than trying to
> evaluate an expression that requires a frame in a context that doesn't
> have a frame.
> 
> So it seems to me like a new exception type would be desirable.  Maybe
> INSUFFICIENT_CONTEXT_ERROR?

We did discuss this and at the time the feeling was that we want to 
avoid adding new exception types, then I realized that the GENERIC_ERROR 
error doesn't work after all so I chose the error that made the most 
sense and was already expected by the evaluator callers for other scenarios.

I wouldn't be against adding a new exception type if the feeling is that 
it is justifiable. Actually, I would prefer it to make the solution 
cleaner.

> 
> On an unrelated topic, things like ensure_have_frame always worry me
> because I'm scared I'll forget to call it where necessary.  An
> alternative could be to make sure we get the frame through a getter that
> throws if frame == nullptr.
> 
> Basically, a tiny class (could be internal to dwarf_expr_context):
> 
> struct frame_safe
> {
>    frame_safe (frame_info *frame)
>      : m_frame (frame)
>    {}
> 
>    frame_info *frame (const char *op)
>    {
>      if (m_frame == nullptr)
>        ... throw ...
> 
>      return m_frame;
>    }
> 
> private:
>    frame_info *m_frame;
> };
> 
> dwarf_expr_context would have a `frame_safe m_frame` field and a
> `frame (const char *op)` getter that just does
> `return m_frame->frame (op)`.
> 
> This way, it's impossible to get the frame and forgetting an
> ensure_have_frame call.

True, but then you end up with even more "switch" statements in 
different parts of the gdb that one needs to update every time they add 
a new operation and this is one of the problems with the current design 
as is. This would force us to add two new places (one for frame and one 
for compilation unit) and there will be more in the future when we add a 
concept of a thread to the context.

At least with the current implementation the implementer of the case 
statement that process that operation needs to know which part of the 
context has to be present for that operation, which is clearly defined 
in the DWARF standard as part of that operations definition (or at least 
it will be after our extensions).

Another problem with the new class approach is that it makes the 
information about what each operation needs convoluted and hidden which 
in my mind adds even bigger chance of someone forgetting to add their 
operation to the query calls.

I don't think that there is a good way to avoid this problem. People 
that work on the evaluator changes need to pay close attention to what 
that operation requires.

> 
>> @@ -857,7 +928,7 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
>>            if (this->location == DWARF_VALUE_MEMORY)
>>              result = fetch_address (0);
>>            else if (this->location == DWARF_VALUE_REGISTER)
>> -           result = this->read_addr_from_reg (value_as_long (fetch (0)));
>> +           result = read_addr_from_reg (this->frame, value_as_long (fetch (0)));
> 
> This line is slightly too long.

Thanks, I will fix it in the next iteration.

> 
>> @@ -259,8 +247,23 @@ struct dwarf_expr_context
>>     void add_piece (ULONGEST size, ULONGEST offset);
>>     void execute_stack_op (const gdb_byte *op_ptr, const gdb_byte *op_end);
>>     void pop ();
>> +
>> +  /* Return a value of type TYPE, stored in register number REGNUM
>> +     of the frame associated to the given BATON.
> 
> "the given BATON" is a bit confusing, I'm not sure which BATON this
> refers to.  Could you try to improve this comment?


Thanks, I will fix it in the next iteration.

> 
>> +
>> +     REGNUM is a DWARF register number.  */
>> +  struct value *get_reg_value (struct type *type, int regnum);
>> +
>> +  /* Return the location expression for the frame base attribute, in
>> +     START and LENGTH.  The result must be live until the current
>> +     expression evaluation is complete.  */
>> +  void get_frame_base (const gdb_byte **start, size_t *length);
>>   };
>>
>> +/* Return the value of register number REG (a DWARF register number),
>> +   read as an address in a given FRAME.  */
>> +CORE_ADDR read_addr_from_reg (struct frame_info *, int);
> 
> Can you please add the names to the parameters?  It's a bit confusing
> otherwise, since the comment refers to them.


Thanks, I will fix it in the next iteration.

Zoran