Re: [PATCH 15/16] gdb: make cmd_list_element var an optional union

[Lancelot SIX <lsix@lancelotsix.com>]

On Wed, Jul 14, 2021 at 12:08:51PM +0000, Lancelot SIX via Gdb-patches wrote:
> On Wed, Jul 14, 2021 at 12:55:19AM -0400, Simon Marchi via Gdb-patches wrote:
> > The field cmd_list_element::var is used by set and show commands and
> > points to the variable affected by the set command and shown by the show
> > command.  It is of type `void *` and cast to a pointer of the
> > appriopriate type on each use depending on the type of the setting
> > (cmd_list_element::var_type).
> > 
> > I propose changing cmd_list_element to be a union of all the possible
> > pointer types.  Fundamentally, this doesn't change much what is
> > happening.  But I think this helps understand better how the field is
> > used and get a bit of type safety (while it is still possible to use the
> > wrong member at some point, we can't cast to something completely
> > unrealted).
> 
> Hi,
> 
> I happen to have been working on a different approach to the same
> problem.  This was triggered when I was reading one of your previous
> patches where the surrounding codes had a lot of C style casts all
> around.  My approach is arguably too complicated for the task, but it
> is meant to address (at least partly) the last point you discuss.
> 
> Here is the gist of my approach:  I add templated getter / setter to the
> var member, the template parameter being the var_type you expect.  The
> usage looks like this, if you want to access the value of a
> cmd_list_element of var_types var_zuinteger:
> 
> 	cmd_list_element *el;
> 	unsigned int val = el->get_var<var_zuinteger> ();  // This type checks.
> 
> The implementation for that requires a bit of boiler plate, but is not
> too complex to follow.  I add a helper struct I use to describe the
> storage type used or each var_types:
> 
> 	template<var_types T> struct var_types_storage { };
> 	template<> struct var_types_storage<var_integer> { using t = int; } ;
> 	template<> struct var_types_storage<var_uinteger> { using t = unsigned int; };
> 	template<> struct var_types_storage<var_string> { using t = char *; };
> 	template<> struct var_types_storage<var_enum> { using t = const char *; };
> 	// And so on for all the possible var type
> 
> With that available, the getter / setter is quite easy to write:
> 
> 	template<var_types T>
> 	typename var_types_storage<T>::t get_var () const
> 	{
> 	  gdb_assert (T == this->var_type);
> 	  return *static_cast<typename var_types_storage<T>::t *> (this->var);
> 	}
> 	
> 	template<var_types T>
> 	void set_var (typename var_types_storage<T>::t v)
> 	{
> 	  gdb_assert (T == this->var_type);
> 	  *static_cast<typename var_types_storage<T>::t *> (this->var> = v;
> 	}
> 
> 	template<var_types T>
> 	void set_var_p (typename var_types_storage<T>::t v)
> 	{
> 	  this->var_type = T;
> 	  this->var = static_cast<void *> v;
> 	}
> 
> add_set_or_show_cmd can also be updated in a similar way:
> 
> 	template<var_types T> static struct cmd_list_element *
> 	add_set_or_show_cmd (const char *name,
> 			     enum cmd_types type,
> 			     enum command_class theclass,
> 			     typename var_types_storage<T>::t *var,
> 			     const char *doc,
> 			     struct cmd_list_element **list)
> 	{
> 	  struct cmd_list_element *c = add_cmd (name, theclass, doc, list);
> 	  c->set_var_p<T> (var);
> 	  // rest is unchanged
> 	  return c;
> 	}
> 	
> 	// similar stuff for add_setshow_cmd_full
> 
> Obviously there are a few differences with your approach.
> 
> First, with my approach, it is possible to add the gdb_assert to check
> that when one accesses a value assuming it is of a given type, it
> actually is.  However, there are few limitations with that, most
> notably in gdb/cli/cli-setshow.c, where some processing is common
> between cases (var_uinteger and var_zuinteger have quite a lot in
> common for example).  At some point, the caller wants to call
> set_var<one of var_uinteger or var_zuinteger>, but obviously the
> assertion will fail half of the time. To go around this, I also use less
> type-safe setters / getters:
> 
> 	template<typename T>
> 	const T get_var_as() { return *static_cast<T *> (this->var); }
> 	template<typename T>
> 	void set_var_as (T v) { *static_cast<T *> (this->var) = v; }
> 
> Second, with the templated approach the caller does not need to know
> which member of the union he should access for a given var_types, the
> compiler will tel him if an incompatible types are used on the call
> site.
> 
> On the other hand, my approach might be more difficult to follow / more
> surprising for someone not familiar with this kind of use of template
> specialization.  It also requires a bit more boiler plate code (i.e. the
> var_types_storage struct).
> 
> I still rely on type punning and a underlying void * pointer, which is
> not ideal.  But at least, this becomes a detail of the implementation
> and can be ignored by the caller with all the casting done in just one
> place.  The void * pointer should (in my approach) eventually be turned
> into a private member, which I have not done yet.
> 
> Anyway, your approach is quite an improvement compared to the current
> type casting all over the place.  I guess that we could, if you want,
> have a solution which which is a hybrid of both (i.e. a union instead of
> a void*, and templated access functions).
> 
> Obviously I am a bit biased toward my approach.  Any thoughts?  I’ll
> pause my work on my patch for the moment.
> 
> I still need to give a proper read to your patch series, I just skimmed
> through to compare our approaches for the moment.
> 
> Best,
> Lancelot.

Actually, I have been playing around with it a bit more today, and with
extra boiler plate, I can remove the get_var_as accessor, and have a
more all-rounded get_var. 

It becomes possible to write something like:

	// No matter if we are looking at a var_uinteger or a
	// var_zuinteger, the underlying type we are interested in is
	// a unsigned int.
	unsigned int *foo = c->get_var<var_uinteger, var_zuinteger> ();

In this case, get_var will even figure the common ground for all
possibilities the user asks for.  The intended use case is when printing
the value of a string (ish) variable, and we want to cover multiple
scenarios including var_enum and var_string, we can write something
like:

	// c is a const char *, but would be char * if var_enum was not
	// in the list.
	auto c = v.get_var<var_enum, var_string, var_filename> ();

The code could look like something like this (I am sure it is possible
to have a simpler solution to create a similar result):

	template<var_types... Ts>
	struct var_types_storage_helper;
	
	template<var_types T>
	struct var_types_storage_helper<T>
	{
	  static constexpr bool all_same = true;
	  using type = typename var_types_storage<T>::t;
	  static constexpr bool covers_var_type (var_types t)
	    {
	      return t == T;
	    }
	};
	
	template<var_types T, var_types U>
	struct var_types_storage_helper<T, U>
	{
	  static constexpr bool all_same = std::is_same<typename var_types_storage<T>::t, typename var_types_storage<U>::t>::value;
	  using type = typename var_types_storage<T>::t;
	  static constexpr bool covers_var_type (var_types t)
	    {
	      return var_types_storage_helper<T>::covers_var_type (t) || var_types_storage_helper<U>::covers_var_type (t);
	    }
	};
	
	template<var_types T, var_types U, var_types... Us>
	struct var_types_storage_helper<T, U, Us...>
	{
	  static constexpr bool all_same = var_types_storage_helper<T, U>::all_same && var_types_storage_helper<T, Us...>::all_same;
	  using type = typename var_types_storage<T>::t;
	  static constexpr bool covers_var_type (var_types t)
	    {
	      return var_types_storage_helper<T> (t) || var_types_storage_helper<U, Us...>::covers_var_type (t);
	    }
	};

and the accessors become:

	  template<var_types... T>
	    typename std::common_type<typename var_types_storage<T>::t...>::type get_var() const
	      {
		assert (var_types_storage_helper<T...>::covers_var_type (this->var_type));
		return *static_cast<typename std::common_type<typename var_types_storage<T>::t...>::type *>(this->var);
	      }
	
	  template<var_types... T>
	    void set_var(typename var_types_storage_helper<T...>::type v)
	      {
		assert (var_types_storage_helper<T...>::covers_var_type (this->var_type));
		static_assert (var_types_storage_helper<T...>::all_same);
		*static_cast<typename var_types_storage_helper<T...>::type *> (this->var) = v;
	      }
	
	  template<var_types T>
	    void set_var_p (typename var_types_storage<T>::t *v)
	      {
		this->var_type = T;
		this->var = static_cast<void *> (v);
	      }

I guess improves the type safety side of things, but the
implementation definitely gets more complicated.  I am not sure the
added checks come at a reasonable price.

This should all be valid c++11.

Any thoughts?

Best,
Lancelot.

> 
> 
> > 
> > I wrapped the union in an optional, because we need to check in some
> > spots whether var is set or not.  I think that conceptually, an optional
> > makes the most sense.  Another option would be to pick an arbitrary
> > member of the union and compare it to nullptr, whenever we want to know
> > whether var is set, but that seems a bit more hack-ish.
> > 
> > A note on the Guile situation: something that makes our life a little
> > bit complicated is that is possible to assign and read the value of a
> > Guile-created parameter that is not yet registered (see previous patch
> > that adds a test for this).  It would have been much more simple to say
> > that this is simply not supported anymore, but that could break existing
> > scripts, so I don't think it is a good idea.  In some cases, for example
> > when printing the value of a built-in parameter, we have access to a
> > show command and its union setting_variable.  When we have an
> > un-registered Guile param, we don't have a show command associated to
> > it, but we can pass the parameter's pascm_variable union, stored in
> > param_smob.  Because we have these two kinds of incompatible parameters,
> > we need two versions of the pascm_param_value function.
> > 
> > Change-Id: I74a6be2a9188520dbdac4cb123c08e00ebb40a91
> > ---
> >  gdb/cli/cli-cmds.c           |  48 +++++++++-----
> >  gdb/cli/cli-decode.c         | 113 +++++++++++++++++++++++--------
> >  gdb/cli/cli-decode.h         |  31 ++++++++-
> >  gdb/cli/cli-setshow.c        |  87 +++++++++++++-----------
> >  gdb/guile/scm-param.c        | 125 +++++++++++++++++++++++++++++++----
> >  gdb/python/py-param.c        |   8 ++-
> >  gdb/python/python-internal.h |   2 +-
> >  gdb/python/python.c          |  35 +++++++---
> >  gdb/remote.c                 |   2 +-
> >  9 files changed, 340 insertions(+), 111 deletions(-)