Re: [PATCH 05/43] Move compilation unit info to dwarf_expr_context

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

>> +static void
>> +ensure_have_per_cu (struct dwarf2_per_cu_data *per_cu, const char* op_name)
>> +{
>> +  if (per_cu == nullptr)
>> +    throw_error (NOT_AVAILABLE_ERROR,
>> +              _("%s evaluation requires a compilation unit."), op_name);
>> +}
> 
> Same comment as earlier about NOT_AVAILABLE_ERROR, and about wrapping
> per_cu to force accessing it through a getter that throws if it is
> nullptr (whatever we end up doing for the frame should be done here).

Same comment as for the frame side. I am not convinced that this is a 
better approach for the reasons mentioned in my previous response.

> 
>> +
>>   /* See expr.h.  */
>>
>>   CORE_ADDR
>> @@ -201,6 +215,25 @@ dwarf_expr_context::get_frame_base (const gdb_byte **start,
>>                                   start, length);
>>   }
>>
>> +/* See expr.h.  */
>> +
>> +struct type *
>> +dwarf_expr_context::get_base_type (cu_offset die_cu_off, int size)
>> +{
>> +  if (per_cu == nullptr)
>> +    return builtin_type (this->gdbarch)->builtin_int;
> 
> Ok, so this seems to be a case where we need to check if per_cu is
> nullptr or not.  The getter that throws won't work here, but there could
> be an `has_per_cu ()` method on the side.

As I said, this approach can make things convoluted and difficult for 
someone to extend in the future. How are they supposed to know if they 
should use has_per_cu method or the getter method?

The ensure_* functions are not part of the class interface so they are 
only used where the standard requires them to be used. All other places 
just use the class member in the current design.

> 
>> +
>> +  struct type *result = dwarf2_get_die_type (die_cu_off, per_cu, per_objfile);
>> +
>> +  if (result == NULL)
>> +    error (_("Could not find type for DW_OP_const_type"));
> 
> Might as well change this NULL for nullptr.

Thanks, I will change it in the next iteration.
> 
>> +
>> +  if (size != 0 && TYPE_LENGTH (result) != size)
>> +    error (_("DW_OP_const_type has different sizes for type and data"));
>> +
>> +  return result;
>> +}
> 
> I find get_base_type a bit awkward.  Do you think you could split it in
> two:
> 
>   - get_base_type
>   - get_base_type_check_size
> 
> ... where get_base_type_check_size uses get_base_type?  The callers that
> don't need the size checking wouldn't need to pass 0.
> 
> I also find it awkward that get_base_type hardcodes DW_OP_const_type in
> its error message, given that it's used in the context of many other
> operators.  In the end we understand that DW_OP_const_type is the only
> case that passes size != 0, so it's the only case where that error can
> possibly be thrown.  But if you could pass the op name as a parameter,
> like you did for the ensure_have_per_cu function, I think it would make
> things better.
> 
> Or, since that size checking is only used when handling
> DW_OP_const_type, we could in-line that code in the caller and not
> bother with having get_base_type_check_size.

To be honest, this is not part of my implementation. I just copied it 
from the loc.c file and inlined one existing functions into another 
during the merging process.

Considering that it is mostly used by the GNU specific operations 
(therefore orthogonal to my implementation), and that there is no 
official spec that defines those operations, I am reluctant to change 
that part of the code.

It was already there previously so it shouldn't be a problem to keep it 
as is after my patch series.

> 
>> @@ -834,10 +873,7 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
>>        case DW_OP_GNU_implicit_pointer:
>>          {
>>            int64_t len;
>> -
>> -         if (this->ref_addr_size == -1)
>> -           error (_("DWARF-2 expression error: DW_OP_implicit_pointer "
>> -                    "is not allowed in frame context"));
>> +         ensure_have_per_cu (per_cu, "DW_OP_implicit_pointer");
>>
>>            /* The referred-to DIE of sect_offset kind.  */
>>            this->len = extract_unsigned_integer (op_ptr, this->ref_addr_size,
> 
> This case would be a bit weird with my suggestion of per_cu getter that
> throws if per_cu is nullptr, because nothing actually uses per_cu here.
> But, I think that in the end we could get rid of the
> `this->ref_addr_size` field, and use `this->per_cu ()->ref_addr_size
> ()`.
> 
> Simon
> 

Yes, this would be one benefit of the class approach. But nothing really 
prevent us to always use the per_cu object, considering that all the 
places that use the ref_addr_size information called the 
ensure_have_per_cu function before that.

There are many places in gdb that currently use the same logic for the 
validity of the passed in frame information. Some don't even do that 
much, but that is an issue for some other discussion.

I am still not convinced that this is a better way to go. Both 
approaches have downsides I guess...

Zoran