[RFA 0/3] Discriminated unions

[RFA 0/3] Discriminated unions

[Tom Tromey]

This series provides a minimal implementation of DW_TAG_variant and
DW_TAG_variant_part reading for gdb.  It also converts the Rust
language support to use this new approach.  The Rust changes are
motivated by some work to change how the Rust compiler emits enums;
see:

    https://github.com/rust-lang/rust/issues/32920

A DW_TAG_variant is represented in gdb as a union with some extra
information about how to compute the discriminant and thus find the
active variant part.

I have intentionally omitted a few things you might expect to find in
this series:

* Support for DW_AT_discr_list.  This was not needed by Rust.

* Support for multiple members in a single variant.  This was also not
  needed by Rust.

* Special handling in c-varobj.c and c-valprint.c.  These constructs
  can't appear in C, and I think it's useful to be able to "set lang c"
  and see the underlying bits.

I think these holes can be filled in later, as needed, without too
much difficulty.

Regression tested by the buildbot; but I also ran the DWARF and Rust
tests locally.

Tom

[RFA 2/3] Convert Rust to use discriminated unions

[Tom Tromey]

A Rust enum is, essentially, a discriminated union.  Currently the
Rust language support handles Rust enums locally, in rust-lang.c.
However, because I am changing the Rust compiler to use
DW_TAG_variant* to represent enums, it seemed better to have a single
internal representation for Rust enums in gdb.

This patch implements this idea by moving the current Rust enum
handling code to dwarf2read.  This allows the simplification of some
parts of rust-lang.c as well.

2018-02-19  Tom Tromey  <tom@tromey.com>

	* rust-lang.h (rust_last_path_segment): Declare.
	* rust-lang.c (rust_last_path_segment): Now public.  Change
	contract.
	(struct disr_info): Remove.
	(RUST_ENUM_PREFIX, RUST_ENCODED_ENUM_REAL)
	(RUST_ENCODED_ENUM_HIDDEN, rust_union_is_untagged)
	(rust_get_disr_info, rust_tuple_variant_type_p): Remove.
	(rust_enum_p, rust_enum_variant): New function.
	(rust_underscore_fields): Remove "offset" parameter.
	(rust_print_enum): New function.
	(rust_val_print) <TYPE_CODE_UNION>: Remove enum code.
	<TYPE_CODE_STRUCT>: Call rust_print_enum when appropriate.
	(rust_print_struct_def): Add "for_rust_enum" parameter.  Handle
	enums.
	(rust_internal_print_type): New function, from rust_print_type.
	Remove enum code.
	(rust_print_type): Call rust_internal_print_type.
	(rust_evaluate_subexp) <STRUCTOP_ANONYMOUS, STRUCTOP_STRUCT>:
	Update enum handling.
	* dwarf2read.c (struct dwarf2_cu) <rust_unions>: New field.
	(rust_fully_qualify, alloc_discriminant_info, quirk_rust_enum)
	(rust_union_quirks): New functions.
	(process_full_comp_unit, process_full_type_unit): Call
	rust_union_quirks.
	(process_structure_scope): Update rust_unions if necessary.

2018-02-19  Tom Tromey  <tom@tromey.com>

	* gdb.rust/simple.exp: Accept more possible results in enum test.
---
 gdb/ChangeLog                     |  28 ++
 gdb/dwarf2read.c                  | 314 +++++++++++++++
 gdb/rust-lang.c                   | 794 ++++++++++++++------------------------
 gdb/rust-lang.h                   |   5 +
 gdb/testsuite/ChangeLog           |   4 +
 gdb/testsuite/gdb.rust/simple.exp |   4 +-
 6 files changed, 639 insertions(+), 510 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 28990a4ec6..b4c34e0e54 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,33 @@
 2018-02-19  Tom Tromey  <tom@tromey.com>
 
+	* rust-lang.h (rust_last_path_segment): Declare.
+	* rust-lang.c (rust_last_path_segment): Now public.  Change
+	contract.
+	(struct disr_info): Remove.
+	(RUST_ENUM_PREFIX, RUST_ENCODED_ENUM_REAL)
+	(RUST_ENCODED_ENUM_HIDDEN, rust_union_is_untagged)
+	(rust_get_disr_info, rust_tuple_variant_type_p): Remove.
+	(rust_enum_p, rust_enum_variant): New function.
+	(rust_underscore_fields): Remove "offset" parameter.
+	(rust_print_enum): New function.
+	(rust_val_print) <TYPE_CODE_UNION>: Remove enum code.
+	<TYPE_CODE_STRUCT>: Call rust_print_enum when appropriate.
+	(rust_print_struct_def): Add "for_rust_enum" parameter.  Handle
+	enums.
+	(rust_internal_print_type): New function, from rust_print_type.
+	Remove enum code.
+	(rust_print_type): Call rust_internal_print_type.
+	(rust_evaluate_subexp) <STRUCTOP_ANONYMOUS, STRUCTOP_STRUCT>:
+	Update enum handling.
+	* dwarf2read.c (struct dwarf2_cu) <rust_unions>: New field.
+	(rust_fully_qualify, alloc_discriminant_info, quirk_rust_enum)
+	(rust_union_quirks): New functions.
+	(process_full_comp_unit, process_full_type_unit): Call
+	rust_union_quirks.
+	(process_structure_scope): Update rust_unions if necessary.
+
+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.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 6cdb96335f..12ed4ae33c 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -86,6 +86,7 @@
 #include <cmath>
 #include <set>
 #include <forward_list>
+#include "rust-lang.h"
 
 /* When == 1, print basic high level tracing messages.
    When > 1, be more verbose.
@@ -762,6 +763,14 @@ struct dwarf2_cu
      whether the DW_AT_ranges attribute came from the skeleton or DWO.  */
   ULONGEST ranges_base = 0;
 
+  /* When reading debug info generated by older versions of rustc, we
+     have to rewrite some union types to be struct types with a
+     variant part.  This rewriting must be done after the CU is fully
+     read in, because otherwise at the point of rewriting some struct
+     type might not have been fully processed.  So, we keep a list of
+     all such types here and process them after expansion.  */
+  std::vector<struct type *> rust_unions;
+
   /* Mark used when releasing cached dies.  */
   unsigned int mark : 1;
 
@@ -10274,6 +10283,303 @@ fixup_go_packaging (struct dwarf2_cu *cu)
     }
 }
 
