This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [Patch v2 02/10] type: add c99 variable length array support
- From: "Agovic, Sanimir" <sanimir dot agovic at intel dot com>
- To: 'Tom Tromey' <tromey at redhat dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, "Boell, Keven" <keven dot boell at intel dot com>
- Date: Mon, 25 Nov 2013 16:05:25 +0000
- Subject: RE: [Patch v2 02/10] type: add c99 variable length array support
- Authentication-results: sourceware.org; auth=none
- References: <1385044223-24650-1-git-send-email-sanimir dot agovic at intel dot com> <1385044223-24650-3-git-send-email-sanimir dot agovic at intel dot com> <87y54g9nri dot fsf at fleche dot redhat dot com>
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