Re: [PATCH] Add support for untagged unions

Manish Goregaokar
2016-10-31T03:42:48Z

updated:

From cc3807d12c8cc1e205d8d545eb888be43eb2ec1b Mon Sep 17 00:00:00 2001
From: Manish Goregaokar <manish@mozilla.com>
Date: Fri, 28 Oct 2016 18:00:43 -0700
Subject: [PATCH 2/3] Add support for untagged unions in Rust

2016-10-28 Manish Goregaokar <manish@mozilla.com>

gdb/ChangeLog:
* rust-lang.c (rust_union_is_untagged): Add function to
check if a union is an untagged unioni
(rust_val_print): Handle printing of untagged union values
(rust_print_type): Handle printing of untagged union types
(rust_evaluate_subexp): Handle evaluating field
access on untagged unions
---
gdb/rust-lang.c | 309 ++++++++++++++++++++++++++++++++++----------------------
1 file changed, 189 insertions(+), 120 deletions(-)

diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index 0ced4bb..a9446bc 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -93,6 +93,28 @@ struct disr_info

#define RUST_ENCODED_ENUM_HIDDEN 1

+/* Whether or not a TYPE_CODE_UNION value is an untagged union
+ as opposed to being a regular Rust enum. */
+static bool
+rust_union_is_untagged(struct type *type) {
+ /* Unions must have at least one field. */
+ if (TYPE_NFIELDS (type) == 0)
+ return false;
+ /* If the first field is named, but the name has the rust enum prefix,
+ it is an enum. */
+ if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
+ strlen (RUST_ENUM_PREFIX)) == 0)
+ return false;
+ /* Unions only have named fields. */
+ for (int i = 0; i < TYPE_NFIELDS (type); ++i)
+ {
+ if (strlen (TYPE_FIELD_NAME (type, i)) == 0)
+ return false;
+ }
+ return true;
+
+}
+
/* Utility function to get discriminant info for a given value. */