+/* Allocate a fully-qualified name consisting of the two parts on the
+   obstack.  */
+
+static const char *
+rust_fully_qualify (struct obstack *obstack, const char *p1, const char *p2)
+{
+  return obconcat (obstack, p1, "::", p2, (char *) NULL);
+}
+
+/* A helper that allocates a struct discriminant_info to attach to a
+   union type.  */
+
+static struct discriminant_info *
+alloc_discriminant_info (struct type *type, int discriminant_index,
+			 int default_index)
+{
+  gdb_assert (TYPE_CODE (type) == TYPE_CODE_UNION);
+  gdb_assert (default_index == -1
+	      || (default_index > 0 && default_index < TYPE_NFIELDS (type)));
+
+  TYPE_FLAG_DISCRIMINATED_UNION (type) = 1;
+
+  struct discriminant_info *disc
+    = ((struct discriminant_info *)
+       TYPE_ZALLOC (type,
+		    offsetof (struct discriminant_info, discriminants)
+		    + TYPE_NFIELDS (type) * sizeof (disc->discriminants[0])));
+  disc->default_index = default_index;
+  disc->discriminant_index = discriminant_index;
+
+  struct dynamic_prop prop;
+  prop.kind = PROP_UNDEFINED;
+  prop.data.baton = disc;
+
+  add_dyn_prop (DYN_PROP_DISCRIMINATED, prop, type);
+
+  return disc;
+}
+
+/* Some versions of rustc emitted enums in an unusual way.
+
+   Ordinary enums were emitted as unions.  The first element of each
+   structure in the union was named "RUST$ENUM$DISR".  This element
+   held the discriminant.
+
+   These versions of Rust also implemented the "non-zero"
+   optimization.  When the enum had two values, and one is empty and
+   the other holds a pointer that cannot be zero, the pointer is used
+   as the discriminant, with a zero value meaning the empty variant.
+   Here, the union's first member is of the form
+   RUST$ENCODED$ENUM$<fieldno>$<fieldno>$...$<variantname>
+   where the fieldnos are the indices of the fields that should be
+   traversed in order to find the field (which may be several fields deep)
+   and the variantname is the name of the variant of the case when the
+   field is zero.
+
+   This function recognizes whether TYPE is of one of these forms,
+   and, if so, smashes it to be a variant type.  */
+
+static void
+quirk_rust_enum (struct type *type, struct objfile *objfile)
+{
+  gdb_assert (TYPE_CODE (type) == TYPE_CODE_UNION);
+
+  /* We don't need to deal with empty enums.  */
+  if (TYPE_NFIELDS (type) == 0)
+    return;
+
+#define RUST_ENUM_PREFIX "RUST$ENCODED$ENUM$"
+  if (TYPE_NFIELDS (type) == 1
+      && strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
+		  strlen (RUST_ENUM_PREFIX)) == 0)
+    {
+      const char *name = TYPE_FIELD_NAME (type, 0) + strlen (RUST_ENUM_PREFIX);
+
+      /* Decode the field name to find the offset of the
+	 discriminant.  */
+      ULONGEST bit_offset = 0;
+      struct type *field_type = TYPE_FIELD_TYPE (type, 0);
+      while (name[0] >= '0' && name[0] <= '9')
+	{
+	  char *tail;
+	  unsigned long index = strtoul (name, &tail, 10);
+	  name = tail;
+	  if (*name != '$'
+	      || index >= TYPE_NFIELDS (field_type)
+	      || (TYPE_FIELD_LOC_KIND (field_type, index)
+		  != FIELD_LOC_KIND_BITPOS))
+	    {
+	      complaint (&symfile_complaints,
+			 _("Could not parse Rust enum encoding string \"%s\""
+			   "[in module %s]"),
+			 TYPE_FIELD_NAME (type, 0),
+			 objfile_name (objfile));
+	      return;
+	    }
+	  ++name;
+
+	  bit_offset += TYPE_FIELD_BITPOS (field_type, index);
+	  field_type = TYPE_FIELD_TYPE (field_type, index);
+	}
+
+      /* Make a union to hold the variants.  */
+      struct type *union_type = alloc_type (objfile);
+      TYPE_CODE (union_type) = TYPE_CODE_UNION;
+      TYPE_NFIELDS (union_type) = 3;
+      TYPE_FIELDS (union_type)
+	= (struct field *) TYPE_ZALLOC (type, 3 * sizeof (struct field));
+      TYPE_LENGTH (union_type) = TYPE_LENGTH (type);
+
+      /* Put the discriminant must at index 0.  */
+      TYPE_FIELD_TYPE (union_type, 0) = field_type;
+      TYPE_FIELD_ARTIFICIAL (union_type, 0) = 1;
+      TYPE_FIELD_NAME (union_type, 0) = "<<discriminant>>";
+      SET_FIELD_BITPOS (TYPE_FIELD (union_type, 0), bit_offset);
+
+      /* The order of fields doesn't really matter, so put the real
+	 field at index 1 and the data-less field at index 2.  */
+      struct discriminant_info *disc
+	= alloc_discriminant_info (union_type, 0, 1);
+      TYPE_FIELD (union_type, 1) = TYPE_FIELD (type, 0);
+      TYPE_FIELD_NAME (union_type, 1)
+	= rust_last_path_segment (TYPE_NAME (TYPE_FIELD_TYPE (union_type, 1)));
+      TYPE_NAME (TYPE_FIELD_TYPE (union_type, 1))
+	= rust_fully_qualify (&objfile->objfile_obstack, TYPE_NAME (type),
+			      TYPE_FIELD_NAME (union_type, 1));
+
+      const char *dataless_name
+	= rust_fully_qualify (&objfile->objfile_obstack, TYPE_NAME (type),
+			      name);
+      struct type *dataless_type = init_type (objfile, TYPE_CODE_VOID, 0,
+					      dataless_name);
+      TYPE_FIELD_TYPE (union_type, 2) = dataless_type;
+      /* NAME points into the original discriminant name, which
+	 already has the correct lifetime.  */
+      TYPE_FIELD_NAME (union_type, 2) = name;
+      SET_FIELD_BITPOS (TYPE_FIELD (union_type, 2), 0);
+      disc->discriminants[2] = 0;
+
+      /* Smash this type to be a structure type.  We have to do this
+	 because the type has already been recorded.  */
+      TYPE_CODE (type) = TYPE_CODE_STRUCT;
+      TYPE_NFIELDS (type) = 1;
+      TYPE_FIELDS (type)
+	= (struct field *) TYPE_ZALLOC (type, sizeof (struct field));
+
+      /* Install the variant part.  */
+      TYPE_FIELD_TYPE (type, 0) = union_type;
+      SET_FIELD_BITPOS (TYPE_FIELD (type, 0), 0);
+      TYPE_FIELD_NAME (type, 0) = "<<variants>>";
+    }
+  else if (TYPE_NFIELDS (type) == 1)
+    {
+      /* We assume that a union with a single field is a univariant
+	 enum.  */
+      /* Smash this type to be a structure type.  We have to do this
+	 because the type has already been recorded.  */
+      TYPE_CODE (type) = TYPE_CODE_STRUCT;
+
+      /* Make a union to hold the variants.  */
+      struct type *union_type = alloc_type (objfile);
+      TYPE_CODE (union_type) = TYPE_CODE_UNION;
+      TYPE_NFIELDS (union_type) = TYPE_NFIELDS (type);
+      TYPE_LENGTH (union_type) = TYPE_LENGTH (type);
+      TYPE_FIELDS (union_type) = TYPE_FIELDS (type);
+
+      struct type *field_type = TYPE_FIELD_TYPE (union_type, 0);
+      const char *variant_name
+	= rust_last_path_segment (TYPE_NAME (field_type));
+      TYPE_FIELD_NAME (union_type, 0) = variant_name;
+      TYPE_NAME (field_type)
+	= rust_fully_qualify (&objfile->objfile_obstack,
+			      TYPE_NAME (field_type), variant_name);
+
+      /* Install the union in the outer struct type.  */
+      TYPE_NFIELDS (type) = 1;
+      TYPE_FIELDS (type)
+	= (struct field *) TYPE_ZALLOC (union_type, sizeof (struct field));
+      TYPE_FIELD_TYPE (type, 0) = union_type;
+      TYPE_FIELD_NAME (type, 0) = "<<variants>>";
+      SET_FIELD_BITPOS (TYPE_FIELD (type, 0), 0);
+
+      alloc_discriminant_info (union_type, -1, 0);
+    }
+  else
+    {
+      struct type *disr_type = nullptr;
+      for (int i = 0; i < TYPE_NFIELDS (type); ++i)
+	{
+	  disr_type = TYPE_FIELD_TYPE (type, i);
+
+	  if (TYPE_NFIELDS (disr_type) == 0)
+	    {
+	      /* Could be data-less variant, so keep going.  */
+	    }
+	  else if (strcmp (TYPE_FIELD_NAME (disr_type, 0),
+			   "RUST$ENUM$DISR") != 0)
+	    {
+	      /* Not a Rust enum.  */
+	      return;
+	    }
+	  else
+	    {
+	      /* Found one.  */
+	      break;
+	    }
+	}
+
+      /* If we got here without a discriminant, then it's probably
+	 just a union.  */
+      if (disr_type == nullptr)
+	return;
+
+      /* Smash this type to be a structure type.  We have to do this
+	 because the type has already been recorded.  */
+      TYPE_CODE (type) = TYPE_CODE_STRUCT;
+
+      /* Make a union to hold the variants.  */
+      struct field *disr_field = &TYPE_FIELD (disr_type, 0);
+      struct type *union_type = alloc_type (objfile);
+      TYPE_CODE (union_type) = TYPE_CODE_UNION;
+      TYPE_NFIELDS (union_type) = 1 + TYPE_NFIELDS (type);
+      TYPE_LENGTH (union_type) = TYPE_LENGTH (type);
+      TYPE_FIELDS (union_type)
+	= (struct field *) TYPE_ZALLOC (union_type,
+					(TYPE_NFIELDS (union_type)
+					 * sizeof (struct field)));
+
+      memcpy (TYPE_FIELDS (union_type) + 1, TYPE_FIELDS (type),
+	      TYPE_NFIELDS (type) * sizeof (struct field));
+
+      /* Install the discriminant at index 0 in the union.  */
+      TYPE_FIELD (union_type, 0) = *disr_field;
+      TYPE_FIELD_ARTIFICIAL (union_type, 0) = 1;
+      TYPE_FIELD_NAME (union_type, 0) = "<<discriminant>>";
+
+      /* Install the union in the outer struct type.  */
+      TYPE_FIELD_TYPE (type, 0) = union_type;
+      TYPE_FIELD_NAME (type, 0) = "<<variants>>";
+      TYPE_NFIELDS (type) = 1;
+
+      /* Set the size and offset of the union type.  */
+      SET_FIELD_BITPOS (TYPE_FIELD (type, 0), 0);
+
+      /* We need a way to find the correct discriminant given a
+	 variant name.  For convenience we build a map here.  */
+      struct type *enum_type = FIELD_TYPE (*disr_field);
+      std::unordered_map<std::string, ULONGEST> discriminant_map;
+      for (int i = 0; i < TYPE_NFIELDS (enum_type); ++i)
+	{
+	  if (TYPE_FIELD_LOC_KIND (enum_type, i) == FIELD_LOC_KIND_ENUMVAL)
+	    {
+	      const char *name
+		= rust_last_path_segment (TYPE_FIELD_NAME (enum_type, i));
+	      discriminant_map[name] = TYPE_FIELD_ENUMVAL (enum_type, i);
+	    }
+	}
+
+      int n_fields = TYPE_NFIELDS (union_type);
+      struct discriminant_info *disc
+	= alloc_discriminant_info (union_type, 0, -1);
+      /* Skip the discriminant here.  */
+      for (int i = 1; i < n_fields; ++i)
+	{
+	  /* Find the final word in the name of this variant's type.
+	     That name can be used to look up the correct
+	     discriminant.  */
+	  const char *variant_name
+	    = rust_last_path_segment (TYPE_NAME (TYPE_FIELD_TYPE (union_type,
+								  i)));
+
+	  auto iter = discriminant_map.find (variant_name);
+	  if (iter != discriminant_map.end ())
+	    disc->discriminants[i] = iter->second;
+
+	  /* Remove the discriminant field.  */
+	  struct type *sub_type = TYPE_FIELD_TYPE (union_type, i);
+	  --TYPE_NFIELDS (sub_type);
+	  ++TYPE_FIELDS (sub_type);
+	  TYPE_FIELD_NAME (union_type, i) = variant_name;
+	  TYPE_NAME (sub_type)
+	    = rust_fully_qualify (&objfile->objfile_obstack,
+				  TYPE_NAME (type), variant_name);
+	}
+    }
+}
+
+/* Rewrite some Rust unions to be structures with variants parts.  */
+
+static void
+rust_union_quirks (struct dwarf2_cu *cu)
+{
+  gdb_assert (cu->language == language_rust);
+  for (struct type *type : cu->rust_unions)
+    quirk_rust_enum (type, cu->per_cu->dwarf2_per_objfile->objfile);
+}
+
 /* Return the symtab for PER_CU.  This works properly regardless of
    whether we're using the index or psymtabs.  */
 
@@ -10458,6 +10764,9 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
      physnames.  */
   compute_delayed_physnames (cu);
 
+  if (cu->language == language_rust)
+    rust_union_quirks (cu);
+
   /* Some compilers don't define a DW_AT_high_pc attribute for the
      compilation unit.  If the DW_AT_high_pc is missing, synthesize
      it, by scanning the DIE's below the compilation unit.  */
@@ -10560,6 +10869,9 @@ process_full_type_unit (struct dwarf2_per_cu_data *per_cu,
      physnames.  */
   compute_delayed_physnames (cu);
 
+  if (cu->language == language_rust)
+    rust_union_quirks (cu);
+
   /* TUs share symbol tables.
      If this is the first TU to use this symtab, complete the construction
      of it with end_expandable_symtab.  Otherwise, complete the addition of
@@ -16138,6 +16450,8 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
     }
 
   quirk_gcc_member_function_pointer (type, objfile);
+  if (cu->language == language_rust && die->tag == DW_TAG_union_type)
+    cu->rust_unions.push_back (type);
 
   /* NOTE: carlton/2004-03-16: GCC 3.4 (or at least one of its
      snapshots) has been known to create a die giving a declaration
diff --git a/gdb/rust-lang.c b/gdb/rust-lang.c
index 5ff80b249b..ce208674f4 100644
--- a/gdb/rust-lang.c
+++ b/gdb/rust-lang.c
@@ -36,17 +36,15 @@
 #include <string>
 #include <vector>
 
-/* Returns the last segment of a Rust path like foo::bar::baz.  Will
-   not handle cases where the last segment contains generics.  This
-   will return NULL if the last segment cannot be found.  */
+/* See rust-lang.h.  */
 
-static const char *
-rust_last_path_segment (const char * path)
+const char *
+rust_last_path_segment (const char *path)
 {
   const char *result = strrchr (path, ':');
 
   if (result == NULL)
-    return NULL;
+    return path;
   return result + 1;
 }
 
@@ -63,209 +61,29 @@ rust_crate_for_block (const struct block *block)
   return std::string (scope, cp_find_first_component (scope));
 }
 
