[PATCH 4/4] gdb: make internalvar use a variant

[Simon Marchi <simon.marchi@polymtl.ca>]

Change the union in internalvar to be a variant.  This union holds
different data, based on the internalvar kind, which is stored in
internalvar::kind.

The advantage of a variant here is that we can more easily use non-POD
types in the alternatives.  And since it remembers which alternative is
the current one, accesses are checked so that accesses to the wrong
alternative throw an exception (see question about that below).

This is the first use of a variant in our code base, and it will
probably be used as a template for future uses.  So all suggestions
about how the variant is used are welcome.

Define one struct type for each kind of data we want to store in the
variant (for each internalvar kind), that makes the alternatives clear.
Remove the internalvar::kind field, as it becomes redundant with the
variant.  Keeping it just consumes more space, but most importantly  we
run the risk of having it and the variant out of sync.  The variant's
selector becomes the source of truth for the internalvar kind.

However, I find that being able to get the kind of an internalvar as an
enumerator quite handy.  Plus, it would be quite a bit of work to
rewrite all the code to replace

  if (var->kind == INTERNALVAR_VALUE)

with

  if (nonstd::holds_alternative<internalvar_value> (var->v))

and

  switch (var->kind)

with some functor + std::visit.  I also think it makes the code less
readable.  So I added a kind method to internalvar, to get an
internalvar_kind value from the variant's active alternative.  That's
perhaps not the most efficient thing, but it helps a lot.

Open questions:

 - The library uses std::variant if it is available (so, if building
   with C++17).  I think there are slight differences between the std
   and nonstd versions of variant.  I can't really explain what they
   are, I am not expert enough in C++.  But for example, when I had not
   made my visitor's methods const, the code would compile with
   std::variant but not nonstd::variant.  This means we could get some
   occasional build failures if some people build in C++11/14 and others
   C++17.  One way to avoid this would be to define the macro
   variant_CONFIG_SELECT_VARIANT so that we always use nonstd::variant,
   regardless of the C++ version.  The small downside is that we
   wouldn't be testing against std::variant, so some day in the future
   we want to make the switch to use std::variant, we'll have some fixes
   to do.

 - The library has the option [2] of being built with or without
   exception.  For example, that decides what to do if one tries to
   access an alternative that is not the active one:

     https://github.com/martinmoene/variant-lite/blob/f1af3518e4c28f12b09839b9d2ee37984cbf137a/include/nonstd/variant.hpp#L2107-L2114

   With exceptions enabled, bad_variant_access is thrown.  Without
   exceptions enabled, an assert() fails.  In our context, it would be
   nice if we could call gdb_assert fails, such that we get the usual
   "internal error" message if that ever happens.  I don't see an easy
   way to do this other than modifying the header file itself.

[1] https://github.com/martinmoene/variant-lite#select-stdvariant-or-nonstdvariant
[2] https://github.com/martinmoene/variant-lite/#disable-exceptions

Change-Id: I15957e091f740441128301e2bc603771f18771b5
---
 gdb/value.c | 334 ++++++++++++++++++++++++++++------------------------
 1 file changed, 183 insertions(+), 151 deletions(-)

diff --git a/gdb/value.c b/gdb/value.c
index 2c8af4b9d5b5..97ae469f670d 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -46,6 +46,7 @@
 #include "cli/cli-style.h"
 #include "expop.h"
 #include "inferior.h"
+#include "gdbsupport/variant/variant.hpp"
 
 /* Definition of a user function.  */
 struct internal_function
@@ -1989,41 +1990,50 @@ enum internalvar_kind
   INTERNALVAR_STRING,
 };
 
-union internalvar_data
+/* Used with INTERNALVAR_VOID.  */
+struct internalvar_void
+{};
+
+/* Value object used with INTERNALVAR_VALUE.  */
+struct internalvar_value
 {
-  /* A value object used with INTERNALVAR_VALUE.  */
-  struct value *value;
+  value_ref_ptr value;
+};
 
