Re: [PATCH 12/43] Merge evaluate_for_locexpr_baton evaluator

[Zoran Zaric <Zoran.Zaric@amd.com>]

On 4/28/21 2:33 AM, Simon Marchi wrote:
> 
> On 2021-03-01 9:45 a.m., Zoran Zaric via Gdb-patches wrote:
>> From: Zoran Zaric <zoran.zaric@amd.com>
>>
>> The evaluate_for_locexpr_baton is the last derived class from the
>> dwarf_expr_context class. It's purpose is to support the passed in
>> buffer functionality.
>>
>> Although, it is not really necessary to merge this class with it's
>> base class, doing that simplifies new expression evaluator design.
>>
>> Considering that this functionality is going around the DWARF standard,
>> it is also reasonable to expect that with a new evaluator design and
>> extending the push object address functionality to accept any location
>> description, there will be no need to support passed in buffers.
>>
>> Alternatively, it would also makes sense to abstract the interaction
>> between the evaluator and a given resource in the near future. The
>> passed in buffer would then be a specialization of that abstraction.
>>
>> gdb/ChangeLog:
>>
>>        * dwarf2/expr.c (dwarf_expr_context::read_mem): Merge with
>>        evaluate_for_locexpr_baton implementation.
>>        * dwarf2/loc.c (class evaluate_for_locexpr_baton): Remove
>>        class.
>>        (evaluate_for_locexpr_baton::read_mem): Move to
>>        dwarf_expr_context.
>>        (dwarf2_locexpr_baton_eval): Instantiate dwarf_expr_context
>>        instead of evaluate_for_locexpr_baton class.
>> ---
>>   gdb/dwarf2/expr.c | 19 +++++++++++++++++--
>>   gdb/dwarf2/expr.h | 13 +++----------
>>   gdb/dwarf2/loc.c  | 45 ++-------------------------------------------
>>   3 files changed, 22 insertions(+), 55 deletions(-)
>>
>> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
>> index 1212be9c984..fe6d072ac8f 100644
>> --- a/gdb/dwarf2/expr.c
>> +++ b/gdb/dwarf2/expr.c
>> @@ -252,6 +252,18 @@ void
>>   dwarf_expr_context::read_mem (gdb_byte *buf, CORE_ADDR addr,
>>                              size_t length)
>>   {
>> +  if (length == 0)
>> +    return;
> 
> I'm really curious about when `length == 0` happens, but I see it was
> there previously, so this ok.  But if you know the answer, I want to
> know.

I am curious about that as well. I might remove it to check if something 
fails at one point.

> 
>> @@ -239,6 +231,7 @@ struct dwarf_expr_context
>>     void push_dwarf_reg_entry_value (enum call_site_parameter_kind kind,
>>                                   union call_site_parameter_u kind_u,
>>                                   int deref_size);
>> +  void read_mem (gdb_byte *buf, CORE_ADDR addr, size_t length);
> 
> You could keep the comment that was there previously over the read_mem
> method signature.
> 
> Simon
> 

Thanks, I was sure that I've fixed that already...

Zoran