Re: [PATCH 16/43] Simplify dwarf_expr_context class interface

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

>>   /* Create a new context for the expression evaluator.  */
>>
>> -dwarf_expr_context::dwarf_expr_context (dwarf2_per_objfile *per_objfile)
>> -: per_objfile (per_objfile)
>> +dwarf_expr_context::dwarf_expr_context (dwarf2_per_objfile *per_objfile,
>> +                                     int addr_size)
>> +: gdbarch (per_objfile->objfile->arch ()),
> 
> If gdbarch is always obtained from the objfile, we could maybe avoid
> storing it.

Sure, but it is used so many times (often as a few times per operation) 
so we currently cut off two indirection per each use.

I think having one pointer as a class member that cuts through all that 
is worth it.

> 
>> +  addr_size (addr_size),
>> +  per_objfile (per_objfile)
>>   {
>>   }
>>
>> @@ -825,13 +828,17 @@ dwarf_expr_context::read_mem (gdb_byte *buf, CORE_ADDR addr,
>>       return;
>>
>>     /* Prefer the passed-in memory, if it exists.  */
>> -  CORE_ADDR offset = addr - obj_address;
>> +  if (addr_info != nullptr)
>> +    {
>> +      CORE_ADDR offset = addr - addr_info->addr;
>>
>> -  if (offset < data_view.size () && offset + length <= data_view.size ())
>> +      if (offset < addr_info->valaddr.size ()
>> +       && offset + length <= addr_info->valaddr.size ())
>>       {
>> -      memcpy (buf, data_view.data (), length);
>> +      memcpy (buf, addr_info->valaddr.data (), length);
> 
> Not related to this patch, but: I find it odd that the field `valaddr`
> contains the object's value.  Why is it called like that?

My understanding is that this is because it often (although not always) 
represents a pointer to a struct value contents of some object in cases 
where the DWARF spec is not descriptive enough to support the actual case.

This is something that could be replaced by our extensions in the future 
but it would probably require a compiler change.

> 
>>         return;
>>       }
>> +    }
> 
> The indentation here is not right.
> 
>>
>>     read_memory (addr, buf, length);
>>   }
>> @@ -874,8 +881,8 @@ dwarf_expr_context::push_dwarf_reg_entry_value
>>                                                   caller_frame);
>>     scoped_restore save_per_cu = make_scoped_restore (&this->per_cu,
>>                                                    caller_per_cu);
>> -  scoped_restore save_obj_addr = make_scoped_restore (&this->obj_address,
>> -                                                   (CORE_ADDR) 0);
>> +  scoped_restore save_addr_info = make_scoped_restore (&this->addr_info,
>> +                                                    nullptr);
>>     scoped_restore save_per_objfile = make_scoped_restore (&this->per_objfile,
>>                                                         caller_per_objfile);
>>
>> @@ -1043,6 +1050,28 @@ dwarf_expr_context::fetch_result (struct type *type,
>>     return retval;
>>   }
>>
>> +/* See expr.h.  */
>> +
>> +struct value *
>> +dwarf_expr_context::evaluate (const gdb_byte *addr, size_t len,
>> +                           struct dwarf2_per_cu_data *per_cu,
>> +                           struct frame_info *frame,
>> +                           const struct property_addr_info *addr_info,
>> +                           struct type *type,
>> +                           struct type *subobj_type,
>> +                           LONGEST subobj_offset)
>> +{
>> +  this->per_cu = per_cu;
>> +  this->frame = frame;
>> +  this->addr_info = addr_info;
>> +
>> +  if (per_cu != nullptr)
>> +    this->ref_addr_size = per_cu->ref_addr_size ();
> 
> As mentioned in a previous message, I have the feeling that we could get
> rid of the ref_addr_size field and just call per_cu->ref_addr_size when
> we need it.  It could always be a follow-up cleanup.

Agreed.

> 
>> +
>> +  eval (addr, len);
>> +  return fetch_result (type, subobj_type, subobj_offset);
> 
> This is just me, not an officiel style rule, when I like to use `this->`
> when accessing fields and methods, to make it clear what they are (not
> global variables or free functions).  Although for fields prefixed with
> `m_`, I don't, because it's already obvious by the naming.

I was wondering about this because the old code was inconsistent. I will 
probably do like a systematic change for all places in one of the new 
patches.

> 
>> diff --git a/gdb/dwarf2/expr.h b/gdb/dwarf2/expr.h
>> index a0ac21f2ed1..d1374068732 100644
>> --- a/gdb/dwarf2/expr.h
>> +++ b/gdb/dwarf2/expr.h
>> @@ -119,19 +119,30 @@ struct dwarf_stack_value
>>      its current state and its callbacks.  */
>>   struct dwarf_expr_context
>>   {
>> -  dwarf_expr_context (dwarf2_per_objfile *per_objfile);
>> +  /* We should ever only pass in the PER_OBJFILE, while the ADDR_SIZE
>> +     information should be retrievable from there.  The PER_OBJFILE
>> +     contains a pointer to the PER_BFD information anyway and the
>> +     address size information must be the same for the whole BFD.  */
>> +  dwarf_expr_context (struct dwarf2_per_objfile *per_objfile,
>> +                   int addr_size);
> 
> I don't really understand the comment.  What do you mean by "We should
> ever only pass..."?  In general, I don't find that the comment helps me
> understand what the parameters are and do.  Try to change it to be more
> concrete and straight to the point.

This was more me venting about that particular design choice because the 
reality is that an address size has to be consistent with what is 
defined in the original object file but there is no interface to always 
get it in a clean way in gdb.

It makes sense for me to remove that comment altogether in the next 
iteration and thing about a future change that would clean it up.

> 
>>     virtual ~dwarf_expr_context () = default;
>>
>>     void push_address (CORE_ADDR value, bool in_stack_memory);
>> -  void eval (const gdb_byte *addr, size_t len);
>>
>> -  /* Fetch the result of the expression evaluation in a form of
>> -     a struct value, where TYPE, SUBOBJ_TYPE and SUBOBJ_OFFSET
>> -     describe the source level representation of that result.  */
>> -  struct value *fetch_result (struct type *type = nullptr,
>> -                           struct type *subobj_type = nullptr,
>> -                           LONGEST subobj_offset = 0);
>> +  /* Evaluate the expression at ADDR (LEN bytes long) in a given PER_CU
>> +     FRAME context.  Where TYPE, SUBOBJ_TYPE and SUBOBJ_OFFSET describe
>> +     expected struct value representation of the evaluation result.
>> +     The ADDR_INFO property can be specified to override the range of
>> +     memory addresses with the passed in buffer.  */
> 
> It sounds like you are missing a word between PER_CU and FRAME.
> 
> The sentence starting with "Where" sounds strange, I'd remove the
> "Where" to make it a simple statement.
> 
> It sounds like you are missing a "the" between "describe" and
> "expected".

Thank you, I will change it in the next iteration.

> 
>> +  struct value *evaluate (const gdb_byte *addr, size_t len,
>> +                       struct dwarf2_per_cu_data *per_cu,
>> +                       struct frame_info *frame,
>> +                       const struct property_addr_info *addr_info = nullptr,
>> +                       struct type *type = nullptr,
>> +                       struct type *subobj_type = nullptr,
>> +                       LONGEST subobj_offset = 0);
> 
> Note to self that ADDR + LEN would be good candidates to become a
> gdb::array_view later.
> 

Agreed.

>>
>> +private:
>>     /* The stack of values.  */
>>     std::vector<dwarf_stack_value> stack;
> 
> If all these fields become private, it would be good to rename them to
> add the `m_` prefix.  It could be done at the end (if not already done
> later in the series), to avoid the conflicts.
> 
> Simon
> 

I didn't do it, but I will in the next iteration.

Zoran