-  /* The call-back routine used with INTERNALVAR_MAKE_VALUE.  */
-  struct
-  {
-    /* The functions to call.  */
-    const struct internalvar_funcs *functions;
+/* Call-back routine used with INTERNALVAR_MAKE_VALUE.  */
+struct internalvar_make_value
+{
+  /* The functions to call.  */
+  const struct internalvar_funcs *functions;
 
-    /* The function's user-data.  */
-    void *data;
-  } make_value;
+  /* The function's user-data.  */
+  void *data;
+};
 
-  /* The internal function used with INTERNALVAR_FUNCTION.  */
-  struct
-  {
-    struct internal_function *function;
-    /* True if this is the canonical name for the function.  */
-    int canonical;
-  } fn;
+/* The internal function used with INTERNALVAR_FUNCTION.  */
+struct internalvar_function
+{
+  struct internal_function *function;
 
-  /* An integer value used with INTERNALVAR_INTEGER.  */
-  struct
-  {
-    /* If type is non-NULL, it will be used as the type to generate
-       a value for this internal variable.  If type is NULL, a default
-       integer type for the architecture is used.  */
-    struct type *type;
-    LONGEST val;
-  } integer;
+  /* True if this is the canonical name for the function.  */
+  int canonical;
+};
+
+/* An integer value used with INTERNALVAR_INTEGER.  */
+
+struct internalvar_integer
+{
+  /* If type is non-NULL, it will be used as the type to generate
+     a value for this internal variable.  If type is NULL, a default
+     integer type for the architecture is used.  */
+  struct type *type;
+  LONGEST val;
+};
 
+struct internalvar_string
+{
   /* A string value used with INTERNALVAR_STRING.  */
-  char *string;
+  std::string string;
 };
 
 /* Internal variables.  These are variables within the debugger
@@ -2033,16 +2043,45 @@ union internalvar_data
 
 struct internalvar
 {
-  struct internalvar *next;
-  char *name;
+  /* Return the kind of the internalvar.  */
+  internalvar_kind kind () const
+  {
+    struct visitor
+    {
+      internalvar_kind operator() (const internalvar_void &void_) const
+      { return INTERNALVAR_VOID; }
+
+      internalvar_kind operator() (const internalvar_value &value) const
+      { return INTERNALVAR_VALUE; }
+
+      internalvar_kind operator() (const internalvar_make_value &make_value) const
+      { return INTERNALVAR_MAKE_VALUE; }
+
+      internalvar_kind operator() (const internalvar_function &void_) const
+      { return INTERNALVAR_FUNCTION; }
+
+      internalvar_kind operator() (const internalvar_integer &integer) const
+      { return INTERNALVAR_INTEGER; }
 
-  /* We support various different kinds of content of an internal variable.
-     enum internalvar_kind specifies the kind, and union internalvar_data
-     provides the data associated with this particular kind.  */
+      internalvar_kind operator() (const internalvar_string &string) const
+      { return INTERNALVAR_STRING; }
+    };
 
-  enum internalvar_kind kind;
+    return nonstd::visit (visitor {}, this->v);
+  }
 
-  union internalvar_data u;
+  struct internalvar *next = nullptr;
+  char *name = nullptr;
+
+  /* Data associated to each kind of internalvar.  The active variant
+     alternative indicates the kind of the internalvar.  */
+  nonstd::variant<
+    internalvar_void,
+    internalvar_value,
+    internalvar_make_value,
+    internalvar_function,
+    internalvar_integer,
+    internalvar_string> v;
 };
 
 static struct internalvar *internalvars;
@@ -2081,7 +2120,7 @@ init_if_undefined_command (const char* args, int from_tty)
 
   /* Only evaluate the expression if the lvalue is void.
      This may still fail if the expression is invalid.  */
-  if (intvar->kind == INTERNALVAR_VOID)
+  if (intvar->kind () == INTERNALVAR_VOID)
     evaluate_expression (expr.get ());
 }
 
@@ -2126,12 +2165,13 @@ complete_internalvar (completion_tracker &tracker, const char *name)
 struct internalvar *
 create_internalvar (const char *name)
 {
-  struct internalvar *var = XNEW (struct internalvar);
+  struct internalvar *var = new internalvar;
 
   var->name = xstrdup (name);
-  var->kind = INTERNALVAR_VOID;
   var->next = internalvars;
   internalvars = var;
+  var->v = internalvar_void {};
+
   return var;
 }
 