static struct disr_info
@@ -455,6 +477,81 @@ rust_printstr (struct ui_file *stream, struct type *type,

+static void
+val_print_struct (struct type *type, const gdb_byte *valaddr,
+ int embedded_offset, CORE_ADDR address, struct ui_file *stream,
+ int recurse, const struct value *val,
+ const struct value_print_options *options) {
+ int i;
+ int first_field;
+ int is_tuple = rust_tuple_type_p (type);
+ int is_tuple_struct = !is_tuple && rust_tuple_struct_type_p (type);
+ struct value_print_options opts;
+
+ if (!is_tuple)
+ {
+ if (TYPE_TAG_NAME (type) != NULL)
+ fprintf_filtered (stream, "%s", TYPE_TAG_NAME (type));
+
+ if (TYPE_NFIELDS (type) == 0)
+ return;
+
+ if (TYPE_TAG_NAME (type) != NULL)
+ fputs_filtered (" ", stream);
+ }
+
+ if (is_tuple || is_tuple_struct)
+ fputs_filtered ("(", stream);
+ else
+ fputs_filtered ("{", stream);
+
+ opts = *options;
+ opts.deref_ref = 0;
+
+ first_field = 1;
+ for (i = 0; i < TYPE_NFIELDS (type); ++i)
+ {
+ if (field_is_static (&TYPE_FIELD (type, i)))
+ continue;
+
+ if (!first_field)
+ fputs_filtered (",", stream);
+
+ if (options->prettyformat)
+ {
+ fputs_filtered ("\n", stream);
+ print_spaces_filtered (2 + 2 * recurse, stream);
+ }
+ else if (!first_field)
+ fputs_filtered (" ", stream);
+
+ first_field = 0;
+
+ if (!is_tuple && !is_tuple_struct)
+ {
+ fputs_filtered (TYPE_FIELD_NAME (type, i), stream);
+ fputs_filtered (": ", stream);
+ }
+
+ val_print (TYPE_FIELD_TYPE (type, i),
+ valaddr,
+ embedded_offset + TYPE_FIELD_BITPOS (type, i) / 8,
+ address,
+ stream, recurse + 1, val, &opts,
+ current_language);
+ }
+
+ if (options->prettyformat)
+ {
+ fputs_filtered ("\n", stream);
+ print_spaces_filtered (2 * recurse, stream);
+ }
+
+ if (is_tuple || is_tuple_struct)
+ fputs_filtered (")", stream);
+ else
+ fputs_filtered ("}", stream);
+}
static const struct generic_val_print_decorations rust_decorations =
{
/* Complex isn't used in Rust, but we provide C-ish values just in
@@ -566,6 +663,18 @@ rust_val_print (struct type *type, const gdb_byte
*valaddr, int embedded_offset,
struct value_print_options opts;
struct cleanup *cleanup;

+ /* Untagged unions are printed as if they are structs.
+ Since the field bit positions overlap in the debuginfo,
+ the code for printing a union is same as that for a struct,
+ the only difference is that the input type will have overlapping
+ fields. */
+ if (rust_union_is_untagged (type))
+ {
+ val_print_struct (type, valaddr, embedded_offset, address, stream,
+ recurse, val, options);
+ break;
+ }
+
opts = *options;
opts.deref_ref = 0;

@@ -638,89 +747,84 @@ rust_val_print (struct type *type, const
gdb_byte *valaddr, int embedded_offset,
break;

case TYPE_CODE_STRUCT:
- {
- int i;
- int first_field;
- int is_tuple = rust_tuple_type_p (type);
- int is_tuple_struct = !is_tuple && rust_tuple_struct_type_p (type);
- struct value_print_options opts;
-
- if (!is_tuple)
- {
- if (TYPE_TAG_NAME (type) != NULL)
- fprintf_filtered (stream, "%s", TYPE_TAG_NAME (type));
+ val_print_struct (type, valaddr, embedded_offset, address, stream,
+ recurse, val, options);
+ break;

- if (TYPE_NFIELDS (type) == 0)
- break;
+ default:
+ generic_print:
+ /* Nothing special yet. */
+ generic_val_print (type, valaddr, embedded_offset, address, stream,
+ recurse, val, options, &rust_decorations);
+ }
+}

- if (TYPE_TAG_NAME (type) != NULL)
- fputs_filtered (" ", stream);
- }
+

- if (is_tuple || is_tuple_struct)
- fputs_filtered ("(", stream);
- else
- fputs_filtered ("{", stream);
+void
+rust_print_type (struct type *type, const char *varstring,
+ struct ui_file *stream, int show, int level,
+ const struct type_print_options *flags);

- opts = *options;
- opts.deref_ref = 0;
+/* Print a struct or union typedef. */
+static void
+rust_print_struct_def (struct type *type, const char *varstring,
+ struct ui_file *stream, int show, int level,
+ const struct type_print_options *flags) {
+ int is_tuple_struct, i;

- first_field = 1;
- for (i = 0; i < TYPE_NFIELDS (type); ++i)
+ /* Print a tuple type simply. */
+ if (rust_tuple_type_p (type))
{
- if (field_is_static (&TYPE_FIELD (type, i)))
- continue;
+ fputs_filtered (TYPE_TAG_NAME (type), stream);
+ return;
+ }

- if (!first_field)
- fputs_filtered (",", stream);
+ /* If we see a base class, delegate to C. */
+ if (TYPE_N_BASECLASSES (type) > 0)
+ c_print_type (type, varstring, stream, show, level, flags);

- if (options->prettyformat)
- {
- fputs_filtered ("\n", stream);
- print_spaces_filtered (2 + 2 * recurse, stream);
- }
- else if (!first_field)
- fputs_filtered (" ", stream);
+ /* This code path is also used by unions. */
+ if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
+ fputs_filtered ("struct ", stream);
+ else
+ fputs_filtered ("union ", stream);

- first_field = 0;
+ if (TYPE_TAG_NAME (type) != NULL)
+ fputs_filtered (TYPE_TAG_NAME (type), stream);

- if (!is_tuple && !is_tuple_struct)
- {
- fputs_filtered (TYPE_FIELD_NAME (type, i), stream);
- fputs_filtered (": ", stream);
- }
+ is_tuple_struct = rust_tuple_struct_type_p (type);

- val_print (TYPE_FIELD_TYPE (type, i),
- valaddr,
- embedded_offset + TYPE_FIELD_BITPOS (type, i) / 8,
- address,
- stream, recurse + 1, val, &opts,
- current_language);
- }
+ if (TYPE_NFIELDS (type) == 0 && !rust_tuple_type_p (type))
+ return;
+ fputs_filtered (is_tuple_struct ? " (\n" : " {\n", stream);

- if (options->prettyformat)
+ for (i = 0; i < TYPE_NFIELDS (type); ++i)
{
- fputs_filtered ("\n", stream);
- print_spaces_filtered (2 * recurse, stream);
- }
+ const char *name;

- if (is_tuple || is_tuple_struct)
- fputs_filtered (")", stream);
- else
- fputs_filtered ("}", stream);
- }
- break;
+ QUIT;
+ if (field_is_static (&TYPE_FIELD (type, i)))
+ continue;

- default:
- generic_print:
- /* Nothing special yet. */
- generic_val_print (type, valaddr, embedded_offset, address, stream,
- recurse, val, options, &rust_decorations);
- }
-}
+ /* We'd like to print "pub" here as needed, but rustc
+ doesn't emit the debuginfo, and our types don't have
+ cplus_struct_type attached. */

