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: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.


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