@@ -2148,10 +2188,7 @@ create_internalvar_type_lazy (const char *name,
 			      void *data)
 {
   struct internalvar *var = create_internalvar (name);
-
-  var->kind = INTERNALVAR_MAKE_VALUE;
-  var->u.make_value.functions = funcs;
-  var->u.make_value.data = data;
+  var->v = internalvar_make_value { funcs, data };
   return var;
 }
 
@@ -2162,12 +2199,15 @@ compile_internalvar_to_ax (struct internalvar *var,
 			   struct agent_expr *expr,
 			   struct axs_value *value)
 {
-  if (var->kind != INTERNALVAR_MAKE_VALUE
-      || var->u.make_value.functions->compile_to_ax == NULL)
+  if (var->kind () != INTERNALVAR_MAKE_VALUE)
     return 0;
 
-  var->u.make_value.functions->compile_to_ax (var, expr, value,
-					      var->u.make_value.data);
+  const auto &make_value = nonstd::get<internalvar_make_value> (var->v);
+
+  if (make_value.functions->compile_to_ax == nullptr)
+    return 0;
+
+  make_value.functions->compile_to_ax (var, expr, value, make_value.data);
   return 1;
 }
 
@@ -2213,7 +2253,7 @@ value_of_internalvar (struct gdbarch *gdbarch, struct internalvar *var)
       return val;
     }
 