-/* Information about the discriminant/variant of an enum */
-
-struct disr_info
-{
-  /* Name of field.  */
-  std::string name;
-  /* Field number in union.  Negative on error.  For an encoded enum,
-     the "hidden" member will always be field 1, and the "real" member
-     will always be field 0.  */
-  int field_no;
-  /* True if this is an encoded enum that has a single "real" member
-     and a single "hidden" member.  */
-  unsigned int is_encoded : 1;
-};
-
-/* The prefix of a specially-encoded enum.  */
+/* Return true if TYPE, which must be a struct type, represents a Rust
+   enum.  */
 
-#define RUST_ENUM_PREFIX "RUST$ENCODED$ENUM$"
-
-/* The number of the real field.  */
-
-#define RUST_ENCODED_ENUM_REAL 0
-
-/* The number of the hidden field.  */
-
-#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)
+rust_enum_p (const 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;
+  return (TYPE_CODE (type) == TYPE_CODE_STRUCT
+	  && TYPE_NFIELDS (type) == 1
+	  && TYPE_FLAG_DISCRIMINATED_UNION (TYPE_FIELD_TYPE (type, 0)));
 }
 
-/* Utility function to get discriminant info for a given value.  */
+/* Given an enum type and contents, find which variant is active.  */
 
-static struct disr_info
-rust_get_disr_info (struct type *type, const gdb_byte *valaddr,
-                    int embedded_offset, CORE_ADDR address,
-                    struct value *val)
+struct field *
+rust_enum_variant (struct type *type, const gdb_byte *contents)
 {
-  int i;
-  struct disr_info ret;
-  struct type *disr_type;
-  struct value_print_options opts;
-  const char *name_segment;
-
-  get_no_prettyformat_print_options (&opts);
-
-  ret.field_no = -1;
-  ret.is_encoded = 0;
-
-  if (TYPE_NFIELDS (type) == 0)
-    error (_("Encountered void enum value"));
-
-  /* If an enum has two values where one is empty and the other holds
-     a pointer that cannot be zero; then the Rust compiler optimizes
-     away the discriminant and instead uses a zero value in the
-     pointer field to indicate the empty variant.  */
-  if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
-	       strlen (RUST_ENUM_PREFIX)) == 0)
-    {
-      char *tail, *token, *saveptr = NULL;
-      unsigned long fieldno;
-      struct type *member_type;
-      LONGEST value;
-
-      ret.is_encoded = 1;
-
-      if (TYPE_NFIELDS (type) != 1)
-	error (_("Only expected one field in %s type"), RUST_ENUM_PREFIX);
-
-      /* Optimized enums have only one field.  */
-      member_type = TYPE_FIELD_TYPE (type, 0);
-
-      std::string name (TYPE_FIELD_NAME (type, 0));
-      tail = &name[0] + strlen (RUST_ENUM_PREFIX);
-
-      /* The location of the value that doubles as a discriminant is
-         stored in the name of the field, as
-         RUST$ENCODED$ENUM$<fieldno>$<fieldno>$...$<variantname>
-         where the fieldnos are the indices of the fields that should be
-         traversed in order to find the field (which may be several fields deep)
-         and the variantname is the name of the variant of the case when the
-         field is zero.  */
-      for (token = strtok_r (tail, "$", &saveptr);
-           token != NULL;
-           token = strtok_r (NULL, "$", &saveptr))
-        {
-	  if (sscanf (token, "%lu", &fieldno) != 1)
-	    {
-	      /* We have reached the enum name, which cannot start
-		 with a digit.  */
-	      break;
-	    }
-          if (fieldno >= TYPE_NFIELDS (member_type))
-	    error (_("%s refers to field after end of member type"),
-		   RUST_ENUM_PREFIX);
-
-          embedded_offset += TYPE_FIELD_BITPOS (member_type, fieldno) / 8;
-          member_type = TYPE_FIELD_TYPE (member_type, fieldno);
-        }
-
-      if (token == NULL)
-	error (_("Invalid form for %s"), RUST_ENUM_PREFIX);
-      value = unpack_long (member_type, valaddr + embedded_offset);
-
-      if (value == 0)
-	{
-	  ret.field_no = RUST_ENCODED_ENUM_HIDDEN;
-	  ret.name = std::string (TYPE_NAME (type)) + "::" + token;
-	}
-      else
-	{
-	  ret.field_no = RUST_ENCODED_ENUM_REAL;
-	  ret.name = (std::string (TYPE_NAME (type)) + "::"
-		      + rust_last_path_segment (TYPE_NAME (TYPE_FIELD_TYPE (type, 0))));
-	}
-
-      return ret;
-    }
-
-  disr_type = TYPE_FIELD_TYPE (type, 0);
-
-  if (TYPE_NFIELDS (disr_type) == 0)
-    {
-      /* This is a bounds check and should never be hit unless Rust
-	 has changed its debuginfo format.  */
-      error (_("Could not find enum discriminant field"));
-    }
-  else if (TYPE_NFIELDS (type) == 1)
-    {
-      /* Sometimes univariant enums are encoded without a
-         discriminant.  In that case, treating it as an encoded enum
-         with the first field being the actual type works.  */
-      const char *field_name = TYPE_NAME (TYPE_FIELD_TYPE (type, 0));
-      const char *last = rust_last_path_segment (field_name);
-      ret.name = std::string (TYPE_NAME (type)) + "::" + last;
-      ret.field_no = RUST_ENCODED_ENUM_REAL;
-      ret.is_encoded = 1;
-      return ret;
-    }
-
-  if (strcmp (TYPE_FIELD_NAME (disr_type, 0), "RUST$ENUM$DISR") != 0)
-    error (_("Rust debug format has changed"));
-
-  string_file temp_file;
-  /* The first value of the first field (or any field)
-     is the discriminant value.  */
-  c_val_print (TYPE_FIELD_TYPE (disr_type, 0),
-	       (embedded_offset + TYPE_FIELD_BITPOS (type, 0) / 8
-		+ TYPE_FIELD_BITPOS (disr_type, 0) / 8),
-	       address, &temp_file,
-	       0, val, &opts);
-
-  ret.name = std::move (temp_file.string ());
-  name_segment = rust_last_path_segment (ret.name.c_str ());
-  if (name_segment != NULL)
-    {
-      for (i = 0; i < TYPE_NFIELDS (type); ++i)
-	{
-	  /* Sadly, the discriminant value paths do not match the type
-	     field name paths ('core::option::Option::Some' vs
-	     'core::option::Some').  However, enum variant names are
-	     unique in the last path segment and the generics are not
-	     part of this path, so we can just compare those.  This is
-	     hackish and would be better fixed by improving rustc's
-	     metadata for enums.  */
-	  const char *field_type = TYPE_NAME (TYPE_FIELD_TYPE (type, i));
-
-	  if (field_type != NULL
-	      && strcmp (name_segment,
-			 rust_last_path_segment (field_type)) == 0)
-	    {
-	      ret.field_no = i;
-	      break;
-	    }
-	}
-    }
+  /* In Rust the enum always fills the containing structure.  */
+  gdb_assert (TYPE_FIELD_BITPOS (type, 0) == 0);
 
-  if (ret.field_no == -1 && !ret.name.empty ())
-    {
-      /* Somehow the discriminant wasn't found.  */
-      error (_("Could not find variant of %s with discriminant %s"),
-	     TYPE_TAG_NAME (type), ret.name.c_str ());
-    }
+  struct type *union_type = TYPE_FIELD_TYPE (type, 0);
 
-  return ret;
+  int fieldno = value_union_variant (union_type, contents);
+  return &TYPE_FIELD (union_type, fieldno);
 }
 
 /* See rust-lang.h.  */
@@ -281,12 +99,11 @@ rust_tuple_type_p (struct type *type)
 	  && TYPE_TAG_NAME (type)[0] == '(');
 }
 
-
 /* Return true if all non-static fields of a structlike type are in a
-   sequence like __0, __1, __2.  OFFSET lets us skip fields.  */
+   sequence like __0, __1, __2.  */
 
 static bool
-rust_underscore_fields (struct type *type, int offset)
+rust_underscore_fields (struct type *type)
 {
   int i, field_number;
 
@@ -298,17 +115,12 @@ rust_underscore_fields (struct type *type, int offset)
     {
       if (!field_is_static (&TYPE_FIELD (type, i)))
 	{
-	  if (offset > 0)
-	    offset--;
-	  else
-	    {
-	      char buf[20];
+	  char buf[20];
 
-	      xsnprintf (buf, sizeof (buf), "__%d", field_number);
-	      if (strcmp (buf, TYPE_FIELD_NAME (type, i)) != 0)
-		return false;
-	      field_number++;
-	    }
+	  xsnprintf (buf, sizeof (buf), "__%d", field_number);
+	  if (strcmp (buf, TYPE_FIELD_NAME (type, i)) != 0)
+	    return false;
+	  field_number++;
 	}
     }
   return true;
@@ -322,16 +134,7 @@ rust_tuple_struct_type_p (struct type *type)
   /* This is just an approximation until DWARF can represent Rust more
      precisely.  We exclude zero-length structs because they may not
      be tuple structs, and there's no way to tell.  */
-  return TYPE_NFIELDS (type) > 0 && rust_underscore_fields (type, 0);
-}
-
-/* Return true if a variant TYPE is a tuple variant, false otherwise.  */
-
-static bool
-rust_tuple_variant_type_p (struct type *type)
-{
-  /* First field is discriminant */
-  return rust_underscore_fields (type, 1);
+  return TYPE_NFIELDS (type) > 0 && rust_underscore_fields (type);
 }
 
 /* Return true if TYPE is a slice type, otherwise false.  */
@@ -516,7 +319,7 @@ rust_val_print_str (struct ui_file *stream, struct value *val,
 		    options);
 }
 
