[RFA,1/3] Initial support for variant parts
Commit Message
This adds some initial support for variant parts to gdbtypes.h. A
variant part is represented as a union. The union has a flag
indicating that it has a discriminant, and information about the
discriminant is attached using the dynamic property system.
2018-02-19 Tom Tromey <tom@tromey.com>
* value.h (value_union_variant): Declare.
* valops.c (value_union_variant): New function.
* gdbtypes.h (TYPE_FLAG_DISCRIMINATED_UNION): New macro.
(struct discriminant_info): New.
(enum dynamic_prop_node_kind) <DYN_PROP_DISCRIMINATED>: New
enumerator.
(struct main_type) <flag_discriminated_union>: New field.
---
gdb/ChangeLog | 10 ++++++++++
gdb/gdbtypes.h | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
gdb/valops.c | 41 +++++++++++++++++++++++++++++++++++++++++
gdb/value.h | 8 ++++++++
4 files changed, 110 insertions(+)
Comments
On Tue, Feb 20, 2018 at 12:06:11PM -0700, Tom Tromey wrote:
> This adds some initial support for variant parts to gdbtypes.h. A
> variant part is represented as a union. The union has a flag
> indicating that it has a discriminant, and information about the
> discriminant is attached using the dynamic property system.
>
> 2018-02-19 Tom Tromey <tom@tromey.com>
>
> * value.h (value_union_variant): Declare.
> * valops.c (value_union_variant): New function.
> * gdbtypes.h (TYPE_FLAG_DISCRIMINATED_UNION): New macro.
> (struct discriminant_info): New.
> (enum dynamic_prop_node_kind) <DYN_PROP_DISCRIMINATED>: New
> enumerator.
> (struct main_type) <flag_discriminated_union>: New field.
OK for me. Some questions and notes below, for discussion, more than
a request to change...
> +struct discriminant_info
> +{
> + /* * The index of the discriminant field. If -1, then this union
> + must have just a single field. */
> +
> + int discriminant_index;
> +
> + /* * The index of the default branch of the union. If -1, then
> + there is no default branch. */
> +
> + int default_index;
> +
> + /* * The discriminant values corresponding to each branch. This has
> + a number of entries equal to the number of fields in this union.
> + If discriminant_index is not -1, then that entry in this array is
> + not used. If default_index is not -1, then that entry in this
> + array is not used. */
> +
> + ULONGEST discriminants[1];
It took me a while to answer to myself why "discriminants[default_index]"
is unused if default_index != 1. IIUC, that's because the default branch
can apply to multiple indices, right?
A small note: We (probably the Ada maintainers) will have to extend
your current approach to discriminant handling because a given branch
may be apply to multiple values of a given discriminant.
Another note for the future: Discriminants in Ada can be any discrete
type (integers, signed or unsigned, enum, etc) or even an access type
(the equivalent of a pointer in C). Thinking of the future, I think we
would change the ULONGEST here into an array of char * containing the
target-side representation of the dicriminant. Or would you suggest
something different?
This would also help with ...
> + if (info->default_index == -1)
> + error (_("Could not find variant corresponding to discriminant %s"),
> + pulongest (discriminant));
... printing the value of the discriminant whose branch we couldn't
find in the error message above.
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
>> + /* * The index of the discriminant field. If -1, then this union
>> + must have just a single field. */
>> +
>> + int discriminant_index;
>> +
>> + /* * The index of the default branch of the union. If -1, then
>> + there is no default branch. */
>> +
>> + int default_index;
>> +
>> + /* * The discriminant values corresponding to each branch. This has
>> + a number of entries equal to the number of fields in this union.
>> + If discriminant_index is not -1, then that entry in this array is
>> + not used. If default_index is not -1, then that entry in this
>> + array is not used. */
>> +
>> + ULONGEST discriminants[1];
Joel> It took me a while to answer to myself why "discriminants[default_index]"
Joel> is unused if default_index != 1. IIUC, that's because the default branch
Joel> can apply to multiple indices, right?
Yes (assuming you meant to write "-1" there). Here's an example showing
what a default case looks like in DWARF:
<4><24e>: Abbrev Number: 12 (DW_TAG_variant)
<5><24f>: Abbrev Number: 13 (DW_TAG_member)
<250> DW_AT_type : <0x262>
<254> DW_AT_alignment : 8
<255> DW_AT_data_member_location: 0
Here, the DW_TAG_variant does not have a DW_AT_discr_value or
DW_AT_discr_list. This means it is the default -- if the discriminator
value does not match one of the other discriminants, then this variant
is assumed.
Joel> A small note: We (probably the Ada maintainers) will have to extend
Joel> your current approach to discriminant handling because a given branch
Joel> may be apply to multiple values of a given discriminant.
Yeah, that's the DW_AT_discr_list case that I mentioned in one of the
messages. I think this should be reasonably easy to add.
Joel> Another note for the future: Discriminants in Ada can be any discrete
Joel> type (integers, signed or unsigned, enum, etc) or even an access type
Joel> (the equivalent of a pointer in C). Thinking of the future, I think we
Joel> would change the ULONGEST here into an array of char * containing the
Joel> target-side representation of the dicriminant. Or would you suggest
Joel> something different?
My first thought is that using the target representation is inconvenient
in the DWARF reader, because with DW_AT_discr_* we just get scalar
values.
It seems to me that using a wide-enough scalar here would be enough for
all the cases you list, provided that sign extension was handled
properly. I'm not 100% sure this does handle sign extension though -- I
can update the test for that.
Tom
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
Tom> It seems to me that using a wide-enough scalar here would be enough for
Tom> all the cases you list, provided that sign extension was handled
Tom> properly. I'm not 100% sure this does handle sign extension though -- I
Tom> can update the test for that.
I did this, and I think my patch is ok here -- but this revealed a
latent bug in unpack_bits_as_long. That function was not doing sign
extension in the case where the field was not a bitfield. This seems
incorrect to me, so I will add another patch to my series to fix this.
Tom
> Joel> Another note for the future: Discriminants in Ada can be any discrete
> Joel> type (integers, signed or unsigned, enum, etc) or even an access type
> Joel> (the equivalent of a pointer in C). Thinking of the future, I think we
> Joel> would change the ULONGEST here into an array of char * containing the
> Joel> target-side representation of the dicriminant. Or would you suggest
> Joel> something different?
>
> My first thought is that using the target representation is inconvenient
> in the DWARF reader, because with DW_AT_discr_* we just get scalar
> values.
> It seems to me that using a wide-enough scalar here would be enough for
> all the cases you list, provided that sign extension was handled
> properly. I'm not 100% sure this does handle sign extension though -- I
> can update the test for that.
Yeah. I re-read the DWARF 5 standard, and indeed, I agree.
(and thanks for cathing a latent bug elsewhere ;-)).
For people like me, a small comment explaining that signed discriminant
values are also handled despite the use of an unsigned type thanks to
proper sign extension might avoid some confusion. Would it be OK to
add something like this?
Thanks Tom!
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
Joel> For people like me, a small comment explaining that signed discriminant
Joel> values are also handled despite the use of an unsigned type thanks to
Joel> proper sign extension might avoid some confusion. Would it be OK to
Joel> add something like this?
Sure, I put a comment in value_union_variant where the discriminant is
computed.
Tom
@@ -1,3 +1,13 @@
+2018-02-19 Tom Tromey <tom@tromey.com>
+
+ * value.h (value_union_variant): Declare.
+ * valops.c (value_union_variant): New function.
+ * gdbtypes.h (TYPE_FLAG_DISCRIMINATED_UNION): New macro.
+ (struct discriminant_info): New.
+ (enum dynamic_prop_node_kind) <DYN_PROP_DISCRIMINATED>: New
+ enumerator.
+ (struct main_type) <flag_discriminated_union>: New field.
+
2018-02-20 Markus Metzger <markus.t.metzger@intel.com>
* gnulib/update-gnulib.sh (IMPORTED_GNULIB_MODULES): Add mkstemp.
@@ -304,6 +304,14 @@ DEF_ENUM_FLAGS_TYPE (enum type_instance_flag_value, type_instance_flags);
#define TYPE_FLAG_ENUM(t) (TYPE_MAIN_TYPE (t)->flag_flag_enum)
+/* * True if this type is a discriminated union type. Only valid for
+ TYPE_CODE_UNION. A discriminated union stores a reference to the
+ discriminant field along with the discriminator values in a dynamic
+ property. */
+
+#define TYPE_FLAG_DISCRIMINATED_UNION(t) \
+ (TYPE_MAIN_TYPE (t)->flag_discriminated_union)
+
/* * Constant type. If this is set, the corresponding type has a
const modifier. */
@@ -373,6 +381,39 @@ DEF_ENUM_FLAGS_TYPE (enum type_instance_flag_value, type_instance_flags);
#define TYPE_ADDRESS_CLASS_ALL(t) (TYPE_INSTANCE_FLAGS(t) \
& TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL)
+/* * Information needed for a discriminated union. A discriminated
+ union is handled somewhat differently from an ordinary union.
+
+ One field is designated as the discriminant. Only one other field
+ is active at a time; which one depends on the value of the
+ discriminant and the data in this structure.
+
+ Additionally, it is possible to have a univariant discriminated
+ union. In this case, the union has just a single field, which is
+ assumed to be the only active variant -- in this case no
+ discriminant is provided. */
+
+struct discriminant_info
+{
+ /* * The index of the discriminant field. If -1, then this union
+ must have just a single field. */
+
+ int discriminant_index;
+
+ /* * The index of the default branch of the union. If -1, then
+ there is no default branch. */
+
+ int default_index;
+
+ /* * The discriminant values corresponding to each branch. This has
+ a number of entries equal to the number of fields in this union.
+ If discriminant_index is not -1, then that entry in this array is
+ not used. If default_index is not -1, then that entry in this
+ array is not used. */
+
+ ULONGEST discriminants[1];
+};
+
enum dynamic_prop_kind
{
PROP_UNDEFINED, /* Not defined. */
@@ -431,6 +472,9 @@ enum dynamic_prop_node_kind
/* A property providing an array's byte stride. */
DYN_PROP_BYTE_STRIDE,
+
+ /* A property holding information about a discriminated union. */
+ DYN_PROP_DISCRIMINATED,
};
/* * List for dynamic type attributes. */
@@ -650,6 +694,13 @@ struct main_type
unsigned int flag_flag_enum : 1;
+ /* * True if this type is a discriminated union type. Only valid
+ for TYPE_CODE_UNION. A discriminated union stores a reference to
+ the discriminant field along with the discriminator values in a
+ dynamic property. */
+
+ unsigned int flag_discriminated_union : 1;
+
/* * A discriminant telling us which field of the type_specific
union is being used for this type, if any. */
@@ -2257,6 +2257,47 @@ value_struct_elt_bitpos (struct value **argp, int bitpos, struct type *ftype,
return NULL;
}
+/* See value.h. */
+
+int
+value_union_variant (struct type *union_type, const gdb_byte *contents)
+{
+ gdb_assert (TYPE_CODE (union_type) == TYPE_CODE_UNION
+ && TYPE_FLAG_DISCRIMINATED_UNION (union_type));
+
+ struct dynamic_prop *discriminant_prop
+ = get_dyn_prop (DYN_PROP_DISCRIMINATED, union_type);
+ gdb_assert (discriminant_prop != nullptr);
+
+ struct discriminant_info *info
+ = (struct discriminant_info *) discriminant_prop->data.baton;
+ gdb_assert (info != nullptr);
+
+ /* If this is a univariant union, just return the sole field. */
+ if (TYPE_NFIELDS (union_type) == 1)
+ return 0;
+ /* This should only happen for univariants, which we already dealt
+ with. */
+ gdb_assert (info->discriminant_index != -1);
+
+ /* Compute the discriminant. */
+ ULONGEST discriminant = unpack_field_as_long (union_type, contents,
+ info->discriminant_index);
+
+ for (int i = 0; i < TYPE_NFIELDS (union_type); ++i)
+ {
+ if (i != info->default_index
+ && i != info->discriminant_index
+ && discriminant == info->discriminants[i])
+ return i;
+ }
+
+ if (info->default_index == -1)
+ error (_("Could not find variant corresponding to discriminant %s"),
+ pulongest (discriminant));
+ return info->default_index;
+}
+
/* Search through the methods of an object (and its bases) to find a
specified method. Return the pointer to the fn_field list FN_LIST of
overloaded instances defined in the source language. If available
@@ -1169,4 +1169,12 @@ extern struct type *result_type_of_xmethod (struct value *method,
extern struct value *call_xmethod (struct value *method,
int argc, struct value **argv);
+/* Given a discriminated union type and some corresponding value
+ contents, this will return the field index of the currently active
+ variant. This will throw an exception if no active variant can be
+ found. */
+
+extern int value_union_variant (struct type *union_type,
+ const gdb_byte *contents);
+
#endif /* !defined (VALUE_H) */