Re: [PATCH 12/43] Merge evaluate_for_locexpr_baton evaluator

Zoran Zaric via Gdb-patches
2021-04-28T11:39:27Z

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