-/* rust_print_type branch for structs and untagged unions.  */
+/* rust_val_print helper for structs and untagged unions.  */
 
 static void
 val_print_struct (struct type *type, int embedded_offset,
@@ -601,6 +404,67 @@ val_print_struct (struct type *type, int embedded_offset,
     fputs_filtered ("}", stream);
 }
 
+/* rust_val_print helper for discriminated unions (Rust enums).  */
+
+static void
+rust_print_enum (struct type *type, int embedded_offset,
+		 CORE_ADDR address, struct ui_file *stream,
+		 int recurse, struct value *val,
+		 const struct value_print_options *options)
+{
+  struct value_print_options opts = *options;
+
+  opts.deref_ref = 0;
+
+  const gdb_byte *valaddr = value_contents_for_printing (val);
+  struct field *variant_field = rust_enum_variant (type, valaddr);
+  struct type *variant_type = FIELD_TYPE (*variant_field);
+  int nfields = TYPE_NFIELDS (variant_type);
+
+  bool is_tuple = rust_tuple_struct_type_p (variant_type);
+
+  fprintf_filtered (stream, "%s", TYPE_NAME (variant_type));
+  if (nfields == 0)
+    {
+      /* In case of a nullary variant like 'None', just output
+	 the name. */
+      return;
+    }
+
+  /* In case of a non-nullary variant, we output 'Foo(x,y,z)'. */
+  if (is_tuple)
+    fprintf_filtered (stream, "(");
+  else
+    {
+      /* struct variant.  */
+      fprintf_filtered (stream, "{");
+    }
+
+  bool first_field = true;
+  for (int j = 0; j < TYPE_NFIELDS (variant_type); j++)
+    {
+      if (!first_field)
+	fputs_filtered (", ", stream);
+      first_field = false;
+
+      if (!is_tuple)
+	fprintf_filtered (stream, "%s: ",
+			  TYPE_FIELD_NAME (variant_type, j));
+
+      val_print (TYPE_FIELD_TYPE (variant_type, j),
+		 (embedded_offset
+		  + TYPE_FIELD_BITPOS (variant_type, j) / 8),
+		 address,
+		 stream, recurse + 1, val, &opts,
+		 current_language);
+    }
+
+  if (is_tuple)
+    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
@@ -707,93 +571,22 @@ rust_val_print (struct type *type, int embedded_offset,
       break;
 
     case TYPE_CODE_UNION:
-      {
-	int j, nfields, first_field, is_tuple, start;
-	struct type *variant_type;
-	struct disr_info disr;
-	struct value_print_options opts;
-
-	/* 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, embedded_offset, address, stream,
-			      recurse, val, options);
-	    break;
-	  }
-
-	opts = *options;
-	opts.deref_ref = 0;
-
-	disr = rust_get_disr_info (type, valaddr, embedded_offset, address,
-				   val);
-
-	if (disr.is_encoded && disr.field_no == RUST_ENCODED_ENUM_HIDDEN)
-	  {
-	    fprintf_filtered (stream, "%s", disr.name.c_str ());
-	    break;
-	  }
-
-	first_field = 1;
-	variant_type = TYPE_FIELD_TYPE (type, disr.field_no);
-	nfields = TYPE_NFIELDS (variant_type);
-
-	is_tuple = (disr.is_encoded
-		    ? rust_tuple_struct_type_p (variant_type)
-		    : rust_tuple_variant_type_p (variant_type));
-	start = disr.is_encoded ? 0 : 1;
-
-	if (nfields > start)
-	  {
-	    /* In case of a non-nullary variant, we output 'Foo(x,y,z)'. */
-	    if (is_tuple)
-	      fprintf_filtered (stream, "%s(", disr.name.c_str ());
-	    else
-	      {
-		/* struct variant.  */
-		fprintf_filtered (stream, "%s{", disr.name.c_str ());
-	      }
-	  }
-	else
-	  {
-	    /* In case of a nullary variant like 'None', just output
-	       the name. */
-	    fprintf_filtered (stream, "%s", disr.name.c_str ());
-	    break;
-	  }
-
-	for (j = start; j < TYPE_NFIELDS (variant_type); j++)
-	  {
-	    if (!first_field)
-	      fputs_filtered (", ", stream);
-	    first_field = 0;
-
-	    if (!is_tuple)
-	      fprintf_filtered (stream, "%s: ",
-				TYPE_FIELD_NAME (variant_type, j));
-
-	    val_print (TYPE_FIELD_TYPE (variant_type, j),
-		       (embedded_offset
-			+ TYPE_FIELD_BITPOS (type, disr.field_no) / 8
-			+ TYPE_FIELD_BITPOS (variant_type, j) / 8),
-		       address,
-		       stream, recurse + 1, val, &opts,
-		       current_language);
-	  }
-
-	if (is_tuple)
-	  fputs_filtered (")", stream);
-	else
-	  fputs_filtered ("}", stream);
-      }
+      /* 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.  */
+      val_print_struct (type, embedded_offset, address, stream,
+			recurse, val, options);
       break;
 
     case TYPE_CODE_STRUCT:
-      val_print_struct (type, embedded_offset, address, stream,
-			recurse, val, options);
+      if (rust_enum_p (type))
+	rust_print_enum (type, embedded_offset, address, stream,
+			 recurse, val, options);
+      else
+	val_print_struct (type, embedded_offset, address, stream,
+			  recurse, val, options);
       break;
 
     default:
@@ -807,19 +600,18 @@ rust_val_print (struct type *type, int embedded_offset,
 \f
 
 static void
-rust_print_type (struct type *type, const char *varstring,
-		 struct ui_file *stream, int show, int level,
-		 const struct type_print_options *flags);
+rust_internal_print_type (struct type *type, const char *varstring,
+			  struct ui_file *stream, int show, int level,
+			  const struct type_print_options *flags,
+			  bool for_rust_enum);
 
 /* 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)
+		       const struct type_print_options *flags,
+		       bool for_rust_enum)
 {
-  bool is_tuple_struct;
-  int i;
-
   /* Print a tuple type simply.  */
   if (rust_tuple_type_p (type))
     {
@@ -831,22 +623,57 @@ rust_print_struct_def (struct type *type, const char *varstring,
   if (TYPE_N_BASECLASSES (type) > 0)
     c_print_type (type, varstring, stream, show, level, flags);
 
-  /* This code path is also used by unions.  */
-  if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
-    fputs_filtered ("struct ", stream);
-  else
-    fputs_filtered ("union ", stream);
+  /* Compute properties of TYPE here because, in the enum case, the
+     rest of the code ends up looking only at the variant part.  */
+  const char *tagname = TYPE_TAG_NAME (type);
+  bool is_tuple_struct = rust_tuple_struct_type_p (type);
+  bool is_tuple = rust_tuple_type_p (type);
+  bool is_enum = rust_enum_p (type);
+  bool is_univariant = false;
 
-  if (TYPE_TAG_NAME (type) != NULL)
-    fputs_filtered (TYPE_TAG_NAME (type), stream);
+  int enum_discriminant_index = -1;
+
+  if (for_rust_enum)
+    {
+      /* Already printing an outer enum, so nothing to print here.  */
+    }
+  else
+    {
+      /* This code path is also used by unions and enums.  */
+      if (is_enum)
+	{
+	  fputs_filtered ("enum ", stream);
+	  type = TYPE_FIELD_TYPE (type, 0);
+
+	  struct dynamic_prop *discriminant_prop
+	    = get_dyn_prop (DYN_PROP_DISCRIMINATED, type);
+	  struct discriminant_info *info
+	    = (struct discriminant_info *) discriminant_prop->data.baton;
+	  enum_discriminant_index = info->discriminant_index;
+	}
+      else if (TYPE_CODE (type) == TYPE_CODE_UNION && TYPE_NFIELDS (type) == 1)
+	{
+	  /* Probably a univariant enum.  */
+	  fputs_filtered ("enum ", stream);
+	  is_univariant = true;
+	}
+      else if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
+	fputs_filtered ("struct ", stream);
+      else
+	fputs_filtered ("union ", stream);
 
-  is_tuple_struct = rust_tuple_struct_type_p (type);
+      if (tagname != NULL)
+	fputs_filtered (tagname, stream);
+    }
 
-  if (TYPE_NFIELDS (type) == 0 && !rust_tuple_type_p (type))
+  if (TYPE_NFIELDS (type) == 0 && !is_tuple)
     return;
-  fputs_filtered (is_tuple_struct ? " (\n" : " {\n", stream);
+  if (for_rust_enum)
+    fputs_filtered (is_tuple_struct ? "(" : "{", stream);
+  else
+    fputs_filtered (is_tuple_struct ? " (\n" : " {\n", stream);
 
-  for (i = 0; i < TYPE_NFIELDS (type); ++i)
+  for (int i = 0; i < TYPE_NFIELDS (type); ++i)
     {
       QUIT;
       if (field_is_static (&TYPE_FIELD (type, i)))
@@ -858,17 +685,36 @@ rust_print_struct_def (struct type *type, const char *varstring,
 
       /* For a tuple struct we print the type but nothing
 	 else.  */
-      print_spaces_filtered (level + 2, stream);
-      if (!is_tuple_struct)
+      if (!for_rust_enum)
+	print_spaces_filtered (level + 2, stream);
+      if (is_enum)
+	{
+	  if (i == enum_discriminant_index)
+	    continue;
+	  fputs_filtered (TYPE_FIELD_NAME (type, i), stream);
+	}
+      else if (is_univariant)
+	{
+	  const char *name
+	    = rust_last_path_segment (TYPE_NAME (TYPE_FIELD_TYPE (type, i)));
+	  fputs_filtered (name, stream);
+	}
+      else 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);
+      rust_internal_print_type (TYPE_FIELD_TYPE (type, i), NULL,
+				stream,
+				((is_enum || is_univariant) ? show : show - 1),
+				level + 2, flags, is_enum || is_univariant);
+      if (!for_rust_enum)
+	fputs_filtered (",\n", stream);
+      else if (i + 1 < TYPE_NFIELDS (type))
+	fputs_filtered (", ", stream);
     }
 
-  fprintfi_filtered (level, stream, is_tuple_struct ? ")" : "}");
+  if (!for_rust_enum)
+    print_spaces_filtered (level, stream);
+  fputs_filtered (is_tuple_struct ? ")" : "}", stream);
 }
 
 /* la_print_typedef implementation for Rust.  */
@@ -887,9 +733,10 @@ rust_print_typedef (struct type *type,
 /* la_print_type implementation for Rust.  */
 
 static void
-rust_print_type (struct type *type, const char *varstring,
-		 struct ui_file *stream, int show, int level,
-		 const struct type_print_options *flags)
+rust_internal_print_type (struct type *type, const char *varstring,
+			  struct ui_file *stream, int show, int level,
+			  const struct type_print_options *flags,
+			  bool for_rust_enum)
 {
   int i;
 
@@ -910,7 +757,11 @@ rust_print_type (struct type *type, const char *varstring,
   switch (TYPE_CODE (type))
     {
     case TYPE_CODE_VOID:
-      fputs_filtered ("()", stream);
+      /* If we have an enum, we've already printed the type's
+	 unqualified name, and there is nothing else to print
+	 here.  */
+      if (!for_rust_enum)
+	fputs_filtered ("()", stream);
       break;
 
     case TYPE_CODE_FUNC:
@@ -927,15 +778,16 @@ rust_print_type (struct type *type, const char *varstring,
 	  QUIT;
 	  if (i > 0)
 	    fputs_filtered (", ", stream);
-	  rust_print_type (TYPE_FIELD_TYPE (type, i), "", stream, -1, 0,
-			   flags);
+	  rust_internal_print_type (TYPE_FIELD_TYPE (type, i), "", stream,
+				    -1, 0, flags, false);
 	}
       fputs_filtered (")", stream);
       /* If it returns unit, we can omit the return type.  */
       if (TYPE_CODE (TYPE_TARGET_TYPE (type)) != TYPE_CODE_VOID)
         {
           fputs_filtered (" -> ", stream);
-          rust_print_type (TYPE_TARGET_TYPE (type), "", stream, -1, 0, flags);
+          rust_internal_print_type (TYPE_TARGET_TYPE (type), "", stream,
+				    -1, 0, flags, false);
         }
       break;
 
@@ -944,8 +796,8 @@ rust_print_type (struct type *type, const char *varstring,
 	LONGEST low_bound, high_bound;
 
 	fputs_filtered ("[", stream);
-	rust_print_type (TYPE_TARGET_TYPE (type), NULL,
-			 stream, show - 1, level, flags);
+	rust_internal_print_type (TYPE_TARGET_TYPE (type), NULL,
+				  stream, show - 1, level, flags, false);
 
 	if (TYPE_HIGH_BOUND_KIND (TYPE_INDEX_TYPE (type)) == PROP_LOCEXPR
 	    || TYPE_HIGH_BOUND_KIND (TYPE_INDEX_TYPE (type)) == PROP_LOCLIST)
@@ -957,8 +809,10 @@ rust_print_type (struct type *type, const char *varstring,
       }
       break;
 
+    case TYPE_CODE_UNION:
     case TYPE_CODE_STRUCT:
-      rust_print_struct_def (type, varstring, stream, show, level, flags);
+      rust_print_struct_def (type, varstring, stream, show, level, flags,
+			     for_rust_enum);
       break;
 
     case TYPE_CODE_ENUM:
@@ -992,93 +846,21 @@ rust_print_type (struct type *type, const char *varstring,
       }
       break;
 
-    case TYPE_CODE_UNION:
-      {
-	/* ADT enums.  */
-	int i;
-	/* 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)
-	  {
-	    fputs_filtered (TYPE_TAG_NAME (type), stream);
-	    fputs_filtered (" ", stream);
-	  }
-	fputs_filtered ("{\n", stream);
-
-	if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
-		     strlen (RUST_ENUM_PREFIX)) == 0)
-	  {
-	    const char *zero_field = strrchr (TYPE_FIELD_NAME (type, 0), '$');
-	    if (zero_field != NULL && strlen (zero_field) > 1)
-	      {
-		fprintfi_filtered (level + 2, stream, "%s,\n", zero_field + 1);
-		/* There is no explicit discriminant field, skip nothing.  */
-		skip_to = 0;
-	      }
-	  }
-	else if (TYPE_NFIELDS (type) == 1)
-	  skip_to = 0;
-
-	for (i = 0; i < TYPE_NFIELDS (type); ++i)
-	  {
-	    struct type *variant_type = TYPE_FIELD_TYPE (type, i);
-	    const char *name
-	      = rust_last_path_segment (TYPE_NAME (variant_type));
-
-	    fprintfi_filtered (level + 2, stream, "%s", name);
-
-	    if (TYPE_NFIELDS (variant_type) > skip_to)
-	      {
-		int first = 1;
-		bool is_tuple = (TYPE_NFIELDS (type) == 1
-				 ? rust_tuple_struct_type_p (variant_type)
-				 : rust_tuple_variant_type_p (variant_type));
-		int j;
-
-		fputs_filtered (is_tuple ? "(" : "{", stream);
-		for (j = skip_to; j < TYPE_NFIELDS (variant_type); j++)
-		  {
-		    if (first)
-		      first = 0;
-		    else
-		      fputs_filtered (", ", stream);
-
-		    if (!is_tuple)
-		      fprintf_filtered (stream, "%s: ",
-					TYPE_FIELD_NAME (variant_type, j));
-
-		    rust_print_type (TYPE_FIELD_TYPE (variant_type, j), NULL,
-				     stream, show - 1, level + 2,
-				     flags);
-		  }
-		fputs_filtered (is_tuple ? ")" : "}", stream);
-	      }
-
-	    fputs_filtered (",\n", stream);
-	  }
-
-	fputs_filtered ("}", stream);
-      }
-      break;
-
     default:
     c_printer:
       c_print_type (type, varstring, stream, show, level, flags);
     }
 }
 
+static void
+rust_print_type (struct type *type, const char *varstring,
+		 struct ui_file *stream, int show, int level,
+		 const struct type_print_options *flags)
+{
+  rust_internal_print_type (type, varstring, stream, show, level,
+			    flags, false);
+}
+
 \f
 
 /* Compute the alignment of the type T.  */
@@ -1800,7 +1582,6 @@ rust_evaluate_subexp (struct type *expect_type, struct expression *exp,
         struct value *lhs;
         int pc, field_number, nfields;
         struct type *type, *variant_type;
-        struct disr_info disr;
 
         pc = (*pos)++;
         field_number = longest_to_int (exp->elts[pc + 1].longconst);
@@ -1808,58 +1589,64 @@ rust_evaluate_subexp (struct type *expect_type, struct expression *exp,
         lhs = evaluate_subexp (NULL_TYPE, exp, pos, noside);
 
         type = value_type (lhs);
-        /* 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))
+
+	/* Treat a univariant union as if it were an enum.  */
+	if (TYPE_CODE (type) == TYPE_CODE_UNION && TYPE_NFIELDS (type) == 1)
 	  {
-	    disr = rust_get_disr_info (type, value_contents (lhs),
-				       value_embedded_offset (lhs),
-				       value_address (lhs), lhs);
+	    lhs = value_primitive_field (lhs, 0, 0, type);
+	    type = value_type (lhs);
+	  }
 
-	    if (disr.is_encoded && disr.field_no == RUST_ENCODED_ENUM_HIDDEN)
-	      {
-		variant_type = NULL;
-		nfields = 0;
-	      }
-	    else
+	if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
+	  {
+	    struct type *outer_type = NULL;
+
+	    if (rust_enum_p (type))
 	      {
-		variant_type = TYPE_FIELD_TYPE (type, disr.field_no);
-		nfields = TYPE_NFIELDS (variant_type);
+		const gdb_byte *valaddr = value_contents (lhs);
+		struct field *variant_field = rust_enum_variant (type, valaddr);
+
+		struct value *union_value = value_primitive_field (lhs, 0, 0,
+								   type);
+
+		int fieldno = (variant_field
+			       - &TYPE_FIELD (value_type (union_value), 0));
+		lhs = value_primitive_field (union_value, 0, fieldno,
+					     value_type (union_value));
+		outer_type = type;
+		type = value_type (lhs);
 	      }
 
-	    if (!disr.is_encoded)
-	      ++field_number;
-
-	    if (field_number >= nfields || field_number < 0)
-	      error(_("Cannot access field %d of variant %s, \
-there are only %d fields"),
-		    disr.is_encoded ? field_number : field_number - 1,
-		    disr.name.c_str (),
-		    disr.is_encoded ? nfields : nfields - 1);
-
-	    if (!(disr.is_encoded
-		  ? rust_tuple_struct_type_p (variant_type)
-		  : rust_tuple_variant_type_p (variant_type)))
-	      error(_("Variant %s is not a tuple variant"), disr.name.c_str ());
-
-	    result = value_primitive_field (lhs, 0, field_number,
-					    variant_type);
-	  }
-	else if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
-	  {
 	    /* Tuples and tuple structs */
-	    nfields = TYPE_NFIELDS(type);
+	    nfields = TYPE_NFIELDS (type);
 
 	    if (field_number >= nfields || field_number < 0)
-	      error(_("Cannot access field %d of %s, there are only %d fields"),
-		    field_number, TYPE_TAG_NAME (type), nfields);
+	      {
+		if (outer_type != NULL)
+		  error(_("Cannot access field %d of variant %s::%s, "
+			  "there are only %d fields"),
+			field_number, TYPE_TAG_NAME (outer_type),
+			rust_last_path_segment (TYPE_TAG_NAME (type)),
+			nfields);
+		else
+		  error(_("Cannot access field %d of %s, "
+			  "there are only %d fields"),
+			field_number, TYPE_TAG_NAME (type), nfields);
+	      }
 
 	    /* Tuples are tuple structs too.  */
 	    if (!rust_tuple_struct_type_p (type))
-	      error(_("Attempting to access anonymous field %d of %s, which is \
-not a tuple, tuple struct, or tuple-like variant"),
-		    field_number, TYPE_TAG_NAME (type));
+	      {
+		if (outer_type != NULL)
+		  error(_("Variant %s::%s is not a tuple variant"),
+			TYPE_TAG_NAME (outer_type),
+			rust_last_path_segment (TYPE_TAG_NAME (type)));
+		else
+		  error(_("Attempting to access anonymous field %d "
+			  "of %s, which is not a tuple, tuple struct, or "
+			  "tuple-like variant"),
+		      field_number, TYPE_TAG_NAME (type));
+	      }
 
 	    result = value_primitive_field (lhs, 0, field_number, type);
 	  }
@@ -1882,53 +1669,44 @@ tuple structs, and tuple-like enum variants"));
 
 	const char *field_name = &exp->elts[pc + 2].string;
         type = value_type (lhs);
-        if (TYPE_CODE (type) == TYPE_CODE_UNION
-            && !rust_union_is_untagged (type))
+        if (TYPE_CODE (type) == TYPE_CODE_STRUCT && rust_enum_p (type))
 	  {
-	    int i, start;
-	    struct disr_info disr;
-	    struct type *variant_type;
-
-	    disr = rust_get_disr_info (type, value_contents (lhs),
-				       value_embedded_offset (lhs),
-				       value_address (lhs), lhs);
-
-	    if (disr.is_encoded && disr.field_no == RUST_ENCODED_ENUM_HIDDEN)
-	      error(_("Could not find field %s of struct variant %s"),
-		    field_name, disr.name.c_str ());
-
-	    variant_type = TYPE_FIELD_TYPE (type, disr.field_no);
-
-	    if (variant_type == NULL
-	        || (disr.is_encoded
-	            ? rust_tuple_struct_type_p (variant_type)
-	            : rust_tuple_variant_type_p (variant_type)))
-	      error(_("Attempting to access named field %s of tuple variant %s, \
-which has only anonymous fields"),
-		    field_name, disr.name.c_str ());
-
-	    start = disr.is_encoded ? 0 : 1;
-	    for (i = start; i < TYPE_NFIELDS (variant_type); i++)
+	    const gdb_byte *valaddr = value_contents (lhs);
+	    struct field *variant_field = rust_enum_variant (type, valaddr);
+
+	    struct value *union_value = value_primitive_field (lhs, 0, 0,
+							       type);
+
+	    int fieldno = (variant_field
+			   - &TYPE_FIELD (value_type (union_value), 0));
+	    lhs = value_primitive_field (union_value, 0, fieldno,
+					 value_type (union_value));
+
+	    struct type *outer_type = type;
+	    type = value_type (lhs);
+	    if (rust_tuple_type_p (type) || rust_tuple_struct_type_p (type))
+		error (_("Attempting to access named field foo of tuple "
+			 "variant %s::%s, which has only anonymous fields"),
+		       TYPE_TAG_NAME (outer_type),
+		       rust_last_path_segment (TYPE_NAME (type)));
+
+	    TRY
 	      {
-		if (strcmp (TYPE_FIELD_NAME (variant_type, i),
-			    field_name) == 0) {
-		  result = value_primitive_field (lhs, 0, i, variant_type);
-		  break;
-		}
+		result = value_struct_elt (&lhs, NULL, field_name,
+					   NULL, "structure");
 	      }
-
-	    if (i == TYPE_NFIELDS (variant_type))
-	      /* We didn't find it.  */
-	      error(_("Could not find field %s of struct variant %s"),
-		    field_name, disr.name.c_str ());
+	    CATCH (except, RETURN_MASK_ERROR)
+	      {
+		error (_("Could not find field %s of struct variant %s::%s"),
+		       field_name, TYPE_TAG_NAME (outer_type),
+		       rust_last_path_segment (TYPE_NAME (type)));
+	      }
+	    END_CATCH
 	  }
 	else
-	  {
-	    result = value_struct_elt (&lhs, NULL, field_name, NULL,
-				       "structure");
-	    if (noside == EVAL_AVOID_SIDE_EFFECTS)
-	      result = value_zero (value_type (result), VALUE_LVAL (result));
-	  }
+	  result = value_struct_elt (&lhs, NULL, field_name, NULL, "structure");
+	if (noside == EVAL_AVOID_SIDE_EFFECTS)
+	  result = value_zero (value_type (result), VALUE_LVAL (result));
       }
       break;
 
diff --git a/gdb/rust-lang.h b/gdb/rust-lang.h
index 554a440d58..ce30e33276 100644
--- a/gdb/rust-lang.h
+++ b/gdb/rust-lang.h
@@ -39,6 +39,11 @@ extern bool rust_tuple_struct_type_p (struct type *type);
    stringif no crate name can be found.  */
 extern std::string rust_crate_for_block (const struct block *block);
 
+/* Returns the last segment of a Rust path like foo::bar::baz.  Will
+   not handle cases where the last segment contains generics.  */
+
+extern const char *rust_last_path_segment (const char *path);
+
 /* Create a new slice type.  NAME is the name of the type.  ELT_TYPE
    is the type of the elements of the slice.  USIZE_TYPE is the Rust
    "usize" type to use.  The new type is allocated whereever ELT_TYPE
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index d2ebdbe011..c7a2142f65 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2018-02-19  Tom Tromey  <tom@tromey.com>
+
+	* gdb.rust/simple.exp: Accept more possible results in enum test.
+
 2018-02-20  Markus Metzger  <markus.t.metzger@intel.com>
 
 	* gdb.btrace/buffer-size.exp: Do not force BTS.
diff --git a/gdb/testsuite/gdb.rust/simple.exp b/gdb/testsuite/gdb.rust/simple.exp
index 04750d9eb1..230e6a7a88 100644
--- a/gdb/testsuite/gdb.rust/simple.exp
+++ b/gdb/testsuite/gdb.rust/simple.exp
@@ -220,8 +220,8 @@ gdb_test "print .." " = .*::ops::RangeFull"
 gdb_test "print str_some" \
     " = core::option::Option<\[a-z\]+::string::String>::Some\\(\[a-z\]+::string::String .*"
 gdb_test "print str_none" " = core::option::Option<\[a-z\]+::string::String>::None"
-gdb_test "print int_some" " = core::option::Option::Some\\(1\\)"
-gdb_test "print int_none" " = core::option::Option::None"
+gdb_test "print int_some" " = core::option::Option<u8>::Some\\(1\\)"
+gdb_test "print int_none" " = core::option::Option<u8>::None"
 gdb_test "print box_some" " = core::option::Option<\[a-z:\]*Box<u8>>::Some\\(.*\\)"
 gdb_test "print box_none" " = core::option::Option<\[a-z:\]*Box<u8>>::None"
 gdb_test "print custom_some" \
-- 
2.13.6

Re: [RFA 2/3] Convert Rust to use discriminated unions

[Joel Brobecker]

> This patch implements this idea by moving the current Rust enum
> handling code to dwarf2read.  This allows the simplification of some
> parts of rust-lang.c as well.

I'm actually wondering whether it make sense to move the "quirk"
functions to dwarf2read or not. This is just thinking out loud,
rather than a request: What do you think of having those functions
in rust-lang.c, and calling them from dwarf2read.c? Obviously,
the downside is that the function has to be non-static, but then
the rust-specific code rests in rust-lang, which you oversee...
(again, there is no obvious answer, and I am just wondering about
this option, so I will follow your preference).

> 2018-02-19  Tom Tromey  <tom@tromey.com>
> 
> 	* rust-lang.h (rust_last_path_segment): Declare.
> 	* rust-lang.c (rust_last_path_segment): Now public.  Change
> 	contract.
> 	(struct disr_info): Remove.
> 	(RUST_ENUM_PREFIX, RUST_ENCODED_ENUM_REAL)
> 	(RUST_ENCODED_ENUM_HIDDEN, rust_union_is_untagged)
> 	(rust_get_disr_info, rust_tuple_variant_type_p): Remove.
> 	(rust_enum_p, rust_enum_variant): New function.
> 	(rust_underscore_fields): Remove "offset" parameter.
> 	(rust_print_enum): New function.
> 	(rust_val_print) <TYPE_CODE_UNION>: Remove enum code.
> 	<TYPE_CODE_STRUCT>: Call rust_print_enum when appropriate.
> 	(rust_print_struct_def): Add "for_rust_enum" parameter.  Handle
> 	enums.
> 	(rust_internal_print_type): New function, from rust_print_type.
> 	Remove enum code.
> 	(rust_print_type): Call rust_internal_print_type.
> 	(rust_evaluate_subexp) <STRUCTOP_ANONYMOUS, STRUCTOP_STRUCT>:
> 	Update enum handling.
> 	* dwarf2read.c (struct dwarf2_cu) <rust_unions>: New field.
> 	(rust_fully_qualify, alloc_discriminant_info, quirk_rust_enum)
> 	(rust_union_quirks): New functions.
> 	(process_full_comp_unit, process_full_type_unit): Call
> 	rust_union_quirks.
> 	(process_structure_scope): Update rust_unions if necessary.

One small suggestion. Otherwise, OK for me.

> +#define RUST_ENUM_PREFIX "RUST$ENCODED$ENUM$"
> +  if (TYPE_NFIELDS (type) == 1
> +      && strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
> +		  strlen (RUST_ENUM_PREFIX)) == 0)

I think you can use startswith, here?


-- 
Joel

Re: [RFA 2/3] Convert Rust to use discriminated unions

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

>> This patch implements this idea by moving the current Rust enum
>> handling code to dwarf2read.  This allows the simplification of some
>> parts of rust-lang.c as well.

Joel> I'm actually wondering whether it make sense to move the "quirk"
Joel> functions to dwarf2read or not. This is just thinking out loud,
Joel> rather than a request: What do you think of having those functions
Joel> in rust-lang.c, and calling them from dwarf2read.c? Obviously,
Joel> the downside is that the function has to be non-static, but then
Joel> the rust-specific code rests in rust-lang, which you oversee...
Joel> (again, there is no obvious answer, and I am just wondering about
Joel> this option, so I will follow your preference).

I stuck it here to make sure the rewriting was very separate from the
uses -- so that I could be sure that the rust language code got a single
representation from the DWARF reader.

Moving it would mean making alloc_discriminant_info public as well.
That's not a big deal though.

I suppose my inclination is to just leave it in place.  But I am happy
to move it if you, or anybody, thinks it is important.

FWIW this quirk code is "temporary" -- at some point in the future, we
can decide not to support the versions of the rust compiler that emitted
this DWARF, and just remove it all.  I'm not sure when it would make
sense to do this.

>> +#define RUST_ENUM_PREFIX "RUST$ENCODED$ENUM$"
>> +  if (TYPE_NFIELDS (type) == 1
>> +      && strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX,
>> +		  strlen (RUST_ENUM_PREFIX)) == 0)

Joel> I think you can use startswith, here?

I made this change locally.

Tom

Re: [RFA 2/3] Convert Rust to use discriminated unions

[Joel Brobecker]

> I suppose my inclination is to just leave it in place.  But I am happy
> to move it if you, or anybody, thinks it is important.

I don't think this will be necessary. I'm happy to leave it there,
especially since it is clearly labeled "rust", so we know what it is
for.

-- 
Joel

[RFA 1/3] Initial support for variant parts

[Tom Tromey]

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(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index eac1572280..28990a4ec6 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -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.
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 613257c47d..62cd8b6f82 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -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.  */
 
diff --git a/gdb/valops.c b/gdb/valops.c
index e038c04fd1..2d31762b8a 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -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
diff --git a/gdb/value.h b/gdb/value.h
index e0ea22d4e5..4386be4135 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -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) */
-- 
2.13.6

Re: [RFA 1/3] Initial support for variant parts

[Joel Brobecker]

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

Re: [RFA 1/3] Initial support for variant parts

[Tom Tromey]

>>>>> "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

Re: [RFA 1/3] Initial support for variant parts

[Tom Tromey]

>>>>> "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

Re: [RFA 1/3] Initial support for variant parts

[Joel Brobecker]

> 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

Re: [RFA 1/3] Initial support for variant parts

[Tom Tromey]

>>>>> "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

[RFA 3/3] Handle DW_TAG_variant_part and DW_TAG_variant

[Tom Tromey]

This changes dwarf2read to understand DW_TAG_variant_part and
DW_TAG_variant.

Note that DW_AT_discr_list is not handled.  I did not need this for
Rust.  I imagine this should not be too hard to add later, should
someone need it.  Meanwhile I have gdb emit a complaint if it is seen.

There is a lurking issue concerning the placement of the discriminant
in the DWARF.  For Rust, I ended up following the letter of the
standard and having the discriminant be a child of the
DW_TAG_variant_part.  However, GCC's Ada support does not do this.
Pierre-Marie filed this with the DWARF committee:

    http://dwarfstd.org/ShowIssue.php?issue=180123.1

However as that is read-only, if you have comments you might consider
adding them to the GCC bug:

    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83935

Finally, there is a DWARF extension lurking in here.  In Rust, a
univariant enum will not have a discriminant.  However, in order to
unify the representation of all data-carrying enums, I've made LLVM
(and my forthcoming rustc patch) emit a univariant enum using a
DW_TAG_variant with a single variant part and without DW_AT_discr.
The lack of this DW_AT_discr is the extension.  I will submit an issue
on dwarfstd.org about this.

2018-02-19  Tom Tromey  <tom@tromey.com>

	* dwarf2read.c (struct nextfield) <discriminant_value,
	default_branch, is_discriminant>: New fields.
	(dwarf2_add_field): Handle DW_TAG_variant_part.
	(dwarf2_attach_fields_to_type): Attach a discriminant_info to a
	discriminated union.
	(read_structure_type): Handle DW_TAG_variant_part.
	(handle_struct_member_die): New function, extracted from
	process_structure_scope.  Handle DW_TAG_variant.
	(process_structure_scope): Handle discriminated unions.  Call
	handle_struct_member_die.

2018-02-20  Tom Tromey  <tom@tromey.com>

	* gdb.dwarf2/variant.c: New file.
	* gdb.dwarf2/variant.exp: New file.
---
 gdb/ChangeLog                        |  13 +++
 gdb/dwarf2read.c                     | 208 ++++++++++++++++++++++++++++-------
 gdb/testsuite/ChangeLog              |   5 +
 gdb/testsuite/gdb.dwarf2/variant.c   |  42 +++++++
 gdb/testsuite/gdb.dwarf2/variant.exp | 175 +++++++++++++++++++++++++++++
 5 files changed, 406 insertions(+), 37 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/variant.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/variant.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b4c34e0e54..dcbbce1e5d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,18 @@
 2018-02-19  Tom Tromey  <tom@tromey.com>
 
+	* dwarf2read.c (struct nextfield) <discriminant_value,
+	default_branch, is_discriminant>: New fields.
+	(dwarf2_add_field): Handle DW_TAG_variant_part.
+	(dwarf2_attach_fields_to_type): Attach a discriminant_info to a
+	discriminated union.
+	(read_structure_type): Handle DW_TAG_variant_part.
+	(handle_struct_member_die): New function, extracted from
+	process_structure_scope.  Handle DW_TAG_variant.
+	(process_structure_scope): Handle discriminated unions.  Call
+	handle_struct_member_die.
+
+2018-02-19  Tom Tromey  <tom@tromey.com>
+
 	* rust-lang.h (rust_last_path_segment): Declare.
 	* rust-lang.c (rust_last_path_segment): Now public.  Change
 	contract.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 12ed4ae33c..db23b1a421 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1659,6 +1659,15 @@ struct nextfield
   int accessibility;
   int virtuality;
   struct field field;
+  /* If we see a DW_TAG_variant, then this will be the discriminant
+     value.  */
+  ULONGEST discriminant_value;
+  /* If we see a DW_TAG_variant, then this will be set if this is the
+     default branch.  */
+  bool default_branch;
+  /* While reading a DW_TAG_variant_part, this will be set if this
+     field is the discriminant.  */
+  bool is_discriminant;
 };
 
 struct nextfnfield
@@ -15599,6 +15608,20 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
       FIELD_NAME (*fp) = type_name_no_tag (fp->type);
       fip->nbaseclasses++;
     }
+  else if (die->tag == DW_TAG_variant_part)
+    {
+      /* process_structure_scope will treat this DIE as a union.  */
+      process_structure_scope (die, cu);
+
+      /* The variant part is relative to the start of the enclosing
+	 structure.  */
+      SET_FIELD_BITPOS (*fp, 0);
+      fp->type = get_die_type (die, cu);
+      fp->artificial = 1;
+      fp->name = "<<variant>>";
+    }
+  else
+    gdb_assert_not_reached ("missing case in dwarf2_add_field");
 }
 
 /* Can the type given by DIE define another type?  */
@@ -15724,6 +15747,27 @@ dwarf2_attach_fields_to_type (struct field_info *fip, struct type *type,
       TYPE_N_BASECLASSES (type) = fip->nbaseclasses;
     }
 
+  if (TYPE_FLAG_DISCRIMINATED_UNION (type))
+    {
+      struct discriminant_info *di = alloc_discriminant_info (type, -1, -1);
+
+      int index = nfields - 1;
+      struct nextfield *field = fip->fields;
+
+      while (index >= 0)
+	{
+	  if (field->is_discriminant)
+	    di->discriminant_index = index;
+	  else if (field->default_branch)
+	    di->default_index = index;
+	  else
+	    di->discriminants[index] = field->discriminant_value;
+
+	  --index;
+	  field = field->next;
+	}
+    }
+
   /* Copy the saved-up fields into the field vector.  Start from the head of
      the list, adding to the tail of the field array, so that they end up in
      the same order in the array in which they were added to the list.  */
@@ -16185,6 +16229,11 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
     {
       TYPE_CODE (type) = TYPE_CODE_UNION;
     }
+  else if (die->tag == DW_TAG_variant_part)
+    {
+      TYPE_CODE (type) = TYPE_CODE_UNION;
+      TYPE_FLAG_DISCRIMINATED_UNION (type) = 1;
+    }
   else
     {
       TYPE_CODE (type) = TYPE_CODE_STRUCT;
@@ -16244,6 +16293,92 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
   return type;
 }
 
+/* A helper for process_structure_scope that handles a single member
+   DIE.  */
+
+static void
+handle_struct_member_die (struct die_info *child_die, struct type *type,
+			  struct field_info *fi,
+			  std::vector<struct symbol *> *template_args,
+			  struct dwarf2_cu *cu)
+{
+  if (child_die->tag == DW_TAG_member
+      || child_die->tag == DW_TAG_variable
+      || child_die->tag == DW_TAG_variant_part)
+    {
+      /* NOTE: carlton/2002-11-05: A C++ static data member
+	 should be a DW_TAG_member that is a declaration, but
+	 all versions of G++ as of this writing (so through at
+	 least 3.2.1) incorrectly generate DW_TAG_variable
+	 tags for them instead.  */
+      dwarf2_add_field (fi, child_die, cu);
+    }
+  else if (child_die->tag == DW_TAG_subprogram)
+    {
+      /* Rust doesn't have member functions in the C++ sense.
+	 However, it does emit ordinary functions as children
+	 of a struct DIE.  */
+      if (cu->language == language_rust)
+	read_func_scope (child_die, cu);
+      else
+	{
+	  /* C++ member function.  */
+	  dwarf2_add_member_fn (fi, child_die, type, cu);
+	}
+    }
+  else if (child_die->tag == DW_TAG_inheritance)
+    {
+      /* C++ base class field.  */
+      dwarf2_add_field (fi, child_die, cu);
+    }
+  else if (type_can_define_types (child_die))
+    dwarf2_add_type_defn (fi, child_die, cu);
+  else if (child_die->tag == DW_TAG_template_type_param
+	   || child_die->tag == DW_TAG_template_value_param)
+    {
+      struct symbol *arg = new_symbol (child_die, NULL, cu);
+
+      if (arg != NULL)
+	template_args->push_back (arg);
+    }
+  else if (child_die->tag == DW_TAG_variant)
+    {
+      /* In a variant we want to get the discriminant and also add a
+	 field for our sole member child.  */
+      struct attribute *discr = dwarf2_attr (child_die, DW_AT_discr_value, cu);
+
+      for (struct die_info *variant_child = child_die->child;
+	   variant_child != NULL;
+	   variant_child = sibling_die (variant_child))
+	{
+	  if (variant_child->tag == DW_TAG_member)
+	    {
+	      handle_struct_member_die (variant_child, type, fi,
+					template_args, cu);
+	      /* Only handle the one.  */
+	      break;
+	    }
+	}
+
+      /* We don't handle this but we might as well report it if we see
+	 it.  */
+      if (dwarf2_attr (child_die, DW_AT_discr_list, cu) != nullptr)
+	  complaint (&symfile_complaints,
+		     _("DW_AT_discr_list is not supported yet"
+		       " - DIE at 0x%x [in module %s]"),
+		     to_underlying (child_die->sect_off),
+		     objfile_name (cu->per_cu->dwarf2_per_objfile->objfile));
+
+      /* The first field was just added, so we can stash the
+	 discriminant there.  */
+      gdb_assert (fi->fields != NULL);
+      if (discr == NULL)
+	fi->fields->default_branch = true;
+      else
+	fi->fields->discriminant_value = DW_UNSND (discr);
+    }
+}
+
 /* Finish creating a structure or union type, including filling in
    its members and creating a symbol for it.  */
 
@@ -16258,6 +16393,39 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
   if (type == NULL)
     type = read_structure_type (die, cu);
 
+  /* When reading a DW_TAG_variant_part, we need to notice when we
+     read the discriminant member, so we can record it later in the
+     discriminant_info.  */
+  bool is_variant_part = TYPE_FLAG_DISCRIMINATED_UNION (type);
+  sect_offset discr_offset;
+
+  if (is_variant_part)
+    {
+      struct attribute *discr = dwarf2_attr (die, DW_AT_discr, cu);
+      if (discr == NULL)
+	{
+	  /* Maybe it's a univariant form, an extension we support.
+	     In this case arrange not to check the offset.  */
+	  is_variant_part = false;
+	}
+      else if (attr_form_is_ref (discr))
+	{
+	  struct dwarf2_cu *target_cu = cu;
+	  struct die_info *target_die = follow_die_ref (die, discr, &target_cu);
+
+	  discr_offset = target_die->sect_off;
+	}
+      else
+	{
+	  complaint (&symfile_complaints,
+		     _("DW_AT_discr does not have DIE reference form"
+		       " - DIE at 0x%x [in module %s]"),
+		     to_underlying (die->sect_off),
+		     objfile_name (cu->per_cu->dwarf2_per_objfile->objfile));
+	  is_variant_part = false;
+	}
+    }
+
   if (die->child != NULL && ! die_is_declaration (die, cu))
     {
       struct field_info fi;
@@ -16270,44 +16438,10 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
 
       while (child_die && child_die->tag)
 	{
-	  if (child_die->tag == DW_TAG_member
-	      || child_die->tag == DW_TAG_variable)
-	    {
-	      /* NOTE: carlton/2002-11-05: A C++ static data member
-		 should be a DW_TAG_member that is a declaration, but
-		 all versions of G++ as of this writing (so through at
-		 least 3.2.1) incorrectly generate DW_TAG_variable
-		 tags for them instead.  */
-	      dwarf2_add_field (&fi, child_die, cu);
-	    }
-	  else if (child_die->tag == DW_TAG_subprogram)
-	    {
-	      /* Rust doesn't have member functions in the C++ sense.
-		 However, it does emit ordinary functions as children
-		 of a struct DIE.  */
-	      if (cu->language == language_rust)
-		read_func_scope (child_die, cu);
-	      else
-		{
-		  /* C++ member function.  */
-		  dwarf2_add_member_fn (&fi, child_die, type, cu);
-		}
-	    }
-	  else if (child_die->tag == DW_TAG_inheritance)
-	    {
-	      /* C++ base class field.  */
-	      dwarf2_add_field (&fi, child_die, cu);
-	    }
-	  else if (type_can_define_types (child_die))
-	    dwarf2_add_type_defn (&fi, child_die, cu);
-	  else if (child_die->tag == DW_TAG_template_type_param
-		   || child_die->tag == DW_TAG_template_value_param)
-	    {
-	      struct symbol *arg = new_symbol (child_die, NULL, cu);
+	  handle_struct_member_die (child_die, type, &fi, &template_args, cu);
 
-	      if (arg != NULL)
-		template_args.push_back (arg);
-	    }
+	  if (is_variant_part && discr_offset == child_die->sect_off)
+	    fi.fields->is_discriminant = true;
 
 	  child_die = sibling_die (child_die);
 	}
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index c7a2142f65..b62a24466b 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2018-02-20  Tom Tromey  <tom@tromey.com>
+
+	* gdb.dwarf2/variant.c: New file.
+	* gdb.dwarf2/variant.exp: New file.
+
 2018-02-19  Tom Tromey  <tom@tromey.com>
 
 	* gdb.rust/simple.exp: Accept more possible results in enum test.
diff --git a/gdb/testsuite/gdb.dwarf2/variant.c b/gdb/testsuite/gdb.dwarf2/variant.c
new file mode 100644
index 0000000000..e0feadbcae
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/variant.c
@@ -0,0 +1,42 @@
+/* Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+unsigned char buffer[8];
+
+static void
+func (void)
+{
+}
+
+int
+main (void)
+{
+  /* Write the expected values into the buffer.  */
+  unsigned int x = 23;
+  if (*(char *) &x)
+    {
+      /* Little endian.  */
+      buffer[0] = 23;
+      buffer[4] = 23;
+    }
+  else
+    {
+      buffer[3] = 23;
+      buffer[7] = 23;
+    }
+
+  func ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/variant.exp b/gdb/testsuite/gdb.dwarf2/variant.exp
new file mode 100644
index 0000000000..d182182eff
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/variant.exp
@@ -0,0 +1,175 @@
+# Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test DW_TAG_variant_part and DW_TAG_variant.
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use
+# gas.
+if {![dwarf2_support]} {
+    return 0
+}
+
+standard_testfile .c variant.S
+
+# Make some DWARF for the test.
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    upvar cu_lang cu_lang
+
+    declare_labels uinteger_label float_label
+    declare_labels discr_1_label discr_2_label
+    declare_labels one_label two_label
+
+    # Creating a CU with 4-byte addresses lets this test link on
+    # both 32- and 64-bit machines.
+    cu { addr_size 4 } {
+	compile_unit {
+	    {name file1.txt}
+	    {language @DW_LANG_Rust}
+	} {
+            uinteger_label: DW_TAG_base_type {
+                {DW_AT_byte_size 4 DW_FORM_sdata}
+                {DW_AT_encoding  @DW_ATE_unsigned}
+                {DW_AT_name      {unsigned integer}}
+            }
+
+	    float_label: base_type {
+		{name float}
+		{encoding @DW_ATE_float}
+		{byte_size 4 DW_FORM_sdata}
+	    }
+
+	    one_label: structure_type {
+		{name One}
+		{byte_size 4 DW_FORM_sdata}
+	    } {
+		member {
+		    {name __0}
+		    {type :$uinteger_label}
+		    {data_member_location 0 data1}
+		}
+	    }
+
+	    two_label: structure_type {
+		{name Two}
+		{byte_size 4 DW_FORM_sdata}
+	    } {
+		member {
+		    {name __0}
+		    {type :$float_label}
+		    {data_member_location 0 data1}
+		}
+	    }
+
+	    structure_type {
+		{name Simple}
+		{byte_size 8 DW_FORM_sdata}
+	    } {
+		variant_part {
+		    {discr :$discr_1_label DW_FORM_ref4}
+		} {
+		    discr_1_label: member {
+			{type :$uinteger_label}
+			{data_member_location 0 data1}
+		    }
+
+		    variant {
+			{discr_value 23 udata}
+		    } {
+			member {
+			    {type :$one_label}
+			    {data_member_location 4 data1}
+			}
+		    }
+
+		    variant {
+			{discr_value 1 udata}
+		    } {
+			member {
+			    {type :$two_label}
+			    {data_member_location 4 data1}
+			}
+		    }
+		}
+	    }
+
+	    structure_type {
+		{name Defaulted}
+		{byte_size 8 DW_FORM_sdata}
+	    } {
+		variant_part {
+		    {discr :$discr_2_label DW_FORM_ref4}
+		} {
+		    discr_2_label: member {
+			{type :$uinteger_label}
+			{data_member_location 0 data1}
+		    }
+
+		    variant {
+		    } {
+			member {
+			    {type :$one_label}
+			    {data_member_location 4 data1}
+			}
+		    }
+
+		    variant {
+			{discr_value 1 udata}
+		    } {
+			member {
+			    {type :$two_label}
+			    {data_member_location 4 data1}
+			}
+		    }
+		}
+	    }
+
+	    structure_type {
+		{name Univariant}
+		{byte_size 8 DW_FORM_sdata}
+	    } {
+		variant_part {
+		} {
+		    variant {
+		    } {
+			member {
+			    {type :$one_label}
+			    {data_member_location 4 data1}
+			}
+		    }
+		}
+	    }
+	}
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+	  [list $srcfile $asm_file] debug] } {
+    return -1
+}
+
+if ![runto func] {
+    return -1
+}
+
+# Get the value into history so we can use it from Rust.
+gdb_test "print (void *) buffer" "\\\$1 = .void .. $hex .buffer."
+
+gdb_test "set language rust"
+gdb_test "print *(\$1 as *mut Simple)" " = One\\(23\\)"
+gdb_test "print *(\$1 as *mut Defaulted)" " = One\\(23\\)"
+gdb_test "print *(\$1 as *mut Univariant)" " = One\\(23\\)"
-- 
2.13.6

Re: [RFA 3/3] Handle DW_TAG_variant_part and DW_TAG_variant

[Joel Brobecker]

> 2018-02-19  Tom Tromey  <tom@tromey.com>
> 
> 	* dwarf2read.c (struct nextfield) <discriminant_value,
> 	default_branch, is_discriminant>: New fields.
> 	(dwarf2_add_field): Handle DW_TAG_variant_part.
> 	(dwarf2_attach_fields_to_type): Attach a discriminant_info to a
> 	discriminated union.
> 	(read_structure_type): Handle DW_TAG_variant_part.
> 	(handle_struct_member_die): New function, extracted from
> 	process_structure_scope.  Handle DW_TAG_variant.
> 	(process_structure_scope): Handle discriminated unions.  Call
> 	handle_struct_member_die.
> 
> 2018-02-20  Tom Tromey  <tom@tromey.com>
> 
> 	* gdb.dwarf2/variant.c: New file.
> 	* gdb.dwarf2/variant.exp: New file.

Looks good to me as well. One suggestion below.

Also, thanks for the gdb.dwarf2 testcase!

> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 12ed4ae33c..db23b1a421 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -1659,6 +1659,15 @@ struct nextfield
>    int accessibility;
>    int virtuality;
>    struct field field;
> +  /* If we see a DW_TAG_variant, then this will be the discriminant
> +     value.  */
> +  ULONGEST discriminant_value;
> +  /* If we see a DW_TAG_variant, then this will be set if this is the
> +     default branch.  */
> +  bool default_branch;
> +  /* While reading a DW_TAG_variant_part, this will be set if this
> +     field is the discriminant.  */
> +  bool is_discriminant;

Since those 3 new fields are related, what do you think of putting all 3
fields into a new struct, and then add one field in this struct?

> +static void
> +handle_struct_member_die (struct die_info *child_die, struct type *type,
> +			  struct field_info *fi,
> +			  std::vector<struct symbol *> *template_args,
> +			  struct dwarf2_cu *cu)

Thanks for extracting this code out and making it a function! ;-)


-- 
Joel

Re: [RFA 3/3] Handle DW_TAG_variant_part and DW_TAG_variant

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> Since those 3 new fields are related, what do you think of putting all 3
Joel> fields into a new struct, and then add one field in this struct?

I've made this change locally.

Tom

Re: [RFA 0/3] Discriminated unions

[Joel Brobecker]

Hi Tom,

> This series provides a minimal implementation of DW_TAG_variant and
> DW_TAG_variant_part reading for gdb.  It also converts the Rust
> language support to use this new approach.  The Rust changes are
> motivated by some work to change how the Rust compiler emits enums;
> see:
> 
>     https://github.com/rust-lang/rust/issues/32920
> 
> A DW_TAG_variant is represented in gdb as a union with some extra
> information about how to compute the discriminant and thus find the
> active variant part.

Nice work! I only have minor comments/questions...

-- 
Joel