Re: [PATCH 15/43] Make DWARF evaluator return a single struct value

Zoran Zaric via Gdb-patches
2021-04-28T11:48:02Z

On 4/28/21 3:21 AM, Simon Marchi wrote:
> [CAUTION: External Email]
>
>> @@ -886,6 +887,162 @@ dwarf_expr_context::push_dwarf_reg_entry_value
>> this->eval (data_src, size);
>> }
>>
>> +/* See expr.h. */
>> +
>> +struct value *
>> +dwarf_expr_context::fetch_result (struct type *type,
>> + struct type *subobj_type,
>> + LONGEST subobj_offset)
>> +{
>> + struct value *retval = nullptr;
>> +
>> + if (type == nullptr)
>> + type = address_type ();
>> +
>> + if (subobj_type == nullptr)
>> + subobj_type = type;
>> +
>> + if (this->pieces.size () > 0)
>> + {
>> + struct piece_closure *c;
>
> You can move this to where it's first initialized (and drop the struct
> keyword).
>
>> + ULONGEST bit_size = 0;
>> +
>> + for (dwarf_expr_piece &piece : this->pieces)
>> + bit_size += piece.size;
>> + /* Complain if the expression is larger than the size of the
>> + outer type. */
>> + if (bit_size > 8 * TYPE_LENGTH (type))
>> + invalid_synthetic_pointer ();
>> +
>> + c = allocate_piece_closure (this->per_cu, this->per_objfile,
>> + std::move (this->pieces), this->frame);
>> + retval = allocate_computed_value (subobj_type,
>> + &pieced_value_funcs, c);
>> + set_value_offset (retval, subobj_offset);
>> + }
>> + else
>> + {
>> + switch (this->location)
>> + {
>> + case DWARF_VALUE_REGISTER:
>> + {
>> + int dwarf_regnum
>> + = longest_to_int (value_as_long (this->fetch (0)));
>> + int gdb_regnum = dwarf_reg_to_regnum_or_error (this->gdbarch,
>> + dwarf_regnum);
>> +
>> + if (subobj_offset != 0)
>> + error (_("cannot use offset on synthetic pointer to register"));
>> +
>> + gdb_assert (this->frame != NULL);
>> +
>> + retval = value_from_register (subobj_type, gdb_regnum,
>> + this->frame);
>> + if (value_optimized_out (retval))
>> + {
>> + struct value *tmp;
>> +
>> + /* This means the register has undefined value / was
>> + not saved. As we're computing the location of some
>> + variable etc. in the program, not a value for
>> + inspecting a register ($pc, $sp, etc.), return a
>> + generic optimized out value instead, so that we show
>> + <optimized out> instead of <not saved>. */
>> + tmp = allocate_value (subobj_type);
>> + value_contents_copy (tmp, 0, retval, 0,
>> + TYPE_LENGTH (subobj_type));
>> + retval = tmp;
>> + }
>> + }
>> + break;
>> +
>> + case DWARF_VALUE_MEMORY:
>> + {
>> + struct type *ptr_type;
>> + CORE_ADDR address = this->fetch_address (0);
>> + bool in_stack_memory = this->fetch_in_stack_memory (0);
>> +
>> + /* DW_OP_deref_size (and possibly other operations too) may
>> + create a pointer instead of an address. Ideally, the
>> + pointer to address conversion would be performed as part
>> + of those operations, but the type of the object to
>> + which the address refers is not known at the time of
>> + the operation. Therefore, we do the conversion here
>> + since the type is readily available. */
>> +
>> + switch (subobj_type->code ())
>> + {
>> + case TYPE_CODE_FUNC:
>> + case TYPE_CODE_METHOD:
>> + ptr_type = builtin_type (this->gdbarch)->builtin_func_ptr;
>> + break;
>> + default:
>> + ptr_type = builtin_type (this->gdbarch)->builtin_data_ptr;
>> + break;
>> + }
>> + address = value_as_address (value_from_pointer (ptr_type, address));
>> +
>> + retval = value_at_lazy (subobj_type,
>> + address + subobj_offset);
>> + if (in_stack_memory)
>> + set_value_stack (retval, 1);
>> + }
>> + break;
>> +
>> + case DWARF_VALUE_STACK:
>> + {
>> + struct value *value = this->fetch (0);
>> + size_t n = TYPE_LENGTH (value_type (value));
>> + size_t len = TYPE_LENGTH (subobj_type);
>> + size_t max = TYPE_LENGTH (type);
>> +
>> + if (subobj_offset + len > max)
>> + invalid_synthetic_pointer ();
>> +
>> + retval = allocate_value (subobj_type);
>> +
>> + /* The given offset is relative to the actual object. */
>> + if (gdbarch_byte_order (this->gdbarch) == BFD_ENDIAN_BIG)
>> + subobj_offset += n - max;
>> +
>> + memcpy (value_contents_raw (retval),
>> + value_contents_all (value) + subobj_offset, len);
>> + }
>> + break;
>> +
>> + case DWARF_VALUE_LITERAL:
>> + {
>> + bfd_byte *contents;
>
> Same here, declare where initialized.
>
>> + size_t n = TYPE_LENGTH (subobj_type);
>> +
>> + if (subobj_offset + n > this->len)
>> + invalid_synthetic_pointer ();
>> +
>> + retval = allocate_value (subobj_type);
>> + contents = value_contents_raw (retval);
>> + memcpy (contents, this->data + subobj_offset, n);
>> + }
>> + break;
>> +
>> + case DWARF_VALUE_OPTIMIZED_OUT:
>> + retval = allocate_optimized_out_value (subobj_type);
>> + break;
>> +
>> + /* DWARF_VALUE_IMPLICIT_POINTER was converted to a pieced
>> + operation by execute_stack_op. */
>> + case DWARF_VALUE_IMPLICIT_POINTER:
>> + /* DWARF_VALUE_OPTIMIZED_OUT can't occur in this context --
>> + it can only be encountered when making a piece. */
>> + default:
>> + internal_error (__FILE__, __LINE__, _("invalid location type"));
>
> You could change this to use gdb_assert_not_reached.
>
>> @@ -1479,155 +1481,15 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
>> throw;
>> }
>>
>> - if (ctx.pieces.size () > 0)
>> - {
>> - struct piece_closure *c;
>> - ULONGEST bit_size = 0;
>> -
>> - for (dwarf_expr_piece &piece : ctx.pieces)
>> - bit_size += piece.size;
>> - /* Complain if the expression is larger than the size of the
>> - outer type. */
>> - if (bit_size > 8 * TYPE_LENGTH (type))
>> - invalid_synthetic_pointer ();
>> -
>> - c = allocate_piece_closure (per_cu, per_objfile, std::move (ctx.pieces),
>> - frame);
>> - /* We must clean up the value chain after creating the piece
>> - closure but before allocating the result. */
>> - free_values.free_to_mark ();
>> - retval = allocate_computed_value (subobj_type,
>> - &pieced_value_funcs, c);
>> - set_value_offset (retval, subobj_byte_offset);
>> - }
>> - else
>> - {
>> - switch (ctx.location)
>> - {
>> - case DWARF_VALUE_REGISTER:
>> - {
>> - struct gdbarch *arch = get_frame_arch (frame);
>> - int dwarf_regnum
>> - = longest_to_int (value_as_long (ctx.fetch (0)));
>> - int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, dwarf_regnum);
>> -
>> - if (subobj_byte_offset != 0)
>> - error (_("cannot use offset on synthetic pointer to register"));
>> - free_values.free_to_mark ();
>> - retval = value_from_register (subobj_type, gdb_regnum, frame);
>> - if (value_optimized_out (retval))
>> - {
>> - struct value *tmp;
>> -
>> - /* This means the register has undefined value / was
>> - not saved. As we're computing the location of some
>> - variable etc. in the program, not a value for
>> - inspecting a register ($pc, $sp, etc.), return a
>> - generic optimized out value instead, so that we show
>> - <optimized out> instead of <not saved>. */
>> - tmp = allocate_value (subobj_type);
>> - value_contents_copy (tmp, 0, retval, 0,
>> - TYPE_LENGTH (subobj_type));
>> - retval = tmp;
>> - }
>> - }
>> - break;
>> -
>> - case DWARF_VALUE_MEMORY:
>> - {
>> - struct type *ptr_type;
>> - CORE_ADDR address = ctx.fetch_address (0);
>> - bool in_stack_memory = ctx.fetch_in_stack_memory (0);
>> -
>> - /* DW_OP_deref_size (and possibly other operations too) may
>> - create a pointer instead of an address. Ideally, the
>> - pointer to address conversion would be performed as part
>> - of those operations, but the type of the object to
>> - which the address refers is not known at the time of
>> - the operation. Therefore, we do the conversion here
>> - since the type is readily available. */
>> -
>> - switch (subobj_type->code ())
>> - {
>> - case TYPE_CODE_FUNC:
>> - case TYPE_CODE_METHOD:
>> - ptr_type = builtin_type (ctx.gdbarch)->builtin_func_ptr;
>> - break;
>> - default:
>> - ptr_type = builtin_type (ctx.gdbarch)->builtin_data_ptr;
>> - break;
>> - }
>> - address = value_as_address (value_from_pointer (ptr_type, address));
>> -
>> - free_values.free_to_mark ();
>> - retval = value_at_lazy (subobj_type,
>> - address + subobj_byte_offset);
>> - if (in_stack_memory)
>> - set_value_stack (retval, 1);
>> - }
>> - break;
>> -
>> - case DWARF_VALUE_STACK:
>> - {
>> - struct value *value = ctx.fetch (0);
>> - size_t n = TYPE_LENGTH (value_type (value));
>> - size_t len = TYPE_LENGTH (subobj_type);
>> - size_t max = TYPE_LENGTH (type);
>> - gdbarch *objfile_gdbarch = per_objfile->objfile->arch ();
>> -
>> - if (subobj_byte_offset + len > max)
>> - invalid_synthetic_pointer ();
>> -
>> - /* Preserve VALUE because we are going to free values back
>> - to the mark, but we still need the value contents
>> - below. */
>> - value_ref_ptr value_holder = value_ref_ptr::new_reference (value);
>> - free_values.free_to_mark ();
>> -
>> - retval = allocate_value (subobj_type);
>> -
>> - /* The given offset is relative to the actual object. */
>> - if (gdbarch_byte_order (objfile_gdbarch) == BFD_ENDIAN_BIG)
>> - subobj_byte_offset += n - max;
>> -
>> - memcpy (value_contents_raw (retval),
>> - value_contents_all (value) + subobj_byte_offset, len);
>> - }
>> - break;
>> -
>> - case DWARF_VALUE_LITERAL:
>> - {
>> - bfd_byte *contents;
>> - size_t n = TYPE_LENGTH (subobj_type);
>> -
>> - if (subobj_byte_offset + n > ctx.len)
>> - invalid_synthetic_pointer ();
>> -
>> - free_values.free_to_mark ();
>> - retval = allocate_value (subobj_type);
>> - contents = value_contents_raw (retval);
>> - memcpy (contents, ctx.data + subobj_byte_offset, n);
>> - }
>> - break;
>> -
>> - case DWARF_VALUE_OPTIMIZED_OUT:
>> - free_values.free_to_mark ();
>> - retval = allocate_optimized_out_value (subobj_type);
>> - break;
>> -
>> - /* DWARF_VALUE_IMPLICIT_POINTER was converted to a pieced
>> - operation by execute_stack_op. */
>> - case DWARF_VALUE_IMPLICIT_POINTER:
>> - /* DWARF_VALUE_OPTIMIZED_OUT can't occur in this context --
>> - it can only be encountered when making a piece. */
>> - default:
>> - internal_error (__FILE__, __LINE__, _("invalid location type"));
>> - }
>> - }

I've just copied this whole block from the loc.c. Everything is replaced
with a new implementation in the later patch.

Do you still feel that I need to clean it up here or it doesn't really
matter considering the later patches?

>> -
>> - set_value_initialized (retval, ctx.initialized);
>> + /* We need to clean up all the values that are not needed any more.
>> + The problem with a value_ref_ptr class is that it disconnects the
>> + RETVAL from the value garbage collection, so we need to make
>> + a copy of that value on the stack to keep everything consistent.
>> + The value_ref_ptr will clean up after itself at the end of this block. */
>> + value_ref_ptr value_holder = value_ref_ptr::new_reference (retval);
>> + free_values.free_to_mark ();
>>
>> - return retval;
>> + return value_copy(retval);
>
> Missing space.
>
> Simon
>

Thanks, I will fix it in the next iteration.

Zoran