This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [Patch v2 02/10] type: add c99 variable length array support


Thanks for your review.

> Sanimir> +  else if (attr_form_is_ref (attr))
> Sanimir> +    {
> Sanimir> +      struct dwarf2_cu *target_cu = cu;
> Sanimir> +      struct die_info *target_die;
> Sanimir> +      struct attribute *target_attr;
> Sanimir> +      const gdb_byte append_ops[] = { DW_OP_deref };
> 
> I think this won't work in all cases.  For example, suppose the
> reference is to a DIE that has a DWARF expression that evaluates to a
> register.  In this case, you want the value of the register -- but this
> will do something else.
> 
> I appreciate what the deref is trying to do here but I think a different
> approach is probably needed.
> 
> Sanimir> +static int
> Sanimir> +resolve_dynamic_prop (const struct dwarf2_prop *prop, CORE_ADDR address,
> Sanimir> +		      CORE_ADDR *value)
> Sanimir> +{
> Sanimir> +  if (prop == NULL)
> Sanimir> +    return 0;
> Sanimir> +
> Sanimir> +  switch (prop->kind)
> Sanimir> +    {
> Sanimir> +    case DWARF_LOCEXPR:
> Sanimir> +      {
> Sanimir> +	const struct dwarf2_locexpr_baton *baton = prop->data.locexpr;
> Sanimir> +
> Sanimir> +	return dwarf2_locexpr_baton_eval (baton, address, value);
> Sanimir> +      }
> Sanimir> +    case DWARF_CONST:
> Sanimir> +      break;
> Sanimir> +    }
> 
> This is a switch not covering all the cases.
> I think it's fine to use a simple "if" instead.
> Or, if you really want a switch, put the remaining constant in there too.
> 
Pedro run into the above issue(s) as well:

 https://sourceware.org/ml/gdb-patches/2013-11/msg00646.html

I will address this in a separate email/patch. Instead of manually
retrieving DW_AT_location I will re-use the exiting functions dealing with
dwarf locations. This should fix both of your mentioned issues.

> Sanimir> +struct type *
> Sanimir> +resolve_dynamic_type (struct type *type, CORE_ADDR address)
> Sanimir> +{
> [...]
> Sanimir> +  /* Make a deep copy of the type.  */
> Sanimir> +  copied_types = create_copied_types_hash (TYPE_OBJFILE (type));
> Sanimir> +  cleanup = make_cleanup_htab_delete (copied_types);
> Sanimir> +  resolved_type = copy_type_recursive
> Sanimir> +    (TYPE_OBJFILE (type), type, copied_types);
> 
> A couple problems here actually.
> 
> copy_type_recursive is actually not very general purpose.
> 
> It allocates temporary data on the objfile obstack, because it "knows"
> it is only called when the objfile is being destroyed and so some extra
> allocations there are unimportant.  (This is of course fixable.)
> 
> It doesn't copy everything -- it doesn't preserve some C++ data.
> 
> Also it deep copies the things it does copy, so including things like
> field names.
> 
> On the whole this seems pretty expensive.  Don't you just need to copy a
> top-level array type?  If so maybe copy_type would work better.
> 
We need to copy TYPE_CODE_ARRAY, including TYPE_CODE_RANGE. TYPE_CODE_TYPEDEF
needs to be copied as well. Otherwise we modify the bounds in-place. Let me
try to re-create an array with create_array_type/create_range_type
instead to avoid the expensive deep copy.

Below you will find the obvious fixes.

 -Sanimir

> -----Original Message-----
> From: Tom Tromey [mailto:tromey@redhat.com]
> Sent: Friday, November 22, 2013 09:37 PM
> To: Agovic, Sanimir
> Cc: gdb-patches@sourceware.org; Boell, Keven
> Subject: Re: [Patch v2 02/10] type: add c99 variable length array support
> 
> >>>>> "Sanimir" == Sanimir Agovic <sanimir.agovic@intel.com> writes:
> 
> Sanimir> -/* DWARF 2 location expression support for GDB.
> Sanimir> +/* DWARF location expression support for GDB.
> 
> While I agree with this change, it's really unrelated to the patch at
> hand.
> 
Will undo.

> Sanimir> +static int
> Sanimir> +attr_to_dwarf2_prop (const struct attribute *attr, struct die_info *die,
> Sanimir> +		     struct dwarf2_cu *cu, struct dwarf2_prop *prop)
> Sanimir> +{
> Sanimir> +  if (die == NULL || attr == NULL || cu == NULL || prop == NULL)
> Sanimir> +    return 0;
> 
> Can all of these cases actually occur?
> I would guess that at least 'die' and 'cu' can never be NULL.
> 
Left attr/prop check in place, removed die/cu null_ptr checks.

> Sanimir> +static int
> Sanimir> +is_dynamic_type (const struct type *type)
> Sanimir> +{
> Sanimir> +  if (type == NULL)
> Sanimir> +    return 0;
> 
> Can this happen?
> 
Removed null_ptr check.

> Sanimir> +  if (TYPE_CODE (type) == TYPE_CODE_ARRAY
> Sanimir> +      && TYPE_NFIELDS (type) == 1)
> Sanimir> +    {
> Sanimir> +      const struct type *range_type = TYPE_INDEX_TYPE (type);
> 
> Probably should have a check_typedef.
> 
> Sanimir> +  if (TYPE_CODE (type) == TYPE_CODE_PTR
> Sanimir> +      || TYPE_CODE (type) == TYPE_CODE_REF
> Sanimir> +      || TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
> Sanimir> +    return is_dynamic_type (TYPE_TARGET_TYPE (type));
> 
> check_typedef here too.
> 
Added both check_typedefs.

> Also I noticed that the intro comments for resolve_dynamic_prop,
> resolve_dynamic_bounds, and resolve_dynamic_type do not document the
> meaning of the ADDRESS parameter.
> 
Documentation added.

> Sanimir> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> Sanimir> index d01f75e..5500e94 100644
> Sanimir> --- a/gdb/gdbtypes.h
> Sanimir> +++ b/gdb/gdbtypes.h
> Sanimir> @@ -1094,6 +1094,7 @@ extern void allocate_gnat_aux_type (struct type *);
> Sanimir>  #define TYPE_LOW_BOUND_KIND(range_type) \
> Sanimir>    TYPE_RANGE_DATA(range_type)->low.kind
> 
> Sanimir> +
> Sanimir>  /* Moto-specific stuff for FORTRAN arrays.  */
> 
> Random change.
> 
Done.

> Sanimir> +  store_typed_address (value_contents_raw (val), check_typedef (resolved_type),
> addr);
> 
> Line too long.
> 
Done.

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]