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

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

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