-
+ /* For a tuple struct we print the type but nothing
+ else. */
+ print_spaces_filtered (level + 2, stream);
+ if (!is_tuple_struct)
+ fprintf_filtered (stream, "%s: ", TYPE_FIELD_NAME (type, i));

+ rust_print_type (TYPE_FIELD_TYPE (type, i), NULL,
+ stream, show - 1, level + 2,
+ flags);
+ fputs_filtered (",\n", stream);
+ }
+
+ fprintfi_filtered (level, stream, is_tuple_struct ? ")" : "}");
+}
/* la_print_typedef implementation for Rust. */

static void
@@ -736,7 +840,7 @@ rust_print_typedef (struct type *type,

/* la_print_type implementation for Rust. */

-static void
+void
rust_print_type (struct type *type, const char *varstring,
struct ui_file *stream, int show, int level,
const struct type_print_options *flags)
@@ -809,56 +913,7 @@ rust_print_type (struct type *type, const char *varstring,
break;

case TYPE_CODE_STRUCT:
- {
- int is_tuple_struct;
-
- /* Print a tuple type simply. */
- if (rust_tuple_type_p (type))
- {
- fputs_filtered (TYPE_TAG_NAME (type), stream);
- break;
- }
-
- /* If we see a base class, delegate to C. */
- if (TYPE_N_BASECLASSES (type) > 0)
- goto c_printer;
-
- fputs_filtered ("struct ", stream);
- if (TYPE_TAG_NAME (type) != NULL)
- fputs_filtered (TYPE_TAG_NAME (type), stream);
-
- is_tuple_struct = rust_tuple_struct_type_p (type);
-
- if (TYPE_NFIELDS (type) == 0 && !rust_tuple_type_p (type))
- break;
- fputs_filtered (is_tuple_struct ? " (\n" : " {\n", stream);
-
- for (i = 0; i < TYPE_NFIELDS (type); ++i)
- {
- const char *name;
-
- QUIT;
- if (field_is_static (&TYPE_FIELD (type, i)))
- continue;
-
- /* We'd like to print "pub" here as needed, but rustc
- doesn't emit the debuginfo, and our types don't have
- cplus_struct_type attached. */
-
- /* For a tuple struct we print the type but nothing
- else. */
- print_spaces_filtered (level + 2, stream);
- if (!is_tuple_struct)
- fprintf_filtered (stream, "%s: ", TYPE_FIELD_NAME (type, i));
-
- rust_print_type (TYPE_FIELD_TYPE (type, i), NULL,
- stream, show - 1, level + 2,
- flags);
- fputs_filtered (",\n", stream);
- }
-
- fprintfi_filtered (level, stream, is_tuple_struct ? ")" : "}");
- }
+ rust_print_struct_def (type, varstring, stream, show, level, flags);
break;

case TYPE_CODE_ENUM:
@@ -899,6 +954,16 @@ rust_print_type (struct type *type, const char *varstring,
/* Skip the discriminant field. */
int skip_to = 1;

+ /* Unions and structs have the same syntax in Rust,
+ the only difference is that structs are declared with `struct`
+ and union with `union`. This difference is handled in the struct
+ printer. */
+ if (rust_union_is_untagged (type))
+ {
+ rust_print_struct_def (type, varstring, stream, show, level, flags);
+ break;
+ }
+
fputs_filtered ("enum ", stream);
if (TYPE_TAG_NAME (type) != NULL)
{
@@ -1637,7 +1702,10 @@ rust_evaluate_subexp (struct type *expect_type,
struct expression *exp,
lhs = evaluate_subexp (NULL_TYPE, exp, pos, noside);

type = value_type (lhs);
- if (TYPE_CODE (type) == TYPE_CODE_UNION)
+ /* Untagged unions can't have anonymous field access since
+ they can only have named fields. */
+ if (TYPE_CODE (type) == TYPE_CODE_UNION
+ && !rust_union_is_untagged (type))
{
struct cleanup *cleanup;

@@ -1712,8 +1780,8 @@ tuple structs, and tuple-like enum variants"));
lhs = evaluate_subexp (NULL_TYPE, exp, pos, noside);

type = value_type (lhs);
-
- if (TYPE_CODE (type) == TYPE_CODE_UNION)
+ if (TYPE_CODE (type) == TYPE_CODE_UNION
+ && !rust_union_is_untagged (type))
{
int i, start;
struct disr_info disr;
@@ -1762,6 +1830,7 @@ which has only anonymous fields"),
}
else
{
+ /* Field access in structs and untagged unions works like C. */
*pos = pc;
result = evaluate_subexp_standard (expect_type, exp, pos, noside);
}
--
2.10.1

-Manish

On Sun, Oct 30, 2016 at 8:12 PM, Tom Tromey <tom@tromey.com> wrote:
>>>>>> "Manish" == Manish Goregaokar <manish@mozilla.com> writes:
>
> Manish> Rust supports untagged unions (C unions) now (using the same
> Manish> syntax as structs but with `union` instead of `struct` in the
> Manish> declaration). These are mainly used for FFI.
>
> Thank you.
>
> Manish> No tests because stable Rust doesn't have these yet. Let me know
> Manish> if I should add them.
>
> I think they're necessary but I want to understand the options.
>
> Is it hard to determine if rustc supports unions?
> Is there some flag that must be passed?
>
> I suppose one idea would be to feature test rustc in the test suite,
> then change the new tests to be unsupported if this fails. And, if a
> flag is needed, pass it -- but go back once the feature hits stable and
> remove the flag?
>
> I think the essentials of the patch are fine.
>
> Manish> * rust-lang.c (rust_union_is_untagged): Add function to
> Manish> check if a union is an untagged unioni
> Manish> * rust-lang.c (rust_val_print): Handle printing of untagged union values
>
> Just the first entry needs the "* FILENAME" bit.
>
> Manish> +/* Whether or not a TYPE_CODE_UNION value is an untagged union
> Manish> + as opposed to being a regular Rust enum. */
> Manish> +static bool
> Manish> +rust_union_is_untagged(struct type *type) {
>
> This has various style issues.
>
> Thanks for using bool.
>
> Manish> + Since the field bit positions overlap in the debuginfo,
> Manish> + the code for printing a union is same as that for a struct,
> Manish> + the only difference is that the input type will have overlapping
> Manish> + fields. */
> Manish> + if (rust_union_is_untagged (type))
> Manish> + goto struct_val;
>
> I think it'd be better to turn the specific case's body into a helper
> function. I'd like to get rid of a lot of these gotos; and I also plan
> to remove as many cleanups as possible from the rust code in gdb...
>
> Manish> + /* This code path is also used by unions. */
> Manish> + if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
> Manish> + fputs_filtered ("struct ", stream);
> Manish> + else
> Manish> + fputs_filtered ("union ", stream);
> Manish> +
> Manish> if (TYPE_TAG_NAME (type) != NULL)
>
> Indentation of that addition looks wrong.
>
> Manish> + /* Field access in structs and untagged unions works like C. */
> Manish> *pos = pc;
>
> Same here.
>
> Tom