-  switch (var->kind)
+  switch (var->kind ())
     {
     case INTERNALVAR_VOID:
       val = allocate_value (builtin_type (gdbarch)->builtin_void);
@@ -2224,28 +2264,45 @@ value_of_internalvar (struct gdbarch *gdbarch, struct internalvar *var)
       break;
 
     case INTERNALVAR_INTEGER:
-      if (!var->u.integer.type)
-	val = value_from_longest (builtin_type (gdbarch)->builtin_int,
-				  var->u.integer.val);
-      else
-	val = value_from_longest (var->u.integer.type, var->u.integer.val);
-      break;
+      {
+	const auto &integer = nonstd::get<internalvar_integer> (var->v);
+
+	if (integer.type == nullptr)
+	  val = value_from_longest (builtin_type (gdbarch)->builtin_int,
+				    integer.val);
+	else
+	  val = value_from_longest (integer.type, integer.val);
+
+	break;
+      }
 
     case INTERNALVAR_STRING:
-      val = value_cstring (var->u.string, strlen (var->u.string),
-			   builtin_type (gdbarch)->builtin_char);
-      break;
+      {
+	const auto &string = nonstd::get<internalvar_string> (var->v);
+
+	val = value_cstring (string.string.c_str (), string.string.size (),
+			     builtin_type (gdbarch)->builtin_char);
+	break;
+      }
 
     case INTERNALVAR_VALUE:
-      val = value_copy (var->u.value);
-      if (value_lazy (val))
-	value_fetch_lazy (val);
-      break;
+      {
+	const auto &value = nonstd::get<internalvar_value> (var->v);
+
+	val = value_copy (value.value.get ());
+	if (value_lazy (val))
+	  value_fetch_lazy (val);
+
+	break;
+      }
 
     case INTERNALVAR_MAKE_VALUE:
-      val = (*var->u.make_value.functions->make_value) (gdbarch, var,
-							var->u.make_value.data);
-      break;
+      {
+	const auto &make_value = nonstd::get<internalvar_make_value> (var->v);
+
+	val = make_value.functions->make_value (gdbarch, var, make_value.data);
+	break;
+      }
 
     default:
       internal_error (__FILE__, __LINE__, _("bad kind"));
@@ -2268,8 +2325,7 @@ value_of_internalvar (struct gdbarch *gdbarch, struct internalvar *var)
      altogether.  At the moment, this seems like the behavior we
      want.  */
 
-  if (var->kind != INTERNALVAR_MAKE_VALUE
-      && val->lval != lval_computed)
+  if (var->kind () != INTERNALVAR_MAKE_VALUE && val->lval != lval_computed)
     {
       VALUE_LVAL (val) = lval_internalvar;
       VALUE_INTERNALVAR (val) = var;
@@ -2281,19 +2337,21 @@ value_of_internalvar (struct gdbarch *gdbarch, struct internalvar *var)
 int
 get_internalvar_integer (struct internalvar *var, LONGEST *result)
 {
-  if (var->kind == INTERNALVAR_INTEGER)
+  if (var->kind () == INTERNALVAR_INTEGER)
     {
-      *result = var->u.integer.val;
+      const auto &integer = nonstd::get<internalvar_integer> (var->v);
+      *result = integer.val;
       return 1;
     }
 
-  if (var->kind == INTERNALVAR_VALUE)
+  if (var->kind () == INTERNALVAR_VALUE)
     {
-      struct type *type = check_typedef (value_type (var->u.value));
+      const auto &value = nonstd::get<internalvar_value> (var->v);
+      struct type *type = check_typedef (value_type (value.value.get ()));
 
       if (type->code () == TYPE_CODE_INT)
 	{
-	  *result = value_as_long (var->u.value);
+	  *result = value_as_long (value.value.get ());
 	  return 1;
 	}
     }
@@ -2305,11 +2363,14 @@ static int
 get_internalvar_function (struct internalvar *var,
 			  struct internal_function **result)
 {
-  switch (var->kind)
+  switch (var->kind ())
     {
     case INTERNALVAR_FUNCTION:
-      *result = var->u.fn.function;
-      return 1;
+      {
+	const auto &function = nonstd::get<internalvar_function> (var->v);
+	*result = function.function;
+	return 1;
+      }
 
     default:
       return 0;
@@ -2325,20 +2386,23 @@ set_internalvar_component (struct internalvar *var,
   struct gdbarch *arch;
   int unit_size;
 
-  switch (var->kind)
+  switch (var->kind ())
     {
     case INTERNALVAR_VALUE:
-      addr = value_contents_writeable (var->u.value).data ();
-      arch = get_value_arch (var->u.value);
-      unit_size = gdbarch_addressable_memory_unit_size (arch);
-
-      if (bitsize)
-	modify_field (value_type (var->u.value), addr + offset,
-		      value_as_long (newval), bitpos, bitsize);
-      else
-	memcpy (addr + offset * unit_size, value_contents (newval).data (),
-		TYPE_LENGTH (value_type (newval)));
-      break;
+      {
+	const auto &value = nonstd::get<internalvar_value> (var->v);
+	addr = value_contents_writeable (value.value.get ()).data ();
+	arch = get_value_arch (value.value.get ());
+	unit_size = gdbarch_addressable_memory_unit_size (arch);
+
+	if (bitsize)
+	  modify_field (value_type (value.value.get ()), addr + offset,
+			value_as_long (newval), bitpos, bitsize);
+	else
+	  memcpy (addr + offset * unit_size, value_contents (newval).data (),
+		  TYPE_LENGTH (value_type (newval)));
+	break;
+      }
 
     default:
       /* We can never get a component of any other kind.  */
@@ -2349,29 +2413,30 @@ set_internalvar_component (struct internalvar *var,
 void
 set_internalvar (struct internalvar *var, struct value *val)
 {
-  enum internalvar_kind new_kind;
-  union internalvar_data new_data = { 0 };
-
-  if (var->kind == INTERNALVAR_FUNCTION && var->u.fn.canonical)
+  if (var->kind () == INTERNALVAR_FUNCTION
+      && nonstd::get<internalvar_function> (var->v).canonical)
     error (_("Cannot overwrite convenience function %s"), var->name);
 
   /* Prepare new contents.  */
   switch (check_typedef (value_type (val))->code ())
     {
     case TYPE_CODE_VOID:
-      new_kind = INTERNALVAR_VOID;
+      var->v = internalvar_void {};
       break;
 
     case TYPE_CODE_INTERNAL_FUNCTION:
-      gdb_assert (VALUE_LVAL (val) == lval_internalvar);
-      new_kind = INTERNALVAR_FUNCTION;
-      get_internalvar_function (VALUE_INTERNALVAR (val),
-				&new_data.fn.function);
-      /* Copies created here are never canonical.  */
-      break;
+      {
+	gdb_assert (VALUE_LVAL (val) == lval_internalvar);
+
+	internal_function *func;
+	get_internalvar_function (VALUE_INTERNALVAR (val), &func);
+
+	/* Copies created here are never canonical.  */
+	var->v = internalvar_function { func, 0 };
+	break;
+      }
 
     default:
-      new_kind = INTERNALVAR_VALUE;
       struct value *copy = value_copy (val);
       copy->modifiable = 1;
 
@@ -2382,10 +2447,8 @@ set_internalvar (struct internalvar *var, struct value *val)
 	value_fetch_lazy (copy);
 
       /* Release the value from the value chain to prevent it from being
-	 deleted by free_all_values.  From here on this function should not
-	 call error () until new_data is installed into the var->u to avoid
-	 leaking memory.  */
-      new_data.value = release_value (copy).release ();
+	 deleted by free_all_values.  */
+      var->v = internalvar_value { release_value (copy) };
 
       /* Internal variables which are created from values with a dynamic
 	 location don't need the location property of the origin anymore.
@@ -2393,73 +2456,35 @@ set_internalvar (struct internalvar *var, struct value *val)
 	 when accessing the value.
 	 If we keep it, we would still refer to the origin value.
 	 Remove the location property in case it exist.  */
-      value_type (new_data.value)->remove_dyn_prop (DYN_PROP_DATA_LOCATION);
-
+      value_type (copy)->remove_dyn_prop (DYN_PROP_DATA_LOCATION);
       break;
     }
-
-  /* Clean up old contents.  */
-  clear_internalvar (var);
-
-  /* Switch over.  */
-  var->kind = new_kind;
-  var->u = new_data;
-  /* End code which must not call error().  */
 }
 
 void
 set_internalvar_integer (struct internalvar *var, LONGEST l)
 {
-  /* Clean up old contents.  */
-  clear_internalvar (var);
-
-  var->kind = INTERNALVAR_INTEGER;
-  var->u.integer.type = NULL;
-  var->u.integer.val = l;
+  var->v = internalvar_integer { nullptr, l };
 }
 
 void
 set_internalvar_string (struct internalvar *var, const char *string)
 {
-  /* Clean up old contents.  */
-  clear_internalvar (var);
-
-  var->kind = INTERNALVAR_STRING;
-  var->u.string = xstrdup (string);
+  var->v = internalvar_string { string };
 }
 
 static void
 set_internalvar_function (struct internalvar *var, struct internal_function *f)
 {
-  /* Clean up old contents.  */
-  clear_internalvar (var);
-
-  var->kind = INTERNALVAR_FUNCTION;
-  var->u.fn.function = f;
-  var->u.fn.canonical = 1;
   /* Variables installed here are always the canonical version.  */
+  var->v = internalvar_function { f, 1 };
 }
 
 void
 clear_internalvar (struct internalvar *var)
 {
-  /* Clean up old contents.  */
-  switch (var->kind)
-    {
-    case INTERNALVAR_VALUE:
-      value_decref (var->u.value);
-      break;
-
-    case INTERNALVAR_STRING:
-      xfree (var->u.string);
-      break;
-
-    default:
-      break;
-    }
-
   /* Reset to void kind.  */
-  var->kind = INTERNALVAR_VOID;
+  var->v = internalvar_void {};
 }
 
 const char *
@@ -2579,17 +2604,24 @@ static void
 preserve_one_internalvar (struct internalvar *var, struct objfile *objfile,
 			  htab_t copied_types)
 {
-  switch (var->kind)
+  switch (var->kind ())
     {
     case INTERNALVAR_INTEGER:
-      if (var->u.integer.type
-	  && var->u.integer.type->objfile_owner () == objfile)
-	var->u.integer.type
-	  = copy_type_recursive (objfile, var->u.integer.type, copied_types);
-      break;
+      {
+	auto &integer = nonstd::get<internalvar_integer> (var->v);
+
+	if (integer.type != nullptr
+	    && integer.type->objfile_owner () == objfile)
+	  integer.type
+	    = copy_type_recursive (objfile, integer.type, copied_types);
+
+	break;
+      }
 
     case INTERNALVAR_VALUE:
-      preserve_one_value (var->u.value, objfile, copied_types);
+      preserve_one_value
+	(nonstd::get<internalvar_value> (var->v).value.get (), objfile,
+	 copied_types);
       break;
     }
 }
-- 
2.34.1