Re: [PATCH] Add support for untagged unions

[Manish Goregaokar <manish at mozilla dot com>]

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