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

Message ID 20210714045520.1623120-16-simon.marchi@polymtl.ca
State New
Headers show
Series
  • Bunch of commands related cleanups
Related show

Commit Message

Lancelot SIX via Gdb-patches July 14, 2021, 4:55 a.m.
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).  Finally, when doing refactorings, such as one later in this
series which changes `char *` to `std::string`, it ensures that we don't
forget to update some spots.

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

-- 
2.32.0

Comments

Lancelot SIX via Gdb-patches July 14, 2021, 12:08 p.m. | #1
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(-)
Lancelot SIX via Gdb-patches July 14, 2021, 5:12 p.m. | #2
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(-)
Lancelot SIX via Gdb-patches July 14, 2021, 7:22 p.m. | #3
On 2021-07-14 1:12 p.m., Lancelot SIX wrote:
>> 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.


Hi Lancelot,

Thanks for taking a look, good to know that somebody else felt the need
to modernize this code.

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


I am certainly for this kind of consistency checks, making sure we
access the right data the right way.  This is what I did with struct
dynamic_prop, for example.

I was thinking of adding accessors too, eventually.  I am not very good
with advanced C++ templates though, so what I had in mind was more
something like the obvious:

  struct cmd_list_element
  {
    int *var_as_int () // or int &
    {
      gdb_assert (m_var_type == a
		  || m_var_type == b);
      return m_var.int_var;
    }

    unsigned int *var_as_unsigned_int () // or unsigned int &
    {
      gdb_assert (m_var_type == c
		  || m_var_type == c);
      return m_var.unsigned_int_var;
    }

    // And so on.
  };

Your last proposition is a bit better in the sense that you ask "give me
the variable location for a var_zuinteger".  With mine, you still have
to know that a var_zuinteger uses the unsigned int member.  But at
least, there's a gdb_assert to catch mistakes at runtime.

Whether it's worth the extra complexity... I can't tell, I would have to
see the complete patch.  I would also have to see how that fits with the
upcoming fix I have for bug 28085.  In the mean time, does it make your
life more difficult if we merge my patch?  I would guess not, since
you'll need to modify all the call sites (places that access setting
variables) anyway.  You can then decide to keep the union or not in your
patch.

Simon
Lancelot SIX via Gdb-patches July 14, 2021, 11:22 p.m. | #4
On Wed, Jul 14, 2021 at 03:22:41PM -0400, Simon Marchi wrote:
> 

> I am certainly for this kind of consistency checks, making sure we

> access the right data the right way.  This is what I did with struct

> dynamic_prop, for example.

> 

> I was thinking of adding accessors too, eventually.  I am not very good

> with advanced C++ templates though, so what I had in mind was more

> something like the obvious:

> 

>   struct cmd_list_element

>   {

>     int *var_as_int () // or int &

>     {

>       gdb_assert (m_var_type == a

> 		  || m_var_type == b);

>       return m_var.int_var;

>     }

> 

>     unsigned int *var_as_unsigned_int () // or unsigned int &

>     {

>       gdb_assert (m_var_type == c

> 		  || m_var_type == c);

>       return m_var.unsigned_int_var;

>     }

> 

>     // And so on.

>   };

> 

> Your last proposition is a bit better in the sense that you ask "give me

> the variable location for a var_zuinteger".  With mine, you still have

> to know that a var_zuinteger uses the unsigned int member.  But at

> least, there's a gdb_assert to catch mistakes at runtime.

> 

> Whether it's worth the extra complexity... I can't tell, I would have to

> see the complete patch.  I would also have to see how that fits with the

> upcoming fix I have for bug 28085.  In the mean time, does it make your

> life more difficult if we merge my patch?  I would guess not, since

> you'll need to modify all the call sites (places that access setting

> variables) anyway.  You can then decide to keep the union or not in your

> patch.

> 

> Simon


I have not yet finished updating all callsites, but here is a
(hopefully) working patch if you want to see what it looks like.

I have not had a look at 28085 yet, so I do not yet if my approach gets
in the way of fixing it.  For the moment, I do not have something
similar to the gdb_optional you introduce, but it should not be too
difficult to have something similar.

Anyway, to not hold back on my account.  I have not read your series
carefully yet, but it seems a real improvement from the current
situation.  If it helps fixing a bug, it is even better.  I can rework
my patch to work on top of this one (at least if what it brings is
considered worthwhile).

Lancelot.

---
From 5b578bb0ed51bf9a7c03cc9b390ec636a49d68d5 Mon Sep 17 00:00:00 2001
From: Lancelot SIX <lsix@lancelotsix.com>

Date: Wed, 14 Jul 2021 22:30:14 +0000
Subject: [PATCH] [WIP] Add typesafe getter/setter for cmd_list_element.var

cmd_list_element can contain a pointer to data that can be set and / or
shown.  This is achieved with a void* that points to the data that can
be accessed, while the var_type (of type enum var_types) tells how to
interpret the data pointed to.

With this pattern, the user needs to know what the storage type
associated with a given VAR_TYPES tag, and needs to do the proper
casting.

This looks something like:

if (c->var_type == var_uinteger)
  unsigned_int v = *(unsigned int*)v->var_type;

With this approach, it is easy to make an error.

This patch is a proof of concept for an alternative approach.

We add templated get_var / set_var member functions to the
cmd_list_element.  The template parameter is the VAR_TYPES we want to
the access the data for.  The template is written so the return type of
the get_var / parameter type of the set_var matches the data type used
to store the actual data.

For example, instantiating the member functions with var_boolean will
be something similar to:

	bool get_var<var_boolean> () const
	{
	  gdb_assert (this->var_type == var_boolean);
	  return *static_cast<bool *> (this->var);
	}
	void set_var<var_boolean> (bool var)
	{
	  gdb_assert (this->var_type == var_boolean);
	  *static_cast<bool *> (this->var) = var;
	}

With those new methods, the caller does not need to know the underlying
storage nor he does need to perform the cast manually.

Given that the processing of some options is common between VAR_TYPES,
the templates can be instantiated with more than one VAR_TYPES.  In such
situation, all the template parameters need to share the same underlying
type.

Here is another example of what an instantiation looks like with 2
parameters:

	int get_var<var_integer, var_zinteger> ()
	{
	  gdb_assert (this->var_type == var_integer
	              || this->var_type == var_zinteger);
	  return *static_cast<int *> (this->var);
	}
	void set_var<var_integer, var_zinteger> (int v)
	{
	  gdb_assert (this->var_type == var_integer
	              || this->var_type == var_zinteger);
	  *static_cast<int *> (this->var) = v;
	}

For convenience reasons, the getter is written in such a way that it
accepts template parameters that have different underlying types, as long
as they are compatible (i.e. can all be implicitly converted to).

	char *get_var<var_string> ()
	{
	  gdb_assert (this->var_type == var_string);
	  return *static_cast<char **> (this->var);
	}
	const char *get_var<var_string, var_filename, var_enum> ()
	{
	  gdb_assert (this->var_type == var_string
	              || this->var_type == var_filename
	              || this->var_type == var_enum);
	  return *static_cast<const char **> (this->var);
	}

Eventually the 'var' and 'var_type' members of cmd_list_element will
have to be made private members to make sure all access is done through
the getter / setter, but since this is a POC, only a few callsites have
been updated.
---
 gdb/cli/cli-decode.c  | 114 ++++++++++++++++----------------
 gdb/cli/cli-decode.h  |  49 ++++++++++++++
 gdb/cli/cli-setshow.c |  91 ++++++++++++++------------
 gdb/command.h         | 147 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 305 insertions(+), 96 deletions(-)

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 633a3ad9309..35ab7c4d015 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -481,12 +481,12 @@ empty_sfunc (const char *args, int from_tty, struct cmd_list_element *c)
    VAR is address of the variable being controlled by this command.
    DOC is the documentation string.  */
 
+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,
-		     var_types var_type,
-		     void *var,
+		     typename var_types_storage<T>::type *var,
 		     const char *doc,
 		     struct cmd_list_element **list)
 {
@@ -494,7 +494,7 @@ add_set_or_show_cmd (const char *name,
 
   gdb_assert (type == set_cmd || type == show_cmd);
   c->type = type;
-  c->var_type = var_type;
+  c->var_type = T;
   c->var = var;
   /* This needs to be something besides NULL so that this isn't
      treated as a help class.  */
@@ -511,10 +511,11 @@ add_set_or_show_cmd (const char *name,
 
    Return the newly created set and show commands.  */
 
+template<var_types T>
 static set_show_commands
 add_setshow_cmd_full (const char *name,
 		      enum command_class theclass,
-		      var_types var_type, void *var,
+		      typename var_types_storage<T>::type *var,
 		      const char *set_doc, const char *show_doc,
 		      const char *help_doc,
 		      cmd_const_sfunc_ftype *set_func,
@@ -537,15 +538,15 @@ add_setshow_cmd_full (const char *name,
       full_set_doc = xstrdup (set_doc);
       full_show_doc = xstrdup (show_doc);
     }
-  set = add_set_or_show_cmd (name, set_cmd, theclass, var_type, var,
-			     full_set_doc, set_list);
+  set = add_set_or_show_cmd<T> (name, set_cmd, theclass, var,
+				full_set_doc, set_list);
   set->doc_allocated = 1;
 
   if (set_func != NULL)
     set_cmd_sfunc (set, set_func);
 
-  show = add_set_or_show_cmd (name, show_cmd, theclass, var_type, var,
-			      full_show_doc, show_list);
+  show = add_set_or_show_cmd<T> (name, show_cmd, theclass, var,
+				 full_show_doc, show_list);
   show->doc_allocated = 1;
   show->show_value_func = show_func;
   /* Disable the default symbol completer.  Doesn't make much sense
@@ -574,10 +575,10 @@ add_setshow_enum_cmd (const char *name,
 		      struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    =  add_setshow_cmd_full (name, theclass, var_enum, var,
-			     set_doc, show_doc, help_doc,
-			     set_func, show_func,
-			     set_list, show_list);
+    =  add_setshow_cmd_full<var_enum> (name, theclass, var,
+				       set_doc, show_doc, help_doc,
+				       set_func, show_func,
+				       set_list, show_list);
   commands.set->enums = enumlist;
   return commands;
 }
@@ -602,10 +603,10 @@ add_setshow_auto_boolean_cmd (const char *name,
 			      struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_auto_boolean, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_auto_boolean> (name, theclass, var,
+					      set_doc, show_doc, help_doc,
+					      set_func, show_func,
+					      set_list, show_list);
 
   commands.set->enums = auto_boolean_enums;
 
@@ -631,10 +632,10 @@ add_setshow_boolean_cmd (const char *name, enum command_class theclass, bool *va
 			 struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_boolean, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_boolean> (name, theclass, var,
+					 set_doc, show_doc, help_doc,
+					 set_func, show_func,
+					 set_list, show_list);
 
   commands.set->enums = boolean_enums;
 
@@ -655,10 +656,10 @@ add_setshow_filename_cmd (const char *name, enum command_class theclass,
 			  struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_filename, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_filename> (name, theclass, var,
+					  set_doc, show_doc, help_doc,
+					  set_func, show_func,
+					  set_list, show_list);
 
   set_cmd_completer (commands.set, filename_completer);
 
@@ -679,10 +680,10 @@ add_setshow_string_cmd (const char *name, enum command_class theclass,
 			struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_string, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_string> (name, theclass, var,
+					set_doc, show_doc, help_doc,
+					set_func, show_func,
+					set_list, show_list);
 
   /* Disable the default symbol completer.  */
   set_cmd_completer (commands.set, nullptr);
@@ -704,10 +705,10 @@ add_setshow_string_noescape_cmd (const char *name, enum command_class theclass,
 				 struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_string_noescape, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_string_noescape> (name, theclass, var,
+						 set_doc, show_doc, help_doc,
+						 set_func, show_func,
+						 set_list, show_list);
 
   /* Disable the default symbol completer.  */
   set_cmd_completer (commands.set, nullptr);
@@ -729,10 +730,10 @@ add_setshow_optional_filename_cmd (const char *name, enum command_class theclass
 				   struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_optional_filename, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_optional_filename> (name, theclass, var,
+						   set_doc, show_doc, help_doc,
+						   set_func, show_func,
+						   set_list, show_list);
 		
   set_cmd_completer (commands.set, filename_completer);
 
@@ -773,10 +774,10 @@ add_setshow_integer_cmd (const char *name, enum command_class theclass,
 			 struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_integer, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_integer> (name, theclass, var,
+					 set_doc, show_doc, help_doc,
+					 set_func, show_func,
+					 set_list, show_list);
 
   set_cmd_completer (commands.set, integer_unlimited_completer);
 
@@ -799,10 +800,10 @@ add_setshow_uinteger_cmd (const char *name, enum command_class theclass,
 			  struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_uinteger, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_uinteger> (name, theclass, var,
+					  set_doc, show_doc, help_doc,
+					  set_func, show_func,
+					  set_list, show_list);
 
   set_cmd_completer (commands.set, integer_unlimited_completer);
 
@@ -824,10 +825,10 @@ add_setshow_zinteger_cmd (const char *name, enum command_class theclass,
 			  struct cmd_list_element **set_list,
 			  struct cmd_list_element **show_list)
 {
-  return add_setshow_cmd_full (name, theclass, var_zinteger, var,
-			       set_doc, show_doc, help_doc,
-			       set_func, show_func,
-			       set_list, show_list);
+  return add_setshow_cmd_full<var_zinteger> (name, theclass, var,
+					     set_doc, show_doc, help_doc,
+					     set_func, show_func,
+					     set_list, show_list);
 }
 
 set_show_commands
@@ -843,10 +844,11 @@ add_setshow_zuinteger_unlimited_cmd (const char *name,
 				     struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_zuinteger_unlimited, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_zuinteger_unlimited> (name, theclass, var,
+						     set_doc, show_doc,
+						     help_doc, set_func,
+						     show_func, set_list,
+						     show_list);
 
   set_cmd_completer (commands.set, integer_unlimited_completer);
 
@@ -868,10 +870,10 @@ add_setshow_zuinteger_cmd (const char *name, enum command_class theclass,
 			   struct cmd_list_element **set_list,
 			   struct cmd_list_element **show_list)
 {
-  return add_setshow_cmd_full (name, theclass, var_zuinteger, var,
-			       set_doc, show_doc, help_doc,
-			       set_func, show_func,
-			       set_list, show_list);
+  return add_setshow_cmd_full<var_zuinteger> (name, theclass, var,
+					      set_doc, show_doc, help_doc,
+					      set_func, show_func,
+					      set_list, show_list);
 }
 
 /* Remove the command named NAME from the command list.  Return the
diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h
index 241535ae5b5..d54dff5a997 100644
--- a/gdb/cli/cli-decode.h
+++ b/gdb/cli/cli-decode.h
@@ -26,6 +26,7 @@
 #include "gdb_regex.h"
 #include "cli-script.h"
 #include "completer.h"
+#include <type_traits>
 
 /* Not a set/show command.  Note that some commands which begin with
    "set" or "show" might be in this category, if their syntax does
@@ -102,6 +103,54 @@ struct cmd_list_element
   void *context () const
   { return m_context; }
 
+  /* Return the value of the current element that can be shown.
+     The expected template parameter is the VAR_TYPES of the current instance
+     is.  This is enforced with a runtime check.
+
+     If multiple template parameters are given, select the approapriate return
+     type that satisfies all possibilities.  If such type does not exist, the
+     instantiation will fail.  A runtime check is made to ensure that the
+     current instance's type is one of those given as template parameter.  */
+  template<var_types... Ts>
+  typename std::common_type<typename var_types_storage<Ts>::type...>::type get_var() const
+  {
+    gdb_assert (var_types_storage_helper<Ts...>::covers_type (this->var_type));
+
+    return *static_cast<
+      typename std::common_type<
+	typename var_types_storage<Ts>::type...>::type *> (this->var);
+  }
+
+  /* Update the enclosed VAR to the value given as a parameter.
+
+     If one template argument is given, it must be the VAR_TYPE of the current
+     instance.  This is enforced at runtime.
+     If multiple template parameters are given, they must all share the same
+     underlying type (this is checked at compile time), and THIS must be of
+     the type of one of the template parameters (this is checked at runtime).
+     */
+  template<var_types... Ts>
+  void set_var(typename var_types_storage_helper<Ts...>::type v)
+    {
+      /* Ensure that all Ts share the same underlying type.  */
+      static_assert (var_types_storage_helper<Ts...>::all_same);
+
+      /* Check that the current instance is of one of the supported types for
+         this instantiation.  */
+      gdb_assert (var_types_storage_helper<Ts...>::covers_type (this->var_type));
+
+      *static_cast<typename var_types_storage_helper<Ts...>::type *> (this->var)
+	= v;
+    }
+
+  /* Update the pointer to the VAR referenced by this instance.  */
+  template<var_types T>
+  void set_var_p (typename var_types_storage<T>::type *v)
+  {
+    this->type = T;
+    this->var = static_cast<void *> (v);
+  }
+
   /* Points to next command in this list.  */
   struct cmd_list_element *next = nullptr;
 
diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index 5fd5fd15c6a..b5931fe0e2e 100644
--- a/gdb/cli/cli-setshow.c
+++ b/gdb/cli/cli-setshow.c
@@ -353,11 +353,11 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	*q++ = '\0';
 	newobj = (char *) xrealloc (newobj, q - newobj);
 
-	if (*(char **) c->var == NULL
-	    || strcmp (*(char **) c->var, newobj) != 0)
+	if (c->get_var<var_string> () == nullptr
+	    || strcmp (c->get_var<var_string> (), newobj) != 0)
 	  {
-	    xfree (*(char **) c->var);
-	    *(char **) c->var = newobj;
+	    xfree (c->get_var<var_string> ());
+	    c->set_var<var_string> (newobj);
 
 	    option_changed = 1;
 	  }
@@ -366,10 +366,11 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
       }
       break;
     case var_string_noescape:
-      if (*(char **) c->var == NULL || strcmp (*(char **) c->var, arg) != 0)
+      if (c->get_var<var_string_noescape> () == nullptr
+	  || strcmp (c->get_var<var_string_noescape> (), arg) != 0)
 	{
-	  xfree (*(char **) c->var);
-	  *(char **) c->var = xstrdup (arg);
+	  xfree (c->get_var<var_string_noescape> ());
+	  c->set_var<var_string_noescape> (xstrdup (arg));
 
 	  option_changed = 1;
 	}
@@ -398,11 +399,11 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	else
 	  val = xstrdup ("");
 
-	if (*(char **) c->var == NULL
-	    || strcmp (*(char **) c->var, val) != 0)
+	if (c->get_var<var_optional_filename> () == nullptr
+	    || strcmp (c->get_var<var_optional_filename> (), val) != 0)
 	  {
-	    xfree (*(char **) c->var);
-	    *(char **) c->var = val;
+	    xfree (c->get_var<var_optional_filename> ());
+	    c->set_var<var_optional_filename> (val);
 
 	    option_changed = 1;
 	  }
@@ -416,9 +417,9 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
 	if (val < 0)
 	  error (_("\"on\" or \"off\" expected."));
-	if (val != *(bool *) c->var)
+	if (val != c->get_var<var_boolean> ())
 	  {
-	    *(bool *) c->var = val;
+	    c->set_var<var_boolean> (val);
 
 	    option_changed = 1;
 	  }
@@ -428,9 +429,9 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
       {
 	enum auto_boolean val = parse_auto_binary_operation (arg);
 
-	if (*(enum auto_boolean *) c->var != val)
+	if (c->get_var<var_auto_boolean> () != val)
 	  {
-	    *(enum auto_boolean *) c->var = val;
+	    c->set_var<var_auto_boolean> (val);
 
 	    option_changed = 1;
 	  }
@@ -441,9 +442,9 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
       {
 	unsigned int val = parse_cli_var_uinteger (c->var_type, &arg, true);
 
-	if (*(unsigned int *) c->var != val)
+	if (c->get_var<var_uinteger, var_zuinteger> () != val)
 	  {
-	    *(unsigned int *) c->var = val;
+	    c->set_var<var_uinteger, var_zuinteger> (val);
 
 	    option_changed = 1;
 	  }
@@ -477,9 +478,9 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 		 || (c->var_type == var_zinteger && val > INT_MAX))
 	  error (_("integer %s out of range"), plongest (val));
 
-	if (*(int *) c->var != val)
+	if (c->get_var<var_integer, var_zinteger> () != val)
 	  {
-	    *(int *) c->var = val;
+	    c->set_var<var_integer, var_zinteger> (val);
 
 	    option_changed = 1;
 	  }
@@ -495,9 +496,9 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	if (*after != '\0')
 	  error (_("Junk after item \"%.*s\": %s"), len, arg, after);
 
-	if (*(const char **) c->var != match)
+	if (c->get_var<var_enum> () != match)
 	  {
-	    *(const char **) c->var = match;
+	    c->set_var<var_enum> (match);
 
 	    option_changed = 1;
 	  }
@@ -507,9 +508,9 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
       {
 	int val = parse_cli_var_zuinteger_unlimited (&arg, true);
 
-	if (*(int *) c->var != val)
+	if (c->get_var<var_zuinteger_unlimited> () != val)
 	  {
-	    *(int *) c->var = val;
+	    c->set_var<var_zuinteger_unlimited> (val);
 	    option_changed = 1;
 	  }
       }
@@ -584,18 +585,24 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	case var_filename:
 	case var_optional_filename:
 	case var_enum:
-	  gdb::observers::command_param_changed.notify (name, *(char **) c->var);
+	  gdb::observers::command_param_changed.notify
+	    (name,
+	     c->get_var<var_string,
+			var_string_noescape,
+			var_filename,
+			var_optional_filename,
+			var_enum> ());
 	  break;
 	case var_boolean:
 	  {
-	    const char *opt = *(bool *) c->var ? "on" : "off";
+	    const char *opt = c->get_var<var_boolean> () ? "on" : "off";
 
 	    gdb::observers::command_param_changed.notify (name, opt);
 	  }
 	  break;
 	case var_auto_boolean:
 	  {
-	    const char *s = auto_boolean_enums[*(enum auto_boolean *) c->var];
+	    const char *s = auto_boolean_enums[c->get_var<var_auto_boolean> ()];
 
 	    gdb::observers::command_param_changed.notify (name, s);
 	  }
@@ -605,7 +612,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	  {
 	    char s[64];
 
-	    xsnprintf (s, sizeof s, "%u", *(unsigned int *) c->var);
+	    xsnprintf (s, sizeof s, "%u", c->get_var<var_uinteger, var_zuinteger> ());
 	    gdb::observers::command_param_changed.notify (name, s);
 	  }
 	  break;
@@ -615,7 +622,8 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	  {
 	    char s[64];
 
-	    xsnprintf (s, sizeof s, "%d", *(int *) c->var);
+	    xsnprintf (s, sizeof s, "%d",
+		       c->get_var<var_integer, var_zinteger, var_zuinteger_unlimited> ());
 	    gdb::observers::command_param_changed.notify (name, s);
 	  }
 	  break;
@@ -634,21 +642,24 @@ get_setshow_command_value_string (const cmd_list_element *c)
   switch (c->var_type)
     {
     case var_string:
-      if (*(char **) c->var)
-	stb.putstr (*(char **) c->var, '"');
+      if (c->get_var<var_string> () != nullptr)
+	stb.putstr (c->get_var<var_string> (), '"');
       break;
     case var_string_noescape:
     case var_optional_filename:
     case var_filename:
     case var_enum:
-      if (*(char **) c->var)
-	stb.puts (*(char **) c->var);
+      if (c->get_var<var_string_noescape,
+		     var_optional_filename,
+		     var_filename,
+		     var_enum> () != nullptr)
+	stb.puts (c->get_var<var_enum> ());
       break;
     case var_boolean:
-      stb.puts (*(bool *) c->var ? "on" : "off");
+      stb.puts (c->get_var<var_boolean> () ? "on" : "off");
       break;
     case var_auto_boolean:
-      switch (*(enum auto_boolean*) c->var)
+      switch (c->get_var<var_auto_boolean> ())
 	{
 	case AUTO_BOOLEAN_TRUE:
 	  stb.puts ("on");
@@ -667,25 +678,25 @@ get_setshow_command_value_string (const cmd_list_element *c)
     case var_uinteger:
     case var_zuinteger:
       if (c->var_type == var_uinteger
-	  && *(unsigned int *) c->var == UINT_MAX)
+	  && c->get_var<var_uinteger> () == UINT_MAX)
 	stb.puts ("unlimited");
       else
-	stb.printf ("%u", *(unsigned int *) c->var);
+	stb.printf ("%u", c->get_var<var_uinteger, var_zuinteger> ());
       break;
     case var_integer:
     case var_zinteger:
       if (c->var_type == var_integer
-	  && *(int *) c->var == INT_MAX)
+	  && c->get_var<var_integer> () == INT_MAX)
 	stb.puts ("unlimited");
       else
-	stb.printf ("%d", *(int *) c->var);
+	stb.printf ("%d", c->get_var<var_integer, var_zinteger> ());
       break;
     case var_zuinteger_unlimited:
       {
-	if (*(int *) c->var == -1)
+	if (c->get_var<var_zuinteger_unlimited> () == -1)
 	  stb.puts ("unlimited");
 	else
-	  stb.printf ("%d", *(int *) c->var);
+	  stb.printf ("%d", c->get_var<var_zuinteger_unlimited> ());
       }
       break;
     default:
diff --git a/gdb/command.h b/gdb/command.h
index 711cbdcf43e..8fc36e8eb5e 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -125,6 +125,153 @@ typedef enum var_types
   }
 var_types;
 
+/* Helper classes used to associate a storage type for each possible
+   var_type. */
+
+template<var_types T>
+struct var_types_storage;
+
+template<>
+struct var_types_storage<var_boolean>
+{
+  using type = bool;
+};
+
+template<>
+struct var_types_storage<var_auto_boolean>
+{
+  using type = enum auto_boolean;
+};
+
+template<>
+struct var_types_storage<var_uinteger>
+{
+  using type = unsigned int;
+};
+
+template<>
+struct var_types_storage<var_integer>
+{
+  using type = int;
+};
+
+template<>
+struct var_types_storage<var_string>
+{
+  using type = char *;
+};
+
+template<>
+struct var_types_storage<var_string_noescape>
+{
+  using type = char *;
+};
+
+template<>
+struct var_types_storage<var_optional_filename>
+{
+  using type = char *;
+};
+
+template<>
+struct var_types_storage<var_filename>
+{
+  using type = char *;
+};
+
+template<>
+struct var_types_storage<var_zinteger>
+{
+  using type = int;
+};
+
+template<>
+struct var_types_storage<var_zuinteger>
+{
+  using type = unsigned int;
+};
+
+template<>
+struct var_types_storage<var_zuinteger_unlimited>
+{
+  using type = int;
+};
+
+template<>
+struct var_types_storage<var_enum>
+{
+  using type = const char *;
+};
+
+/* Helper class used to check if multiple var_types are represented
+   using the same underlying type.  This class is meant to be instantiated
+   using any number of var_types, and will be used to assess common properties
+   of the underlying storage type.
+
+   Each template instantiating will define the following static members:
+	- all_same: True if and only if all the var_types are stored on the same
+	underlying storage type.
+	- covers_type (var_types t): True if and only if the parameter T is one
+	the templates parameter.
+	- type: Type alias of the underlying type if all_same is true, unspecified
+	otherwise.
+  */
+
+template<var_types... Ts>
+struct var_types_storage_helper;
+
+/* Specialization of var_types_storage_helper when instantiated with only 1
+   template parameter.  */
+template<var_types T>
+struct var_types_storage_helper<T>
+{
+  static constexpr bool all_same = true;
+
+  using type = typename var_types_storage<T>::type;
+
+  static constexpr bool covers_type (var_types t)
+    {
+      return t == T;
+    }
+};
+
+/* Specialization of var_types_storage_helper when instantiated with exactly
+   2 template parameters.  */
+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>::type,
+		   typename var_types_storage<U>::type>::value;
+
+  using type = typename var_types_storage<T>::type;
+
+  static constexpr bool covers_type (var_types t)
+  {
+    return var_types_storage_helper<T>::covers_type (t)
+      || var_types_storage_helper<U>::covers_type (t);
+  }
+};
+
+/* Specialization of var_types_storage_helper when instantiated with 3 or more
+   template parameters.  */
+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>::type;
+
+  static constexpr bool covers_type (var_types t)
+    {
+      return var_types_storage_helper<T>::covers_type (t)
+	|| var_types_storage_helper<U, Us...>::covers_type (t);
+    }
+};
+
+
 /* This structure records one command'd definition.  */
 struct cmd_list_element;
 
-- 
2.30.2
Lancelot SIX via Gdb-patches July 18, 2021, 3:44 p.m. | #5
> 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.

> 


Hi,

I believe you can create an intermediate abstraction that works for
both the registered and unregistered settings.  In both cases, you can
use or build a cmd_list_element::setting_variable easily, and then have
only one function that does the conversion to a SCM value logic.

Given how a pascm_value is laid out (i.e. a union), you can create a
cmd_list_element setting_variable referencing it with something like:

const cmd_list_element::setting_variable v { .bool_var = &param_value->boolval };

This solution is not perfect either, and it forces the const qualifier
of the param_value of pascm_param_value to be dropped, but this avoids
code duplication which I think is good for easier maintenance.

What do you think?

I have built the above changes on top of your patch series (up to this
patch) and tested gdb.guile/*.exp with no regression noticed.

Lancelot.

---
diff --git a/gdb/guile/scm-param.c b/gdb/guile/scm-param.c
index 57a8c7ec68a..9dc4a5d910d 100644
--- a/gdb/guile/scm-param.c
+++ b/gdb/guile/scm-param.c
@@ -113,6 +113,8 @@ struct param_smob
   SCM containing_scm;
 };
 
+using setting_variable = cmd_list_element::setting_variable;
+
 static const char param_smob_name[] = "gdb:parameter";
 
 /* The tag Guile knows the param smob by.  */
@@ -133,8 +135,11 @@ static SCM unlimited_keyword;
 
 static int pascm_is_valid (param_smob *);
 static const char *pascm_param_type_name (enum var_types type);
-static SCM pascm_param_value (enum var_types type,
-			      const pascm_variable *var,
+static SCM pascm_param_value (enum var_types type, pascm_variable *var,
+			      int arg_pos, const char *func_name);
+static SCM pascm_param_value (const cmd_list_element *,
+			      int arg_pos, const char *func_name);
+static SCM pascm_param_value (enum var_types type, const setting_variable v,
 			      int arg_pos, const char *func_name);
 
 /* Administrivia for parameter smobs.  */
@@ -570,79 +575,11 @@ pascm_param_type_name (enum var_types param_type)
    haven't been registered yet.  */
 
 static SCM
-pascm_param_value (enum var_types type, const pascm_variable *param_value,
+pascm_param_value (enum var_types type, pascm_variable *param_value,
 		   int arg_pos, const char *func_name)
 {
-  /* Note: We *could* support var_integer here in case someone is trying to get
-     the value of a Python-created parameter (which is the only place that
-     still supports var_integer).  To further discourage its use we do not.  */
-
-  switch (type)
-    {
-    case var_string:
-    case var_string_noescape:
-    case var_optional_filename:
-    case var_filename:
-      {
-	const char *str = param_value->stringval;
-
-	if (str == NULL)
-	  str = "";
-
-	return gdbscm_scm_from_host_string (str, strlen (str));
-      }
-
-    case var_enum:
-      {
-	const char *str = param_value->cstringval;
-
-	if (str == NULL)
-	  str = "";
-	return gdbscm_scm_from_host_string (str, strlen (str));
-      }
-
-    case var_boolean:
-      {
-	if (param_value->boolval)
-	  return SCM_BOOL_T;
-	else
-	  return SCM_BOOL_F;
-      }
-
-    case var_auto_boolean:
-      {
-	enum auto_boolean ab = param_value->autoboolval;
-
-	if (ab == AUTO_BOOLEAN_TRUE)
-	  return SCM_BOOL_T;
-	else if (ab == AUTO_BOOLEAN_FALSE)
-	  return SCM_BOOL_F;
-	else
-	  return auto_keyword;
-      }
-
-    case var_zuinteger_unlimited:
-      if (param_value->intval == -1)
-	return unlimited_keyword;
-      gdb_assert (param_value->intval >= 0);
-      /* Fall through.  */
-    case var_zinteger:
-      return scm_from_int (param_value->intval);
-
-    case var_uinteger:
-      if (param_value->uintval == UINT_MAX)
-	return unlimited_keyword;
-      /* Fall through.  */
-    case var_zuinteger:
-      return scm_from_uint (param_value->uintval);
-
-    default:
-      break;
-    }
-
-  return gdbscm_make_out_of_range_error (func_name, arg_pos,
-					 scm_from_int (type),
-					 _("program error: unhandled type"));
+  setting_variable v { .bool_var = &param_value->boolval };
+  return pascm_param_value (type, v, arg_pos, func_name);
 }
 
 /* Return the value of a gdb parameter as a Scheme value.
@@ -655,33 +592,37 @@ pascm_param_value (enum var_types type, const pascm_variable *param_value,
 static SCM
 pascm_param_value (const cmd_list_element *show_cmd,
 		   int arg_pos, const char *func_name)
+{
+  gdb_assert (show_cmd->type == cmd_types::show_cmd);
+  gdb_assert (show_cmd->var.has_value ());
+  return pascm_param_value(show_cmd->var_type, *show_cmd->var,
+			   arg_pos, func_name);
+}
+
+/* Return the value of a gdb parameter as a Scheme value.
+
+   contains the common code paths to handle a variable wether is has been
+   registered or not. */
+
+static SCM
+pascm_param_value (enum var_types var_type, const setting_variable var,
+		   int arg_pos, const char *func_name)
 {
   /* Note: We *could* support var_integer here in case someone is trying to get
      the value of a Python-created parameter (which is the only place that
      still supports var_integer).  To further discourage its use we do not.  */
 
-  gdb_assert (show_cmd->type == cmd_types::show_cmd);
-
-  switch (show_cmd->var_type)
+  switch (var_type)
     {
     case var_string:
     case var_string_noescape:
     case var_optional_filename:
     case var_filename:
-      {
-	const char *str = *show_cmd->var->char_ptr_var;
-
-	if (str == NULL)
-	  str = "";
-
-	return gdbscm_scm_from_host_string (str, strlen (str));
-      }
-
     case var_enum:
       {
-	const char *str = *show_cmd->var->const_char_ptr_var;
+	const char *str = *var.const_char_ptr_var;
 
-	if (str == NULL)
+	if (str == nullptr)
 	  str = "";
 
 	return gdbscm_scm_from_host_string (str, strlen (str));
@@ -689,7 +630,7 @@ pascm_param_value (const cmd_list_element *show_cmd,
 
     case var_boolean:
       {
-	if (*show_cmd->var->bool_var)
+	if (*var.bool_var)
 	  return SCM_BOOL_T;
 	else
 	  return SCM_BOOL_F;
@@ -697,7 +638,7 @@ pascm_param_value (const cmd_list_element *show_cmd,
 
     case var_auto_boolean:
       {
-	enum auto_boolean ab = *show_cmd->var->auto_boolean_var;
+	enum auto_boolean ab = *var.auto_boolean_var;
 
 	if (ab == AUTO_BOOLEAN_TRUE)
 	  return SCM_BOOL_T;
@@ -708,26 +649,26 @@ pascm_param_value (const cmd_list_element *show_cmd,
       }
 
     case var_zuinteger_unlimited:
-      if (*show_cmd->var->int_var == -1)
+      if (*var.int_var == -1)
 	return unlimited_keyword;
-      gdb_assert (*show_cmd->var->int_var >= 0);
+      gdb_assert (*var.int_var >= 0);
       /* Fall through.  */
     case var_zinteger:
-      return scm_from_int (*show_cmd->var->int_var);
+      return scm_from_int (*var.int_var);
 
     case var_uinteger:
-      if (*show_cmd->var->unsigned_int_var == UINT_MAX)
+      if (*var.unsigned_int_var == UINT_MAX)
 	return unlimited_keyword;
       /* Fall through.  */
     case var_zuinteger:
-      return scm_from_uint (*show_cmd->var->unsigned_int_var);
+      return scm_from_uint (*var.unsigned_int_var);
 
     default:
       break;
     }
 
   return gdbscm_make_out_of_range_error (func_name, arg_pos,
-					 scm_from_int (show_cmd->var_type),
+					 scm_from_int (var_type),
 					 _("program error: unhandled type"));
 }
Lancelot SIX via Gdb-patches July 19, 2021, 2:19 p.m. | #6
> +  setting_variable v { .bool_var = &param_value->boolval };

> +  return pascm_param_value (type, v, arg_pos, func_name);


I thought about doing something like this, my only itch is that
declaration of `setting_variable v`.  It is confusing to see it just
setting the bool_var field, regardless of the actual type.  I understand
it works because the pointer value is the same in any case, but to be
pedantic it should be something like:

  setting_variable v;
  switch (type)
    {
    case var_boolean:
      v.bool_var = &param_value->boolval;
      break;
    case var_auto_boolean:
      v.auto_boolean_var = &param_value->autoboolval;
      break;
    // and so on
    }

It's a bit more verbose, but it's written only once, and it's not like
new var types are added every week.

I would be fine changing my patch to use your approach with that change,
if you are.

Simon
Lancelot SIX via Gdb-patches July 19, 2021, 2:32 p.m. | #7
On 2021-07-14 7:22 p.m., Lancelot SIX wrote:
> On Wed, Jul 14, 2021 at 03:22:41PM -0400, Simon Marchi wrote:

>>

>> I am certainly for this kind of consistency checks, making sure we

>> access the right data the right way.  This is what I did with struct

>> dynamic_prop, for example.

>>

>> I was thinking of adding accessors too, eventually.  I am not very good

>> with advanced C++ templates though, so what I had in mind was more

>> something like the obvious:

>>

>>   struct cmd_list_element

>>   {

>>     int *var_as_int () // or int &

>>     {

>>       gdb_assert (m_var_type == a

>> 		  || m_var_type == b);

>>       return m_var.int_var;

>>     }

>>

>>     unsigned int *var_as_unsigned_int () // or unsigned int &

>>     {

>>       gdb_assert (m_var_type == c

>> 		  || m_var_type == c);

>>       return m_var.unsigned_int_var;

>>     }

>>

>>     // And so on.

>>   };

>>

>> Your last proposition is a bit better in the sense that you ask "give me

>> the variable location for a var_zuinteger".  With mine, you still have

>> to know that a var_zuinteger uses the unsigned int member.  But at

>> least, there's a gdb_assert to catch mistakes at runtime.

>>

>> Whether it's worth the extra complexity... I can't tell, I would have to

>> see the complete patch.  I would also have to see how that fits with the

>> upcoming fix I have for bug 28085.  In the mean time, does it make your

>> life more difficult if we merge my patch?  I would guess not, since

>> you'll need to modify all the call sites (places that access setting

>> variables) anyway.  You can then decide to keep the union or not in your

>> patch.

>>

>> Simon

> 

> I have not yet finished updating all callsites, but here is a

> (hopefully) working patch if you want to see what it looks like.

> 

> I have not had a look at 28085 yet, so I do not yet if my approach gets

> in the way of fixing it.  For the moment, I do not have something

> similar to the gdb_optional you introduce, but it should not be too

> difficult to have something similar.

> 

> Anyway, to not hold back on my account.  I have not read your series

> carefully yet, but it seems a real improvement from the current

> situation.  If it helps fixing a bug, it is even better.  I can rework

> my patch to work on top of this one (at least if what it brings is

> considered worthwhile).

> 

> Lancelot.


I took a quick look, and that seems reasonable to me.  I don't think it
will conflict (in design) with my upcoming patch.  My upcoming patch
makes it so that some settings don't use the storage that is in
cmd_list_element at all, but instead provide some getter/setter
callback.  This is to fix display of settings where the "source of
truth" is not cmd_list_element::var.

With my patch, the code at various places becomes, conceptually:

  if (cmd_list_element::var is set)
    use cmd_list_element::var
  else
    use the getter/setter

I think it would be nice to make the first case (using
cmd_list_element::var for storage) just a specific case of using the
getter/setter.  That is, when we create a setting that uses
cmd_list_element::var for storage, we install a getter and a setter than
get and set the value in cmd_list_element::var.  But I haven't gotten
there yet.  You'll probably have some good ideas for achieving this :).

But before posting that patch, I'd like to decide how the current patch
series will end up, because that will very much affect how the next
patch will look like.

Simon
Lancelot SIX via Gdb-patches July 19, 2021, 7:52 p.m. | #8
> I took a quick look, and that seems reasonable to me.  I don't think it

> will conflict (in design) with my upcoming patch.  My upcoming patch

> makes it so that some settings don't use the storage that is in

> cmd_list_element at all, but instead provide some getter/setter

> callback.  This is to fix display of settings where the "source of

> truth" is not cmd_list_element::var.

> 

> With my patch, the code at various places becomes, conceptually:

> 

>   if (cmd_list_element::var is set)

>     use cmd_list_element::var

>   else

>     use the getter/setter

> 

> I think it would be nice to make the first case (using

> cmd_list_element::var for storage) just a specific case of using the

> getter/setter.  That is, when we create a setting that uses

> cmd_list_element::var for storage, we install a getter and a setter than

> get and set the value in cmd_list_element::var.  But I haven't gotten

> there yet.  You'll probably have some good ideas for achieving this :).

> 

> But before posting that patch, I'd like to decide how the current patch

> series will end up, because that will very much affect how the next

> patch will look like.


I realize I wasn't accurate here.  The storage isn't actually in
cmd_list_element::var, cmd_list_element::var only contains pointers to
the storage.  But the rest still stands.

Simon
Lancelot SIX via Gdb-patches July 19, 2021, 8:58 p.m. | #9
On Mon, Jul 19, 2021 at 10:19:21AM -0400, Simon Marchi wrote:
> > +  setting_variable v { .bool_var = &param_value->boolval };

> > +  return pascm_param_value (type, v, arg_pos, func_name);

> 

> I thought about doing something like this, my only itch is that

> declaration of `setting_variable v`.  It is confusing to see it just

> setting the bool_var field, regardless of the actual type.  I understand

> it works because the pointer value is the same in any case, but to be

> pedantic it should be something like:

> 

>   setting_variable v;

>   switch (type)

>     {

>     case var_boolean:

>       v.bool_var = &param_value->boolval;

>       break;

>     case var_auto_boolean:

>       v.auto_boolean_var = &param_value->autoboolval;

>       break;

>     // and so on

>     }

> 

> It's a bit more verbose, but it's written only once, and it's not like

> new var types are added every week.

> 

> I would be fine changing my patch to use your approach with that change,

> if you are.

> 

> Simon


When sending my comment, I was wondering about suggesting just that.  It
is definitely more verbose, but I would not mind that.  I tend to prefer
verbose code over code duplication.  It is too easy to update only one
of the two implementations and have behaviors diverge.

Just out of curiosity, I have checked with Compiler Explorer[1], and
starting with -01, the entire switch goes away (when using
__builtin_unreachable, without it the switch is dropped but a check is
maintained).  This is surely not a hot code path so this does not
changes much, but it always nice to know the compiler can do the clever
thing.

Lancelot.

[1] https://godbolt.org/z/GdqGno6cq
Lancelot SIX via Gdb-patches July 20, 2021, 11:03 p.m. | #10
On Mon, Jul 19, 2021 at 03:52:30PM -0400, Simon Marchi wrote:
> > I took a quick look, and that seems reasonable to me.  I don't think it

> > will conflict (in design) with my upcoming patch.  My upcoming patch

> > makes it so that some settings don't use the storage that is in

> > cmd_list_element at all, but instead provide some getter/setter

> > callback.  This is to fix display of settings where the "source of

> > truth" is not cmd_list_element::var.

> > 

> > With my patch, the code at various places becomes, conceptually:

> > 

> >   if (cmd_list_element::var is set)

> >     use cmd_list_element::var

> >   else

> >     use the getter/setter

> > 

> > I think it would be nice to make the first case (using

> > cmd_list_element::var for storage) just a specific case of using the

> > getter/setter.  That is, when we create a setting that uses

> > cmd_list_element::var for storage, we install a getter and a setter than

> > get and set the value in cmd_list_element::var.  But I haven't gotten

> > there yet.  You'll probably have some good ideas for achieving this :).

> > 

> > But before posting that patch, I'd like to decide how the current patch

> > series will end up, because that will very much affect how the next

> > patch will look like.

> 

> I realize I wasn't accurate here.  The storage isn't actually in

> cmd_list_element::var, cmd_list_element::var only contains pointers to

> the storage.  But the rest still stands.

> 

> Simon


Hi,

I reworked a bit my previous implementation to see to what extent it can
support what you are looking for.  Short answer: it can (and it is
completely transparent to the caller).

In the previous draft version[1], I introduced a struct that groups
together the var_type and the pointer to the actual data in order to
abstract a setting (i.e. a value with a type that can be SET or SHOWn).
This struct provides getters and setters in order to enforce that we
cast the pointer to the adequate type.  I have modified this data
structure so we can register user-provided callbacks.  If those
functions are provided, the getter and setters will call them to
generate retrieve the value instead of trying to dereference the
pointer.

For the moment, I just made sure that this compiles and I played with
dummy settings in my local tree.  It is not yet finalized nor fully
operational because do_set_command will try to free the pointer returned
by the getter when dealing with char* data.  With user provided
callbacks memory should probably not be managed at that level but by the
getter / setter.  Nothing worrying, you have a commit that plans to
change the char* with std::string, this should solve the issue quite
nicely.

As for the previous version, this adds a significant amount of templated
code that can make the implementation appear quite complex.

I think I’ll wait for your series to land before I finalize this
proposal (this is just a POC for the moment) unless you want to build on
top of this approach to introduce user provided callbacks.

Best,
Lancelot.

[1] https://sourceware.org/pipermail/gdb-patches/2021-July/180991.html

---

From 2217aeca7a963da7128311758fe3bbeddeaf43bd Mon Sep 17 00:00:00 2001
From: Lancelot SIX <lsix@lancelotsix.com>

Date: Wed, 14 Jul 2021 22:30:14 +0000
Subject: [RFC PATCH v2] Add typesafe getter/setter for cmd_list_element.var

cmd_list_element can contain a pointer to data that can be set and / or
shown.  This is achieved with a void* that points to the data that can
be accessed, while the var_type (of type enum var_types) tells how to
interpret the data pointed to.

With this pattern, the user needs to know what the storage type
associated with a given VAR_TYPES tag, and needs to do the proper
casting.

This looks something like:

	if (c->var_type == var_zuinteger)
	  unsigned int v = *(unsigned int*)v->var_type;

With this approach, it is easy to make an error.

This patch aims at addressing the same problem as
https://sourceware.org/pipermail/gdb-patches/2021-July/180920.html but
uses a different approach.  I am happy rebasing my work on top of the
mentioned series if it is considered a good enough improvement (and I am
happy dropping otherwise).

This patch proposes to add an abstraction around the pair of the
var_type and the void* pointer in order to prevent the user having to
handle the cast.  This is achieved by introducing the setting struct,
and a set of templated member functions.  The template parameter is the
VAR_TYPES we want to the access the data for.  The template ensures the
return type of the get method and the parameter type of the set method
matches the data type used to store the actual data.

For example, instantiating the member functions with var_boolean will
yield something similar to:

	bool get<var_boolean> () const
	{
	  gdb_assert (this->m_var_type == var_boolean);
	  gdb_assert (this->m_var != nullptr);
	  return *static_cast<bool *> (this->m_var);
	}
	void set<var_boolean> (bool var)
	{
	  gdb_assert (this->m_var_type == var_boolean);
	  gdb_assert (this->m_var != nullptr);
	  *static_cast<bool *> (this->m_var) = var;
	}

With those new methods, the caller does not need to know the underlying
storage nor does he need to perform the cast manually.  This combined
with the added dynamic checks (gdb_assert) makes this approach way safer
in my opinion.

Given that the processing of some options is common between VAR_TYPES,
the templates can be instantiated with more than one VAR_TYPES.  In such
situation, all the template parameters need to describe options that
share the same underlying storage type.

Here is another example of what an instantiation looks like with 2
parameters:

	int get<var_integer, var_zinteger> ()
	{
	  gdb_assert (this->m_var_type == var_integer
	              || this->m_var_type == var_zinteger);
	  gdb_assert (this->m_var != nullptr);
	  return *static_cast<int *> (this->m_var);
	}
	void set<var_integer, var_zinteger> (int v)
	{
	  gdb_assert (this->m_var_type == var_integer
	              || this->m_var_type == var_zinteger);
	  gdb_assert (this->m_var != nullptr);
	  *static_cast<int *> (this->m_var) = v;
	}

With such abstractions available, the var_type and var members of the
cmd_list_element are replaced with a setting instance.

No user visible change is expected after this patch.

Tested on GNU/Linux x86_64, with no regression noticed.
---
 gdb/auto-load.c              |   2 +-
 gdb/cli/cli-cmds.c           |  56 +++--
 gdb/cli/cli-decode.c         | 115 +++++-----
 gdb/cli/cli-decode.h         |   6 +-
 gdb/cli/cli-setshow.c        | 156 ++++++++-----
 gdb/command.h                | 418 +++++++++++++++++++++++++++++++++++
 gdb/guile/scm-param.c        | 145 +++++++-----
 gdb/maint.c                  |   2 +-
 gdb/python/py-param.c        |  16 +-
 gdb/python/python-internal.h |   2 +-
 gdb/python/python.c          |  35 +--
 gdb/remote.c                 |   2 +-
 12 files changed, 741 insertions(+), 214 deletions(-)

diff --git a/gdb/auto-load.c b/gdb/auto-load.c
index 9cd70f174c3..033c448eff7 100644
--- a/gdb/auto-load.c
+++ b/gdb/auto-load.c
@@ -1408,7 +1408,7 @@ set_auto_load_cmd (const char *args, int from_tty)
 	     "otherwise check the auto-load sub-commands."));
 
   for (list = *auto_load_set_cmdlist_get (); list != NULL; list = list->next)
-    if (list->var_type == var_boolean)
+    if (list->var.type () == var_boolean)
       {
 	gdb_assert (list->type == set_cmd);
 	do_set_command (args, from_tty, list);
diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 56ae12a0c19..3365a78860c 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -232,7 +232,7 @@ with_command_1 (const char *set_cmd_prefix,
 					  /*ignore_help_classes=*/ 1);
   gdb_assert (set_cmd != nullptr);
 
-  if (set_cmd->var == nullptr)
+  if (!set_cmd->var)
     error (_("Cannot use this setting with the \"with\" command"));
 
   std::string temp_value
@@ -2090,29 +2090,29 @@ setting_cmd (const char *fnname, struct cmd_list_element *showlist,
 static struct value *
 value_from_setting (const cmd_list_element *cmd, struct gdbarch *gdbarch)
 {
-  switch (cmd->var_type)
+  switch (cmd->var.type ())
     {
     case var_integer:
-      if (*(int *) cmd->var == INT_MAX)
+      if (cmd->var.get<var_integer> () == INT_MAX)
 	return value_from_longest (builtin_type (gdbarch)->builtin_int,
 				   0);
       else
 	return value_from_longest (builtin_type (gdbarch)->builtin_int,
-				   *(int *) cmd->var);
+				   cmd->var.get<var_integer> ());
     case var_zinteger:
       return value_from_longest (builtin_type (gdbarch)->builtin_int,
-				 *(int *) cmd->var);
+				 cmd->var.get<var_zinteger> ());
     case var_boolean:
       return value_from_longest (builtin_type (gdbarch)->builtin_int,
-				 *(bool *) cmd->var ? 1 : 0);
+				 cmd->var.get<var_boolean> () ? 1 : 0);
     case var_zuinteger_unlimited:
       return value_from_longest (builtin_type (gdbarch)->builtin_int,
-				 *(int *) cmd->var);
+				 cmd->var.get<var_zuinteger_unlimited> ());
     case var_auto_boolean:
       {
 	int val;
 
-	switch (*(enum auto_boolean*) cmd->var)
+	switch (cmd->var.get<var_auto_boolean> ())
 	  {
 	  case AUTO_BOOLEAN_TRUE:
 	    val = 1;
@@ -2130,24 +2130,34 @@ value_from_setting (const cmd_list_element *cmd, struct gdbarch *gdbarch)
 				   val);
       }
     case var_uinteger:
-      if (*(unsigned int *) cmd->var == UINT_MAX)
+      if (cmd->var.get<var_uinteger> () == UINT_MAX)
 	return value_from_ulongest
 	  (builtin_type (gdbarch)->builtin_unsigned_int, 0);
       else
 	return value_from_ulongest
 	  (builtin_type (gdbarch)->builtin_unsigned_int,
-	   *(unsigned int *) cmd->var);
+	   cmd->var.get<var_uinteger> ());
     case var_zuinteger:
       return value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int,
-				  *(unsigned int *) cmd->var);
+				  cmd->var.get<var_zuinteger> ());
     case var_string:
     case var_string_noescape:
     case var_optional_filename:
     case var_filename:
     case var_enum:
-      if (*(char **) cmd->var)
-	return value_cstring (*(char **) cmd->var, strlen (*(char **) cmd->var),
-			      builtin_type (gdbarch)->builtin_char);
+      if (cmd->var)
+	{
+	  const char * var;
+	  if (cmd->var.type () == var_enum)
+	    var = cmd->var.get<var_enum> ();
+	  else
+	    var = cmd->var.get<var_string,
+			       var_string_noescape,
+			       var_optional_filename,
+			       var_filename> ();
+	  return value_cstring (var, strlen (var),
+				builtin_type (gdbarch)->builtin_char);
+	}
       else
 	return value_cstring ("", 1,
 			      builtin_type (gdbarch)->builtin_char);
@@ -2186,7 +2196,7 @@ gdb_maint_setting_internal_fn (struct gdbarch *gdbarch,
 static struct value *
 str_value_from_setting (const cmd_list_element *cmd, struct gdbarch *gdbarch)
 {
-  switch (cmd->var_type)
+  switch (cmd->var.type ())
     {
     case var_integer:
     case var_zinteger:
@@ -2211,9 +2221,19 @@ str_value_from_setting (const cmd_list_element *cmd, struct gdbarch *gdbarch)
 	 as this function handle some characters specially, e.g. by
 	 escaping quotes.  So, we directly use the cmd->var string value,
 	 similarly to the value_from_setting code for these cases.  */
-      if (*(char **) cmd->var)
-	return value_cstring (*(char **) cmd->var, strlen (*(char **) cmd->var),
-			      builtin_type (gdbarch)->builtin_char);
+      if (cmd->var)
+	{
+	  const char *var;
+	  if (cmd->var.type () == var_enum)
+	    var = cmd->var.get<var_enum> ();
+	  else
+	    var = cmd->var.get<var_string,
+			       var_string_noescape,
+			       var_optional_filename,
+			       var_filename> ();
+	  return value_cstring (var, strlen (var),
+				builtin_type (gdbarch)->builtin_char);
+	}
       else
 	return value_cstring ("", 1,
 			      builtin_type (gdbarch)->builtin_char);
diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 633a3ad9309..f15747cf337 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -481,12 +481,12 @@ empty_sfunc (const char *args, int from_tty, struct cmd_list_element *c)
    VAR is address of the variable being controlled by this command.
    DOC is the documentation string.  */
 
+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,
-		     var_types var_type,
-		     void *var,
+		     typename detail::var_types_storage<T>::type *var,
 		     const char *doc,
 		     struct cmd_list_element **list)
 {
@@ -494,8 +494,7 @@ add_set_or_show_cmd (const char *name,
 
   gdb_assert (type == set_cmd || type == show_cmd);
   c->type = type;
-  c->var_type = var_type;
-  c->var = var;
+  c->var.set_p<T> (var);
   /* This needs to be something besides NULL so that this isn't
      treated as a help class.  */
   set_cmd_sfunc (c, empty_sfunc);
@@ -511,10 +510,11 @@ add_set_or_show_cmd (const char *name,
 
    Return the newly created set and show commands.  */
 
+template<var_types T>
 static set_show_commands
 add_setshow_cmd_full (const char *name,
 		      enum command_class theclass,
-		      var_types var_type, void *var,
+		      typename detail::var_types_storage<T>::type *var,
 		      const char *set_doc, const char *show_doc,
 		      const char *help_doc,
 		      cmd_const_sfunc_ftype *set_func,
@@ -537,15 +537,15 @@ add_setshow_cmd_full (const char *name,
       full_set_doc = xstrdup (set_doc);
       full_show_doc = xstrdup (show_doc);
     }
-  set = add_set_or_show_cmd (name, set_cmd, theclass, var_type, var,
-			     full_set_doc, set_list);
+  set = add_set_or_show_cmd<T> (name, set_cmd, theclass, var,
+				full_set_doc, set_list);
   set->doc_allocated = 1;
 
   if (set_func != NULL)
     set_cmd_sfunc (set, set_func);
 
-  show = add_set_or_show_cmd (name, show_cmd, theclass, var_type, var,
-			      full_show_doc, show_list);
+  show = add_set_or_show_cmd<T> (name, show_cmd, theclass, var,
+				 full_show_doc, show_list);
   show->doc_allocated = 1;
   show->show_value_func = show_func;
   /* Disable the default symbol completer.  Doesn't make much sense
@@ -574,10 +574,10 @@ add_setshow_enum_cmd (const char *name,
 		      struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    =  add_setshow_cmd_full (name, theclass, var_enum, var,
-			     set_doc, show_doc, help_doc,
-			     set_func, show_func,
-			     set_list, show_list);
+    =  add_setshow_cmd_full<var_enum> (name, theclass, var,
+				       set_doc, show_doc, help_doc,
+				       set_func, show_func,
+				       set_list, show_list);
   commands.set->enums = enumlist;
   return commands;
 }
@@ -602,10 +602,10 @@ add_setshow_auto_boolean_cmd (const char *name,
 			      struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_auto_boolean, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_auto_boolean> (name, theclass, var,
+					      set_doc, show_doc, help_doc,
+					      set_func, show_func,
+					      set_list, show_list);
 
   commands.set->enums = auto_boolean_enums;
 
@@ -631,10 +631,10 @@ add_setshow_boolean_cmd (const char *name, enum command_class theclass, bool *va
 			 struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_boolean, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_boolean> (name, theclass, var,
+					 set_doc, show_doc, help_doc,
+					 set_func, show_func,
+					 set_list, show_list);
 
   commands.set->enums = boolean_enums;
 
@@ -655,10 +655,10 @@ add_setshow_filename_cmd (const char *name, enum command_class theclass,
 			  struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_filename, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_filename> (name, theclass, var,
+					  set_doc, show_doc, help_doc,
+					  set_func, show_func,
+					  set_list, show_list);
 
   set_cmd_completer (commands.set, filename_completer);
 
@@ -679,10 +679,10 @@ add_setshow_string_cmd (const char *name, enum command_class theclass,
 			struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_string, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_string> (name, theclass, var,
+					set_doc, show_doc, help_doc,
+					set_func, show_func,
+					set_list, show_list);
 
   /* Disable the default symbol completer.  */
   set_cmd_completer (commands.set, nullptr);
@@ -704,10 +704,10 @@ add_setshow_string_noescape_cmd (const char *name, enum command_class theclass,
 				 struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_string_noescape, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_string_noescape> (name, theclass, var,
+						 set_doc, show_doc, help_doc,
+						 set_func, show_func,
+						 set_list, show_list);
 
   /* Disable the default symbol completer.  */
   set_cmd_completer (commands.set, nullptr);
@@ -729,10 +729,10 @@ add_setshow_optional_filename_cmd (const char *name, enum command_class theclass
 				   struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_optional_filename, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_optional_filename> (name, theclass, var,
+						   set_doc, show_doc, help_doc,
+						   set_func, show_func,
+						   set_list, show_list);
 		
   set_cmd_completer (commands.set, filename_completer);
 
@@ -773,10 +773,10 @@ add_setshow_integer_cmd (const char *name, enum command_class theclass,
 			 struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_integer, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_integer> (name, theclass, var,
+					 set_doc, show_doc, help_doc,
+					 set_func, show_func,
+					 set_list, show_list);
 
   set_cmd_completer (commands.set, integer_unlimited_completer);
 
@@ -799,10 +799,10 @@ add_setshow_uinteger_cmd (const char *name, enum command_class theclass,
 			  struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_uinteger, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_uinteger> (name, theclass, var,
+					  set_doc, show_doc, help_doc,
+					  set_func, show_func,
+					  set_list, show_list);
 
   set_cmd_completer (commands.set, integer_unlimited_completer);
 
@@ -824,10 +824,10 @@ add_setshow_zinteger_cmd (const char *name, enum command_class theclass,
 			  struct cmd_list_element **set_list,
 			  struct cmd_list_element **show_list)
 {
-  return add_setshow_cmd_full (name, theclass, var_zinteger, var,
-			       set_doc, show_doc, help_doc,
-			       set_func, show_func,
-			       set_list, show_list);
+  return add_setshow_cmd_full<var_zinteger> (name, theclass, var,
+					     set_doc, show_doc, help_doc,
+					     set_func, show_func,
+					     set_list, show_list);
 }
 
 set_show_commands
@@ -843,10 +843,11 @@ add_setshow_zuinteger_unlimited_cmd (const char *name,
 				     struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_zuinteger_unlimited, var,
-			    set_doc, show_doc, help_doc,
-			    set_func, show_func,
-			    set_list, show_list);
+    = add_setshow_cmd_full<var_zuinteger_unlimited> (name, theclass, var,
+						     set_doc, show_doc,
+						     help_doc, set_func,
+						     show_func, set_list,
+						     show_list);
 
   set_cmd_completer (commands.set, integer_unlimited_completer);
 
@@ -868,10 +869,10 @@ add_setshow_zuinteger_cmd (const char *name, enum command_class theclass,
 			   struct cmd_list_element **set_list,
 			   struct cmd_list_element **show_list)
 {
-  return add_setshow_cmd_full (name, theclass, var_zuinteger, var,
-			       set_doc, show_doc, help_doc,
-			       set_func, show_func,
-			       set_list, show_list);
+  return add_setshow_cmd_full<var_zuinteger> (name, theclass, var,
+					      set_doc, show_doc, help_doc,
+					      set_func, show_func,
+					      set_list, show_list);
 }
 
 /* Remove the command named NAME from the command list.  Return the
diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h
index 241535ae5b5..6799f965371 100644
--- a/gdb/cli/cli-decode.h
+++ b/gdb/cli/cli-decode.h
@@ -55,7 +55,6 @@ struct cmd_list_element
       allow_unknown (0),
       abbrev_flag (0),
       type (not_set_cmd),
-      var_type (var_boolean),
       doc (doc_)
   {
     memset (&function, 0, sizeof (function));
@@ -160,9 +159,6 @@ struct cmd_list_element
      or "show").  */
   ENUM_BITFIELD (cmd_types) type : 2;
 
-  /* What kind of variable is *VAR?  */
-  ENUM_BITFIELD (var_types) var_type : 4;
-
   /* Function definition of this command.  NULL for command class
      names and for help topics that are not really commands.  NOTE:
      cagney/2002-02-02: This function signature is evolving.  For
@@ -230,7 +226,7 @@ struct cmd_list_element
 
   /* Pointer to variable affected by "set" and "show".  Doesn't
      matter if type is not_set.  */
-  void *var = nullptr;
+  setting var;
 
   /* Pointer to NULL terminated list of enumerated values (like
      argv).  */
diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index 5fd5fd15c6a..883ac30598e 100644
--- a/gdb/cli/cli-setshow.c
+++ b/gdb/cli/cli-setshow.c
@@ -133,7 +133,7 @@ deprecated_show_value_hack (struct ui_file *ignore_file,
   /* Print doc minus "Show " at start.  Tell print_doc_line that
      this is for a 'show value' prefix.  */
   print_doc_line (gdb_stdout, c->doc + 5, true);
-  switch (c->var_type)
+  switch (c->var.type ())
     {
     case var_string:
     case var_string_noescape:
@@ -312,7 +312,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
   if (arg == NULL)
     arg = "";
 
-  switch (c->var_type)
+  switch (c->var.type ())
     {
     case var_string:
       {
@@ -353,11 +353,12 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	*q++ = '\0';
 	newobj = (char *) xrealloc (newobj, q - newobj);
 
-	if (*(char **) c->var == NULL
-	    || strcmp (*(char **) c->var, newobj) != 0)
+        auto const var = c->var.get<var_string> ();
+	if (var == nullptr
+	    || strcmp (var, newobj) != 0)
 	  {
-	    xfree (*(char **) c->var);
-	    *(char **) c->var = newobj;
+	    xfree (var);
+	    c->var.set<var_string> (newobj);
 
 	    option_changed = 1;
 	  }
@@ -366,13 +367,17 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
       }
       break;
     case var_string_noescape:
-      if (*(char **) c->var == NULL || strcmp (*(char **) c->var, arg) != 0)
-	{
-	  xfree (*(char **) c->var);
-	  *(char **) c->var = xstrdup (arg);
+      {
+	auto const var = c->var.get<var_string_noescape> ();
+	if (var == nullptr
+	    || strcmp (var, arg) != 0)
+	  {
+	    xfree (var);
+	    c->var.set<var_string_noescape> (xstrdup (arg));
 
-	  option_changed = 1;
-	}
+	    option_changed = 1;
+	  }
+      }
       break;
     case var_filename:
       if (*arg == '\0')
@@ -381,6 +386,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
     case var_optional_filename:
       {
 	char *val = NULL;
+	auto const var = c->var.get<var_filename, var_optional_filename> ();
 
 	if (*arg != '\0')
 	  {
@@ -398,11 +404,11 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	else
 	  val = xstrdup ("");
 
-	if (*(char **) c->var == NULL
-	    || strcmp (*(char **) c->var, val) != 0)
+	if (var == nullptr
+	    || strcmp (var, val) != 0)
 	  {
-	    xfree (*(char **) c->var);
-	    *(char **) c->var = val;
+	    xfree (val);
+	    c->var.set<var_filename, var_optional_filename> (val);
 
 	    option_changed = 1;
 	  }
@@ -416,9 +422,9 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
 	if (val < 0)
 	  error (_("\"on\" or \"off\" expected."));
-	if (val != *(bool *) c->var)
+	if (val != c->var.get<var_boolean> ())
 	  {
-	    *(bool *) c->var = val;
+	    c->var.set<var_boolean> (val);
 
 	    option_changed = 1;
 	  }
@@ -428,9 +434,9 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
       {
 	enum auto_boolean val = parse_auto_binary_operation (arg);
 
-	if (*(enum auto_boolean *) c->var != val)
+	if (c->var.get<var_auto_boolean> () != val)
 	  {
-	    *(enum auto_boolean *) c->var = val;
+	    c->var.set<var_auto_boolean> (val);
 
 	    option_changed = 1;
 	  }
@@ -439,11 +445,11 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
     case var_uinteger:
     case var_zuinteger:
       {
-	unsigned int val = parse_cli_var_uinteger (c->var_type, &arg, true);
+	unsigned int val = parse_cli_var_uinteger (c->var.type (), &arg, true);
 
-	if (*(unsigned int *) c->var != val)
+	if (c->var.get<var_uinteger, var_zuinteger> () != val)
 	  {
-	    *(unsigned int *) c->var = val;
+	    c->var.set<var_uinteger, var_zuinteger> (val);
 
 	    option_changed = 1;
 	  }
@@ -456,35 +462,35 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
 	if (*arg == '\0')
 	  {
-	    if (c->var_type == var_integer)
+	    if (c->var.type () == var_integer)
 	      error_no_arg (_("integer to set it to, or \"unlimited\"."));
 	    else
 	      error_no_arg (_("integer to set it to."));
 	  }
 
-	if (c->var_type == var_integer && is_unlimited_literal (&arg, true))
+	if (c->var.type () == var_integer && is_unlimited_literal (&arg, true))
 	  val = 0;
 	else
 	  val = parse_and_eval_long (arg);
 
-	if (val == 0 && c->var_type == var_integer)
+	if (val == 0 && c->var.type () == var_integer)
 	  val = INT_MAX;
 	else if (val < INT_MIN
 		 /* For var_integer, don't let the user set the value
 		    to INT_MAX directly, as that exposes an
 		    implementation detail to the user interface.  */
-		 || (c->var_type == var_integer && val >= INT_MAX)
-		 || (c->var_type == var_zinteger && val > INT_MAX))
+		 || (c->var.type () == var_integer && val >= INT_MAX)
+		 || (c->var.type () == var_zinteger && val > INT_MAX))
 	  error (_("integer %s out of range"), plongest (val));
 
-	if (*(int *) c->var != val)
+	if (c->var.get<var_integer, var_zinteger> () != val)
 	  {
-	    *(int *) c->var = val;
+	    c->var.set<var_integer, var_zinteger> (val);
 
 	    option_changed = 1;
 	  }
-	break;
       }
+      break;
     case var_enum:
       {
 	const char *end_arg = arg;
@@ -495,9 +501,9 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	if (*after != '\0')
 	  error (_("Junk after item \"%.*s\": %s"), len, arg, after);
 
-	if (*(const char **) c->var != match)
+	if (c->var.get<var_enum> () != match)
 	  {
-	    *(const char **) c->var = match;
+	    c->var.set<var_enum> (match);
 
 	    option_changed = 1;
 	  }
@@ -507,9 +513,9 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
       {
 	int val = parse_cli_var_zuinteger_unlimited (&arg, true);
 
-	if (*(int *) c->var != val)
+	if (c->var.get<var_zuinteger_unlimited> () != val)
 	  {
-	    *(int *) c->var = val;
+	    c->var.set<var_zuinteger_unlimited> (val);
 	    option_changed = 1;
 	  }
       }
@@ -577,25 +583,35 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
       xfree (cmds);
 
-      switch (c->var_type)
+      switch (c->var.type ())
 	{
 	case var_string:
 	case var_string_noescape:
 	case var_filename:
 	case var_optional_filename:
 	case var_enum:
-	  gdb::observers::command_param_changed.notify (name, *(char **) c->var);
+	  {
+	    const char *var;
+	    if (c->var.type () == var_enum)
+	      var = c->var.get<var_enum> ();
+	    else
+	      var = c->var.get<var_string,
+			       var_string_noescape,
+			       var_filename,
+			       var_optional_filename> ();
+	    gdb::observers::command_param_changed.notify (name, var);
+	  }
 	  break;
 	case var_boolean:
 	  {
-	    const char *opt = *(bool *) c->var ? "on" : "off";
+	    const char *opt = c->var.get<var_boolean> () ? "on" : "off";
 
 	    gdb::observers::command_param_changed.notify (name, opt);
 	  }
 	  break;
 	case var_auto_boolean:
 	  {
-	    const char *s = auto_boolean_enums[*(enum auto_boolean *) c->var];
+	    const char *s = auto_boolean_enums[c->var.get<var_auto_boolean> ()];
 
 	    gdb::observers::command_param_changed.notify (name, s);
 	  }
@@ -605,7 +621,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	  {
 	    char s[64];
 
-	    xsnprintf (s, sizeof s, "%u", *(unsigned int *) c->var);
+	    xsnprintf (s, sizeof s, "%u", c->var.get<var_uinteger, var_zuinteger> ());
 	    gdb::observers::command_param_changed.notify (name, s);
 	  }
 	  break;
@@ -615,7 +631,8 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	  {
 	    char s[64];
 
-	    xsnprintf (s, sizeof s, "%d", *(int *) c->var);
+	    xsnprintf (s, sizeof s, "%d",
+		       c->var.get<var_integer, var_zinteger, var_zuinteger_unlimited> ());
 	    gdb::observers::command_param_changed.notify (name, s);
 	  }
 	  break;
@@ -631,24 +648,36 @@ get_setshow_command_value_string (const cmd_list_element *c)
 {
   string_file stb;
 
-  switch (c->var_type)
+  switch (c->var.type ())
     {
     case var_string:
-      if (*(char **) c->var)
-	stb.putstr (*(char **) c->var, '"');
+      {
+	auto const var = c->var.get<var_string> ();
+	if (var != nullptr)
+	  stb.putstr (var, '"');
+      }
       break;
     case var_string_noescape:
     case var_optional_filename:
     case var_filename:
     case var_enum:
-      if (*(char **) c->var)
-	stb.puts (*(char **) c->var);
+      {
+	const char *var;
+	if (c->var.type () == var_enum)
+	  var = c->var.get<var_enum> ();
+	else
+	  var = c->var.get<var_string_noescape,
+			   var_optional_filename,
+			   var_filename> ();
+	if (var != nullptr)
+	  stb.puts (var);
+      }
       break;
     case var_boolean:
-      stb.puts (*(bool *) c->var ? "on" : "off");
+      stb.puts (c->var.get<var_boolean> () ? "on" : "off");
       break;
     case var_auto_boolean:
-      switch (*(enum auto_boolean*) c->var)
+      switch (c->var.get<var_auto_boolean> ())
 	{
 	case AUTO_BOOLEAN_TRUE:
 	  stb.puts ("on");
@@ -666,26 +695,33 @@ get_setshow_command_value_string (const cmd_list_element *c)
       break;
     case var_uinteger:
     case var_zuinteger:
-      if (c->var_type == var_uinteger
-	  && *(unsigned int *) c->var == UINT_MAX)
-	stb.puts ("unlimited");
-      else
-	stb.printf ("%u", *(unsigned int *) c->var);
+      {
+	auto const var = c->var.get<var_uinteger, var_zuinteger> ();
+	if (c->var.type () == var_uinteger
+	    && var == UINT_MAX)
+	  stb.puts ("unlimited");
+	else
+	  stb.printf ("%u", var);
+      }
       break;
     case var_integer:
     case var_zinteger:
-      if (c->var_type == var_integer
-	  && *(int *) c->var == INT_MAX)
-	stb.puts ("unlimited");
-      else
-	stb.printf ("%d", *(int *) c->var);
+      {
+	const auto var = c->var.get<var_integer, var_zinteger> ();
+	if (c->var.type () == var_integer
+	    && var == INT_MAX)
+	  stb.puts ("unlimited");
+	else
+	  stb.printf ("%d", var);
+      }
       break;
     case var_zuinteger_unlimited:
       {
-	if (*(int *) c->var == -1)
+	auto const var = c->var.get<var_zuinteger_unlimited> ();
+	if (var == -1)
 	  stb.puts ("unlimited");
 	else
-	  stb.printf ("%d", *(int *) c->var);
+	  stb.printf ("%d", var);
       }
       break;
     default:
diff --git a/gdb/command.h b/gdb/command.h
index 711cbdcf43e..489ca9b1b65 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -125,6 +125,424 @@ typedef enum var_types
   }
 var_types;
 
+
+template<typename T>
+struct accessor_sigs
+{
+  using getter = T (*) ();
+  using setter = void (*) (T);
+};
+
+
+union setting_getter
+{
+  typename accessor_sigs<bool>::getter get_bool;
+  typename accessor_sigs<int>::getter get_int;
+  typename accessor_sigs<unsigned int>::getter get_uint;
+  typename accessor_sigs<auto_boolean>::getter get_auto_boolean;
+  typename accessor_sigs<char *>::getter get_char_p;
+  typename accessor_sigs<const char *>::getter get_const_char_p;
+};
+
+union setting_setter
+{
+  typename accessor_sigs<bool>::setter set_bool;
+  typename accessor_sigs<int>::setter set_int;
+  typename accessor_sigs<unsigned int>::setter set_uint;
+  typename accessor_sigs<auto_boolean>::setter set_auto_boolean;
+  typename accessor_sigs<char *>::setter set_char_p;
+  typename accessor_sigs<const char *>::setter set_const_char_p;
+};
+
+namespace detail
+{
+  template<typename>
+  struct accessor_helper;
+
+  template<>
+  struct accessor_helper<bool>
+  {
+    static accessor_sigs<bool>::getter &getter(setting_getter & getters)
+    {
+      return getters.get_bool;
+    }
+
+    static accessor_sigs<bool>::setter &setter(setting_setter & setters)
+    {
+      return setters.set_bool;
+    }
+  };
+
+  template<>
+  struct accessor_helper<int>
+  {
+    static accessor_sigs<int>::getter &getter(setting_getter & getters)
+    {
+      return getters.get_int;
+    }
+
+    static accessor_sigs<int>::setter &setter(setting_setter & setters)
+    {
+      return setters.set_int;
+    }
+  };
+
+  template<>
+  struct accessor_helper<unsigned int>
+  {
+    static accessor_sigs<unsigned int>::getter &getter(setting_getter & getters)
+    {
+      return getters.get_uint;
+    }
+
+    static accessor_sigs<unsigned int>::setter &setter(setting_setter & setters)
+    {
+      return setters.set_uint;
+    }
+  };
+
+  template<>
+  struct accessor_helper<auto_boolean>
+  {
+    static accessor_sigs<auto_boolean>::getter &getter(setting_getter & getters)
+    {
+      return getters.get_auto_boolean;
+    }
+
+    static accessor_sigs<auto_boolean>::setter &setter(setting_setter & setters)
+    {
+      return setters.set_auto_boolean;
+    }
+  };
+
+  template<>
+  struct accessor_helper<char *>
+  {
+    static accessor_sigs<char *>::getter &getter(setting_getter & getters)
+    {
+      return getters.get_char_p;
+    }
+
+    static accessor_sigs<char *>::setter &setter(setting_setter & setters)
+    {
+      return setters.set_char_p;
+    }
+  };
+
+  template<>
+  struct accessor_helper<const char *>
+  {
+    static accessor_sigs<const char *>::getter &getter(setting_getter & getters)
+    {
+      return getters.get_const_char_p;
+    }
+
+    static accessor_sigs<const char *>::setter &setter(setting_setter & setters)
+    {
+      return setters.set_const_char_p;
+    }
+  };
+
+  /* Helper classes used to associate a storage type for each possible
+     var_type. */
+
+  template<var_types T>
+  struct var_types_storage;
+
+  template<>
+  struct var_types_storage<var_boolean>
+  {
+    using type = bool;
+
+  };
+
+  template<>
+  struct var_types_storage<var_auto_boolean>
+  {
+    using type = enum auto_boolean;
+  };
+
+  template<>
+  struct var_types_storage<var_uinteger>
+  {
+    using type = unsigned int;
+  };
+
+  template<>
+  struct var_types_storage<var_integer>
+  {
+    using type = int;
+  };
+
+  template<>
+  struct var_types_storage<var_string>
+  {
+    using type = char *;
+  };
+
+  template<>
+  struct var_types_storage<var_string_noescape>
+  {
+    using type = char *;
+  };
+
+  template<>
+  struct var_types_storage<var_optional_filename>
+  {
+    using type = char *;
+  };
+
+  template<>
+  struct var_types_storage<var_filename>
+  {
+    using type = char *;
+  };
+
+  template<>
+  struct var_types_storage<var_zinteger>
+  {
+    using type = int;
+  };
+
+  template<>
+  struct var_types_storage<var_zuinteger>
+  {
+    using type = unsigned int;
+  };
+
+  template<>
+  struct var_types_storage<var_zuinteger_unlimited>
+  {
+    using type = int;
+  };
+
+  template<>
+  struct var_types_storage<var_enum>
+  {
+    using type = const char *;
+  };
+
+  /* Helper class used to check if multiple var_types are represented
+     using the same underlying type.  This class is meant to be instantiated
+     using any number of var_types, and will be used to assess common properties
+     of the underlying storage type.
+
+     Each template instantiating will define the following static members:
+    - value: True if and only if all the var_types are stored on the same
+    underlying storage type.
+    - covers_type (var_types t): True if and only if the parameter T is one
+    the templates parameter.
+    - type: Type alias of the underlying type if value is true, unspecified
+    otherwise.
+    */
+
+  template<var_types... Ts>
+  struct var_types_have_same_storage;
+
+  /* Specialization of var_types_have_same_storage when instantiated with only 1
+     template parameter.  */
+  template<var_types T>
+  struct var_types_have_same_storage<T>
+  {
+    static constexpr bool value = true;
+
+    using type = typename var_types_storage<T>::type;
+
+    static constexpr bool covers_type (var_types t)
+    {
+      return t == T;
+    }
+  };
+
+  /* Specialization of var_types_have_same_storage when instantiated with exactly
+     2 template parameters.  */
+  template<var_types T, var_types U>
+  struct var_types_have_same_storage<T, U>
+  {
+    static constexpr bool value
+      = std::is_same<typename var_types_storage<T>::type,
+      typename var_types_storage<U>::type>::value;
+
+    using type = typename var_types_storage<T>::type;
+
+    static constexpr bool covers_type (var_types t)
+    {
+      return var_types_have_same_storage<T>::covers_type (t)
+	|| var_types_have_same_storage<U>::covers_type (t);
+    }
+  };
+
+  /* Specialization of var_types_have_same_storage when instantiated with 3 or more
+     template parameters.  */
+  template<var_types T, var_types U, var_types... Us>
+  struct var_types_have_same_storage<T, U, Us...>
+  {
+    static constexpr bool value
+      = var_types_have_same_storage<T, U>::value
+      && var_types_have_same_storage<T, Us...>::value;
+
+    using type = typename var_types_storage<T>::type;
+
+    static constexpr bool covers_type (var_types t)
+      {
+	return var_types_have_same_storage<T>::covers_type (t)
+	  || var_types_have_same_storage<U, Us...>::covers_type (t);
+      }
+  };
+} /* namespace detail */
+
+/* Abstraction that contains access to data that can be set or shown.
+
+   The underlying data can be of an VAR_TYPES type.  */
+struct base_setting_wrapper
+{
+  /* Access the type of the current var.  */
+  var_types type () const
+  {
+    return m_var_type;
+  }
+
+  /* Return the current value (by pointer).
+
+     The expected template parameter is the VAR_TYPES of the current instance.
+     This is enforced with a runtime check.
+
+     If multiple template parameters are given, check that the underlying
+     pointer type associated with each parameter are the same.  */
+  template<var_types... Ts,
+	   typename = gdb::Requires<detail::var_types_have_same_storage<Ts...>>>
+  typename detail::var_types_have_same_storage<Ts...>::type const *get_p() const
+  {
+    gdb_assert (detail::var_types_have_same_storage<Ts...>::covers_type
+                (this->m_var_type));
+    gdb_assert (!empty ());
+    // TODO
+    //gdb_assert (m_getter == nullptr && m_setter == nullptr);
+
+    return static_cast<
+      typename detail::var_types_have_same_storage<Ts...>::type const *>
+      (this->m_var);
+  }
+
+  /* Return the current value.
+
+     See get_p for discussion on the return type.  */
+  template<var_types... Ts>
+  typename detail::var_types_have_same_storage<Ts...>::type get() const
+  {
+    gdb_assert (detail::var_types_have_same_storage<Ts...>::covers_type
+                (this->m_var_type));
+    auto getter = detail::accessor_helper<
+      typename detail::var_types_have_same_storage<Ts...>::type>::getter
+        (const_cast<base_setting_wrapper *> (this)->m_getter);
+
+    if (getter != nullptr)
+      return (*getter) ();
+    else
+      {
+        gdb_assert (!empty ());
+        return *get_p<Ts...> ();
+      }
+  }
+
+  /* Sets the value V to the underlying buffer.  If we have a user-provided
+     setter, use it to set the setting, otherwise set it to the internally
+     referenced buffer.
+
+     If one template argument is given, it must be the VAR_TYPE of the current
+     instance.  This is enforced at runtime.
+
+     If multiple template parameters are given, they must all share the same
+     underlying storage type (this is checked at compile time), and THIS must
+     be of the type of one of the template parameters (this is checked at
+     runtime).  */
+  template<var_types... Ts,
+           typename = gdb::Requires<
+             detail::var_types_have_same_storage<Ts...>>>
+  void set(typename detail::var_types_have_same_storage<Ts...>::type v)
+  {
+    /* Check that the current instance is of one of the supported types for
+       this instantiation.  */
+    gdb_assert (detail::var_types_have_same_storage<Ts...>::covers_type
+                (this->m_var_type));
+
+    auto setter = detail::accessor_helper<
+        typename detail::var_types_have_same_storage<Ts...>::type>::setter (this->m_setter);
+
+    if (setter != nullptr)
+      {
+        (*setter) (v);
+      }
+    else
+      {
+        gdb_assert (!empty ());
+        *static_cast<typename detail::var_types_have_same_storage<Ts...>::type *>
+          (this->m_var) = v;
+      }
+  }
+
+  template<var_types T>
+  void set_accessors(typename accessor_sigs<typename detail::var_types_storage<T>::type>::setter setter,
+                     typename accessor_sigs<typename detail::var_types_storage<T>::type>::getter getter)
+  {
+    m_var_type = T;
+    detail::accessor_helper<typename detail::var_types_storage<T>::type>::setter (this->m_setter) = setter;
+    detail::accessor_helper<typename detail::var_types_storage<T>::type>::getter (this->m_getter) = getter;
+  }
+
+  /* A setting is valid (can be evaluated to true) if it contains user provided
+     getter and setter or has a pointer set to a setting.  */
+  explicit operator bool() const
+  {
+    // TODO refactor that to not rely on one of the options
+    return (m_getter.get_bool != nullptr && m_setter.set_bool != nullptr) || !this->empty();
+  }
+
+protected:
+  /* The type of the variable M_VAR is pointing to.  If M_VAR is nullptr,
+     M_VAR_TYPE is ignored.  */
+  var_types m_var_type { var_boolean };
+
+  /* Pointer to the enclosed variable.  The type of the variable is encoded
+     in M_VAR_TYPE.  Can be nullptr.  */
+  void *m_var { nullptr };
+
+  /* Pointer to a user provided getter.  */
+  union setting_getter m_getter { .get_bool { nullptr } };
+
+  /* Pointer to a user provided setter.  */
+  union setting_setter m_setter { .set_bool { nullptr } };
+
+  /* Indicates if the current instance has a underlying buffer.  */
+  bool empty () const
+  {
+    return m_var == nullptr;;
+  }
+
+};
+
+
+/* A augmented version of base_setting_wrapper with additional methods to set the
+   underlying buffer and declare the var_type.  */
+struct setting final: base_setting_wrapper
+{
+  /*  Set the type of the current variable.  */
+  void set_type (var_types type)
+  {
+    gdb_assert (empty ());
+    this->m_var_type = type;
+  }
+
+  /* Update the pointer to the underlying variable referenced by this
+     instance.  */
+  template<var_types T>
+  void set_p (typename detail::var_types_storage<T>::type *v)
+  {
+    this->set_type (T);
+    this->m_var = static_cast<void *> (v);
+  }
+};
+
 /* This structure records one command'd definition.  */
 struct cmd_list_element;
 
diff --git a/gdb/guile/scm-param.c b/gdb/guile/scm-param.c
index c052d04974a..83fb3a27072 100644
--- a/gdb/guile/scm-param.c
+++ b/gdb/guile/scm-param.c
@@ -113,6 +113,19 @@ struct param_smob
   SCM containing_scm;
 };
 
+/* Wraps a base_setting_wrapper around an existing param_smob.  This abstraction
+   is used
+   to manipulate the value in S->VALUE in a type safe manner the same way as if
+   it was referenced by a setting.  */
+struct setting_wrapper final: base_setting_wrapper
+{
+  explicit setting_wrapper (param_smob *s)
+  {
+    m_var_type = s->type;
+    m_var = &s->value;
+  }
+};
+
 static const char param_smob_name[] = "gdb:parameter";
 
 /* The tag Guile knows the param smob by.  */
@@ -133,8 +146,7 @@ static SCM unlimited_keyword;
 
 static int pascm_is_valid (param_smob *);
 static const char *pascm_param_type_name (enum var_types type);
-static SCM pascm_param_value (enum var_types type, void *var,
-			      int arg_pos, const char *func_name);
+static SCM pascm_param_value (base_setting_wrapper const &c, int arg_pos, const char *func_name);
 
 /* Administrivia for parameter smobs.  */
 
@@ -151,10 +163,9 @@ pascm_print_param_smob (SCM self, SCM port, scm_print_state *pstate)
   if (! pascm_is_valid (p_smob))
     scm_puts (" {invalid}", port);
 
-  gdbscm_printf (port, " %s ", pascm_param_type_name (p_smob->type));
+  gdbscm_printf (port, " %s ", pascm_param_type_name (p_smob->type ));
 
-  value = pascm_param_value (p_smob->type, &p_smob->value,
-			     GDBSCM_ARG_NONE, NULL);
+  value = pascm_param_value (setting_wrapper (p_smob), GDBSCM_ARG_NONE, NULL);
   scm_display (value, port);
 
   scm_puts (">", port);
@@ -444,7 +455,7 @@ add_setshow_generic (enum var_types param_type, enum command_class cmd_class,
 				       show_doc, help_doc, set_func, show_func,
 				       set_list, show_list);
       /* Initialize the value, just in case.  */
-      self->value.cstringval = self->enumeration[0];
+      setting_wrapper (self).set<var_enum> (self->enumeration[0]);
       break;
 
     default:
@@ -566,14 +577,13 @@ pascm_param_type_name (enum var_types param_type)
    If TYPE is not supported, then a <gdb:exception> object is returned.  */
 
 static SCM
-pascm_param_value (enum var_types type, void *var,
-		   int arg_pos, const char *func_name)
+pascm_param_value (base_setting_wrapper const &var, int arg_pos, const char *func_name)
 {
   /* Note: We *could* support var_integer here in case someone is trying to get
      the value of a Python-created parameter (which is the only place that
      still supports var_integer).  To further discourage its use we do not.  */
 
-  switch (type)
+  switch (var.type ())
     {
     case var_string:
     case var_string_noescape:
@@ -581,16 +591,23 @@ pascm_param_value (enum var_types type, void *var,
     case var_filename:
     case var_enum:
       {
-	const char *str = *(char **) var;
+	const char *str;
+	if (var.type () == var_enum)
+	  str = var.get<var_enum> ();
+	else
+	  str = var.get<var_string,
+			var_string_noescape,
+			var_optional_filename,
+			var_filename> ();
 
-	if (str == NULL)
+	if (str == nullptr)
 	  str = "";
 	return gdbscm_scm_from_host_string (str, strlen (str));
       }
 
     case var_boolean:
       {
-	if (* (bool *) var)
+	if (var.get<var_boolean> ())
 	  return SCM_BOOL_T;
 	else
 	  return SCM_BOOL_F;
@@ -598,7 +615,7 @@ pascm_param_value (enum var_types type, void *var,
 
     case var_auto_boolean:
       {
-	enum auto_boolean ab = * (enum auto_boolean *) var;
+	enum auto_boolean ab = var.get<var_auto_boolean> ();
 
 	if (ab == AUTO_BOOLEAN_TRUE)
 	  return SCM_BOOL_T;
@@ -609,67 +626,84 @@ pascm_param_value (enum var_types type, void *var,
       }
 
     case var_zuinteger_unlimited:
-      if (* (int *) var == -1)
+      if (var.get<var_zuinteger_unlimited> () == -1)
 	return unlimited_keyword;
-      gdb_assert (* (int *) var >= 0);
+      gdb_assert (var.get<var_zuinteger_unlimited> () >= 0);
       /* Fall through.  */
     case var_zinteger:
-      return scm_from_int (* (int *) var);
+      return scm_from_int (var.get<var_zuinteger_unlimited, var_zinteger> ());
 
     case var_uinteger:
-      if (* (unsigned int *) var == UINT_MAX)
+      if (var.get<var_uinteger> ()== UINT_MAX)
 	return unlimited_keyword;
       /* Fall through.  */
     case var_zuinteger:
-      return scm_from_uint (* (unsigned int *) var);
+      return scm_from_uint (var.get<var_uinteger, var_zuinteger> ());
 
     default:
       break;
     }
 
   return gdbscm_make_out_of_range_error (func_name, arg_pos,
-					 scm_from_int (type),
+					 scm_from_int (var.type ()),
 					 _("program error: unhandled type"));
 }
 
-/* Set the value of a parameter of type TYPE in VAR from VALUE.
+/* Set the value of a parameter of type P_SMOB->TYPE in P_SMOB->VAR from VALUE.
    ENUMERATION is the list of enum values for enum parameters, otherwise NULL.
    Throws a Scheme exception if VALUE_SCM is invalid for TYPE.  */
 
 static void
-pascm_set_param_value_x (enum var_types type, union pascm_variable *var,
+pascm_set_param_value_x (param_smob *p_smob,
 			 const char * const *enumeration,
 			 SCM value, int arg_pos, const char *func_name)
 {
-  switch (type)
+  setting_wrapper var { p_smob };
+
+  switch (var.type ())
     {
     case var_string:
     case var_string_noescape:
     case var_optional_filename:
     case var_filename:
       SCM_ASSERT_TYPE (scm_is_string (value)
-		       || (type != var_filename
+		       || (var.type () != var_filename
 			   && gdbscm_is_false (value)),
 		       value, arg_pos, func_name,
 		       _("string or #f for non-PARAM_FILENAME parameters"));
       if (gdbscm_is_false (value))
 	{
-	  xfree (var->stringval);
-	  if (type == var_optional_filename)
-	    var->stringval = xstrdup ("");
+	  xfree (var.get<var_string,
+			 var_string_noescape,
+			 var_optional_filename,
+			 var_filename> ());
+	  if (var.type () == var_optional_filename)
+	    var.set<var_string,
+		    var_string_noescape,
+		    var_optional_filename,
+		    var_filename> (xstrdup (""));
 	  else
-	    var->stringval = NULL;
+	    var.set<var_string,
+		    var_string_noescape,
+		    var_optional_filename,
+		    var_filename> (nullptr);
 	}
       else
 	{
 	  SCM exception;
 
 	  gdb::unique_xmalloc_ptr<char> string
-	    = gdbscm_scm_to_host_string (value, NULL, &exception);
-	  if (string == NULL)
+	    = gdbscm_scm_to_host_string (value, nullptr, &exception);
+	  if (string == nullptr)
 	    gdbscm_throw (exception);
-	  xfree (var->stringval);
-	  var->stringval = string.release ();
+	  xfree (var.get<var_string,
+			  var_string_noescape,
+			  var_optional_filename,
+			  var_filename> ());
+	  var.set<var_string,
+		   var_string_noescape,
+		   var_optional_filename,
+		   var_filename> (string.release ());
 	}
       break;
 
@@ -681,27 +715,27 @@ pascm_set_param_value_x (enum var_types type, union pascm_variable *var,
 	SCM_ASSERT_TYPE (scm_is_string (value), value, arg_pos, func_name,
 		       _("string"));
 	gdb::unique_xmalloc_ptr<char> str
-	  = gdbscm_scm_to_host_string (value, NULL, &exception);
-	if (str == NULL)
+	  = gdbscm_scm_to_host_string (value, nullptr, &exception);
+	if (str == nullptr)
 	  gdbscm_throw (exception);
 	for (i = 0; enumeration[i]; ++i)
 	  {
 	    if (strcmp (enumeration[i], str.get ()) == 0)
 	      break;
 	  }
-	if (enumeration[i] == NULL)
+	if (enumeration[i] == nullptr)
 	  {
 	    gdbscm_out_of_range_error (func_name, arg_pos, value,
 				       _("not member of enumeration"));
 	  }
-	var->cstringval = enumeration[i];
+	var.set<var_enum> (enumeration[i]);
 	break;
       }
 
     case var_boolean:
       SCM_ASSERT_TYPE (gdbscm_is_bool (value), value, arg_pos, func_name,
 		       _("boolean"));
-      var->boolval = gdbscm_is_true (value);
+      var.set<var_boolean> (gdbscm_is_true (value));
       break;
 
     case var_auto_boolean:
@@ -710,19 +744,19 @@ pascm_set_param_value_x (enum var_types type, union pascm_variable *var,
 		       value, arg_pos, func_name,
 		       _("boolean or #:auto"));
       if (scm_is_eq (value, auto_keyword))
-	var->autoboolval = AUTO_BOOLEAN_AUTO;
+	var.set<var_auto_boolean> (AUTO_BOOLEAN_AUTO);
       else if (gdbscm_is_true (value))
-	var->autoboolval = AUTO_BOOLEAN_TRUE;
+	var.set<var_auto_boolean> (AUTO_BOOLEAN_TRUE);
       else
-	var->autoboolval = AUTO_BOOLEAN_FALSE;
+	var.set<var_auto_boolean> (AUTO_BOOLEAN_FALSE);
       break;
 
     case var_zinteger:
     case var_uinteger:
     case var_zuinteger:
     case var_zuinteger_unlimited:
-      if (type == var_uinteger
-	  || type == var_zuinteger_unlimited)
+      if (var.type () == var_uinteger
+	  || var.type () == var_zuinteger_unlimited)
 	{
 	  SCM_ASSERT_TYPE (gdbscm_is_bool (value)
 			   || scm_is_eq (value, unlimited_keyword),
@@ -730,10 +764,10 @@ pascm_set_param_value_x (enum var_types type, union pascm_variable *var,
 			   _("integer or #:unlimited"));
 	  if (scm_is_eq (value, unlimited_keyword))
 	    {
-	      if (type == var_uinteger)
-		var->intval = UINT_MAX;
+	      if (var.type () == var_uinteger)
+		var.set<var_integer, var_zuinteger_unlimited> (UINT_MAX);
 	      else
-		var->intval = -1;
+		var.set<var_integer, var_zuinteger_unlimited> (-1);
 	      break;
 	    }
 	}
@@ -743,25 +777,25 @@ pascm_set_param_value_x (enum var_types type, union pascm_variable *var,
 			   _("integer"));
 	}
 
-      if (type == var_uinteger
-	  || type == var_zuinteger)
+      if (var.type () == var_uinteger
+	  || var.type () == var_zuinteger)
 	{
 	  unsigned int u = scm_to_uint (value);
 
-	  if (type == var_uinteger && u == 0)
+	  if (var.type () == var_uinteger && u == 0)
 	    u = UINT_MAX;
-	  var->uintval = u;
+	  var.set<var_uinteger, var_zuinteger> (u);
 	}
       else
 	{
 	  int i = scm_to_int (value);
 
-	  if (type == var_zuinteger_unlimited && i < -1)
+	  if (var.type () == var_zuinteger_unlimited && i < -1)
 	    {
 	      gdbscm_out_of_range_error (func_name, arg_pos, value,
 					 _("must be >= -1"));
 	    }
-	  var->intval = i;
+	  var.set<var_zinteger, var_zuinteger_unlimited> (i);
 	}
       break;
 
@@ -934,7 +968,7 @@ gdbscm_make_parameter (SCM name_scm, SCM rest)
 	  if (gdbscm_is_exception (initial_value_scm))
 	    gdbscm_throw (initial_value_scm);
 	}
-      pascm_set_param_value_x (p_smob->type, &p_smob->value, enum_list,
+      pascm_set_param_value_x (p_smob, enum_list,
 			       initial_value_scm,
 			       initial_value_arg_pos, FUNC_NAME);
     }
@@ -1030,8 +1064,7 @@ gdbscm_parameter_value (SCM self)
       param_smob *p_smob = pascm_get_param_smob_arg_unsafe (self, SCM_ARG1,
 							    FUNC_NAME);
 
-      return pascm_param_value (p_smob->type, &p_smob->value,
-				SCM_ARG1, FUNC_NAME);
+      return pascm_param_value (setting_wrapper (p_smob), SCM_ARG1, FUNC_NAME);
     }
   else
     {
@@ -1062,13 +1095,13 @@ gdbscm_parameter_value (SCM self)
 	  gdbscm_out_of_range_error (FUNC_NAME, SCM_ARG1, self,
 				     _("parameter not found"));
 	}
-      if (cmd->var == NULL)
+      if (!cmd->var)
 	{
 	  gdbscm_out_of_range_error (FUNC_NAME, SCM_ARG1, self,
 				     _("not a parameter"));
 	}
 
-      return pascm_param_value (cmd->var_type, cmd->var, SCM_ARG1, FUNC_NAME);
+      return pascm_param_value (cmd->var, SCM_ARG1, FUNC_NAME);
     }
 }
 
@@ -1080,7 +1113,7 @@ gdbscm_set_parameter_value_x (SCM self, SCM value)
   param_smob *p_smob = pascm_get_param_smob_arg_unsafe (self, SCM_ARG1,
 							FUNC_NAME);
 
-  pascm_set_param_value_x (p_smob->type, &p_smob->value, p_smob->enumeration,
+  pascm_set_param_value_x (p_smob, p_smob->enumeration,
 			   value, SCM_ARG2, FUNC_NAME);
 
   return SCM_UNSPECIFIED;
diff --git a/gdb/maint.c b/gdb/maint.c
index 4637fcbc86c..58a54c22940 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -1088,7 +1088,7 @@ set_per_command_cmd (const char *args, int from_tty)
     error (_("Bad value for 'mt set per-command no'."));
 
   for (list = per_command_setlist; list != NULL; list = list->next)
-    if (list->var_type == var_boolean)
+    if (list->var.type () == var_boolean)
       {
 	gdb_assert (list->type == set_cmd);
 	do_set_command (args, from_tty, list);
diff --git a/gdb/python/py-param.c b/gdb/python/py-param.c
index f9dcb076c60..f2d251a19ea 100644
--- a/gdb/python/py-param.c
+++ b/gdb/python/py-param.c
@@ -88,6 +88,19 @@ struct parmpy_object
   const char **enumeration;
 };
 
+/* Wraps a base_setting_wrapper around an existing param_smob.  This abstraction
+   is used
+   to manipulate the value in S->VALUE in a type safe manner the same way as if
+   it was referenced by a setting.  */
+struct setting_wrapper final: base_setting_wrapper
+{
+  explicit setting_wrapper (parmpy_object *s)
+  {
+    m_var_type = s->type;
+    m_var = &s->value;
+  }
+};
+
 extern PyTypeObject parmpy_object_type
     CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("parmpy_object");
 
@@ -110,7 +123,8 @@ get_attr (PyObject *obj, PyObject *attr_name)
     {
       parmpy_object *self = (parmpy_object *) obj;
 
-      return gdbpy_parameter_value (self->type, &self->value);
+      setting_wrapper wrapper { self };
+      return gdbpy_parameter_value (wrapper);
     }
 
   return PyObject_GenericGetAttr (obj, attr_name);
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 690d2fb43c0..6b6ed009091 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -438,7 +438,7 @@ PyObject *gdbpy_create_ptid_object (ptid_t ptid);
 PyObject *gdbpy_selected_thread (PyObject *self, PyObject *args);
 PyObject *gdbpy_selected_inferior (PyObject *self, PyObject *args);
 PyObject *gdbpy_string_to_argv (PyObject *self, PyObject *args);
-PyObject *gdbpy_parameter_value (enum var_types type, void *var);
+PyObject *gdbpy_parameter_value (base_setting_wrapper const & var);
 gdb::unique_xmalloc_ptr<char> gdbpy_parse_command_name
   (const char *name, struct cmd_list_element ***base_list,
    struct cmd_list_element **start_list);
diff --git a/gdb/python/python.c b/gdb/python/python.c
index e42cbc4fd5e..3b2210b5cc5 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -448,9 +448,9 @@ python_command (const char *arg, int from_tty)
    NULL (and set a Python exception) on error.  Helper function for
    get_parameter.  */
 PyObject *
-gdbpy_parameter_value (enum var_types type, void *var)
+gdbpy_parameter_value (base_setting_wrapper const & var)
 {
-  switch (type)
+  switch (var.type ())
     {
     case var_string:
     case var_string_noescape:
@@ -458,16 +458,23 @@ gdbpy_parameter_value (enum var_types type, void *var)
     case var_filename:
     case var_enum:
       {
-	const char *str = *(char **) var;
-
-	if (! str)
+	const char *str;
+        if (var.type () == var_enum)
+          str = var.get<var_enum> ();
+        else
+          str = var.get<var_string,
+			var_string_noescape,
+			var_optional_filename,
+			var_filename> ();
+
+	if (str == nullptr)
 	  str = "";
 	return host_string_to_python_string (str).release ();
       }
 
     case var_boolean:
       {
-	if (* (bool *) var)
+	if (var.get<var_boolean> ())
 	  Py_RETURN_TRUE;
 	else
 	  Py_RETURN_FALSE;
@@ -475,7 +482,7 @@ gdbpy_parameter_value (enum var_types type, void *var)
 
     case var_auto_boolean:
       {
-	enum auto_boolean ab = * (enum auto_boolean *) var;
+	enum auto_boolean ab = var.get<var_auto_boolean> ();
 
 	if (ab == AUTO_BOOLEAN_TRUE)
 	  Py_RETURN_TRUE;
@@ -486,16 +493,18 @@ gdbpy_parameter_value (enum var_types type, void *var)
       }
 
     case var_integer:
-      if ((* (int *) var) == INT_MAX)
+      if (var.get<var_integer> () == INT_MAX)
 	Py_RETURN_NONE;
       /* Fall through.  */
     case var_zinteger:
     case var_zuinteger_unlimited:
-      return gdb_py_object_from_longest (* (int *) var).release ();
+      return gdb_py_object_from_longest
+	(var.get<var_integer, var_zinteger,
+		 var_zuinteger_unlimited> ()).release ();
 
     case var_uinteger:
       {
-	unsigned int val = * (unsigned int *) var;
+	unsigned int val = var.get<var_uinteger> ();
 
 	if (val == UINT_MAX)
 	  Py_RETURN_NONE;
@@ -504,7 +513,7 @@ gdbpy_parameter_value (enum var_types type, void *var)
 
     case var_zuinteger:
       {
-	unsigned int val = * (unsigned int *) var;
+	unsigned int val = var.get<var_zuinteger> ();
 	return gdb_py_object_from_ulongest (val).release ();
       }
     }
@@ -541,10 +550,10 @@ gdbpy_parameter (PyObject *self, PyObject *args)
     return PyErr_Format (PyExc_RuntimeError,
 			 _("Could not find parameter `%s'."), arg);
 
-  if (! cmd->var)
+  if (!cmd->var)
     return PyErr_Format (PyExc_RuntimeError,
 			 _("`%s' is not a parameter."), arg);
-  return gdbpy_parameter_value (cmd->var_type, cmd->var);
+  return gdbpy_parameter_value (cmd->var);
 }
 
 /* Wrapper for target_charset.  */
diff --git a/gdb/remote.c b/gdb/remote.c
index adc53e324d0..f8cabe8ec87 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2243,7 +2243,7 @@ show_remote_protocol_packet_cmd (struct ui_file *file, int from_tty,
        packet < &remote_protocol_packets[PACKET_MAX];
        packet++)
     {
-      if (&packet->detect == c->var)
+      if (&packet->detect == c->var.get_p<var_auto_boolean> ())
 	{
 	  show_packet_config_cmd (packet);
 	  return;
-- 
2.31.1
Lancelot SIX via Gdb-patches July 23, 2021, 7:56 p.m. | #11
> Hi,

> 

> I reworked a bit my previous implementation to see to what extent it can

> support what you are looking for.  Short answer: it can (and it is

> completely transparent to the caller).

> 

> In the previous draft version[1], I introduced a struct that groups

> together the var_type and the pointer to the actual data in order to

> abstract a setting (i.e. a value with a type that can be SET or SHOWn).

> This struct provides getters and setters in order to enforce that we

> cast the pointer to the adequate type.  I have modified this data

> structure so we can register user-provided callbacks.  If those

> functions are provided, the getter and setters will call them to

> generate retrieve the value instead of trying to dereference the

> pointer.

> 

> For the moment, I just made sure that this compiles and I played with

> dummy settings in my local tree.  It is not yet finalized nor fully

> operational because do_set_command will try to free the pointer returned

> by the getter when dealing with char* data.  With user provided

> callbacks memory should probably not be managed at that level but by the

> getter / setter.  Nothing worrying, you have a commit that plans to

> change the char* with std::string, this should solve the issue quite

> nicely.

> 

> As for the previous version, this adds a significant amount of templated

> code that can make the implementation appear quite complex.

> 

> I think I’ll wait for your series to land before I finalize this

> proposal (this is just a POC for the moment) unless you want to build on

> top of this approach to introduce user provided callbacks.


Do you suggest I merge my patch that makes it a union first?  Won't that
be counter productive if you plan to make it a void* again?  I'm a bit
lost now, so I am not sure what's the best order for doing things.

Simon
Lancelot SIX via Gdb-patches July 23, 2021, 8:46 p.m. | #12
Hi,

I plan on spending some more time on it this week end hopefully.

If you do not mind waiting a bit for my patch to be finalized, and want
to go with my approach, there is indeed no point merging yours now.  The
main thing I try to avoid is having both of us working at the same time
on different solutions of the same problem.  Your approach has the
advantage of being almost ready, I can wait for it to be stabilized
before I adapt my work on top on it.  If you are fine waiting for my
approach, I’ll proceed!

My plan is to:

- Improve slightly
  https://sourceware.org/pipermail/gdb-patches/2021-July/180991.html
  (use gdb::Requires instead of static assert, include the python
  binding I forgot because I did not build it, add some selftests of
  the runtime checks introduced by the getters and getters, and ensure
  the documentation is adequate).
- Adapt your patch using std::string on top on my architecture
  (https://sourceware.org/pipermail/gdb-patches/2021-July/180921.html).
- Add a new patch that allows the use of setter / getter (based on what
  was is drafted in
  https://sourceware.org/pipermail/gdb-patches/2021-July/181037.html).
  As a side note, the storage of user provided getter / setter functions
  is done using a unions (void* is not an appropriate for that).  It is
  possible to use the same approach to create getters and setters around
  the union of pointers you provide in your patch (but this would require
  some work, obviously).

Does that makes sense?

Best,
Lancelot.

On Fri, Jul 23, 2021 at 03:56:57PM -0400, Simon Marchi wrote:
> > Hi,

> > 

> > I reworked a bit my previous implementation to see to what extent it can

> > support what you are looking for.  Short answer: it can (and it is

> > completely transparent to the caller).

> > 

> > In the previous draft version[1], I introduced a struct that groups

> > together the var_type and the pointer to the actual data in order to

> > abstract a setting (i.e. a value with a type that can be SET or SHOWn).

> > This struct provides getters and setters in order to enforce that we

> > cast the pointer to the adequate type.  I have modified this data

> > structure so we can register user-provided callbacks.  If those

> > functions are provided, the getter and setters will call them to

> > generate retrieve the value instead of trying to dereference the

> > pointer.

> > 

> > For the moment, I just made sure that this compiles and I played with

> > dummy settings in my local tree.  It is not yet finalized nor fully

> > operational because do_set_command will try to free the pointer returned

> > by the getter when dealing with char* data.  With user provided

> > callbacks memory should probably not be managed at that level but by the

> > getter / setter.  Nothing worrying, you have a commit that plans to

> > change the char* with std::string, this should solve the issue quite

> > nicely.

> > 

> > As for the previous version, this adds a significant amount of templated

> > code that can make the implementation appear quite complex.

> > 

> > I think I’ll wait for your series to land before I finalize this

> > proposal (this is just a POC for the moment) unless you want to build on

> > top of this approach to introduce user provided callbacks.

> 

> Do you suggest I merge my patch that makes it a union first?  Won't that

> be counter productive if you plan to make it a void* again?  I'm a bit

> lost now, so I am not sure what's the best order for doing things.

> 

> Simon
Lancelot SIX via Gdb-patches July 23, 2021, 9:15 p.m. | #13
On 2021-07-23 4:46 p.m., Lancelot SIX wrote:
> Hi,

> 

> I plan on spending some more time on it this week end hopefully.

> 

> If you do not mind waiting a bit for my patch to be finalized, and want

> to go with my approach, there is indeed no point merging yours now.  The

> main thing I try to avoid is having both of us working at the same time

> on different solutions of the same problem.  Your approach has the

> advantage of being almost ready, I can wait for it to be stabilized

> before I adapt my work on top on it.  If you are fine waiting for my

> approach, I’ll proceed!

> 

> My plan is to:

> 

> - Improve slightly

>   https://sourceware.org/pipermail/gdb-patches/2021-July/180991.html

>   (use gdb::Requires instead of static assert, include the python

>   binding I forgot because I did not build it, add some selftests of

>   the runtime checks introduced by the getters and getters, and ensure

>   the documentation is adequate).

> - Adapt your patch using std::string on top on my architecture

>   (https://sourceware.org/pipermail/gdb-patches/2021-July/180921.html).

> - Add a new patch that allows the use of setter / getter (based on what

>   was is drafted in

>   https://sourceware.org/pipermail/gdb-patches/2021-July/181037.html).

>   As a side note, the storage of user provided getter / setter functions

>   is done using a unions (void* is not an appropriate for that).  It is

>   possible to use the same approach to create getters and setters around

>   the union of pointers you provide in your patch (but this would require

>   some work, obviously).

> 

> Does that makes sense?


Yeah, the only thing I would ask: could you verify whether the approach
of allowing getters / setters that you envision can work with my
work-in-progress patch here?  Or if my patch can be changed to work with
your approach.

    https://review.lttng.org/c/binutils-gdb/+/5831

The reason for all this churn and refactor is to fix some bugs (as
described in the commit log of the patch), so I want to make sure
there's a way forward.

Also, could you provide a git branch with your latest patch?  It will be
easier than figuring out on what to apply it.

Simon
Lancelot SIX via Gdb-patches July 23, 2021, 10:55 p.m. | #14
On Fri, Jul 23, 2021 at 05:15:44PM -0400, Simon Marchi wrote:
> On 2021-07-23 4:46 p.m., Lancelot SIX wrote:

> > Hi,

> > 

> > I plan on spending some more time on it this week end hopefully.

> > 

> > If you do not mind waiting a bit for my patch to be finalized, and want

> > to go with my approach, there is indeed no point merging yours now.  The

> > main thing I try to avoid is having both of us working at the same time

> > on different solutions of the same problem.  Your approach has the

> > advantage of being almost ready, I can wait for it to be stabilized

> > before I adapt my work on top on it.  If you are fine waiting for my

> > approach, I’ll proceed!

> > 

> > My plan is to:

> > 

> > - Improve slightly

> >   https://sourceware.org/pipermail/gdb-patches/2021-July/180991.html

> >   (use gdb::Requires instead of static assert, include the python

> >   binding I forgot because I did not build it, add some selftests of

> >   the runtime checks introduced by the getters and getters, and ensure

> >   the documentation is adequate).

> > - Adapt your patch using std::string on top on my architecture

> >   (https://sourceware.org/pipermail/gdb-patches/2021-July/180921.html).

> > - Add a new patch that allows the use of setter / getter (based on what

> >   was is drafted in

> >   https://sourceware.org/pipermail/gdb-patches/2021-July/181037.html).

> >   As a side note, the storage of user provided getter / setter functions

> >   is done using a unions (void* is not an appropriate for that).  It is

> >   possible to use the same approach to create getters and setters around

> >   the union of pointers you provide in your patch (but this would require

> >   some work, obviously).

> > 

> > Does that makes sense?

> 

> Yeah, the only thing I would ask: could you verify whether the approach

> of allowing getters / setters that you envision can work with my

> work-in-progress patch here?  Or if my patch can be changed to work with

> your approach.

> 

>     https://review.lttng.org/c/binutils-gdb/+/5831

> 

> The reason for all this churn and refactor is to fix some bugs (as

> described in the commit log of the patch), so I want to make sure

> there's a way forward.

> 


Yes, the usage seems to be quite close (from the command writer point of
view).  The major diferences at the momment is:
- Your setter returns a bool to let know the caller that the setting has
  been updated where mine does not.  I should change that.

Other than that, you should be able to fit your set_inferior_tty_command
and get_inferior_tty (among others) in my framework quite easily, in a
very similar way.

Your add_setshow_*_cmd are changed from something like

  commands.set->function.set_cmd.set_func.string = set_func;
  commands.show->function.show_cmd.get_func.string = get_func;

to something like

  commands.set->var.set_accessor<var_filename> (set_func, get_func);
  commands.show->var.set_accessor<var_filename> (set_func, get_func);

(I should probably not set both accessors with one function, only one
will ever be used).

> Also, could you provide a git branch with your latest patch?  It will be

> easier than figuring out on what to apply it.


I have pushed my current work (still a work in progress) to the
following branch on sourceware: users/lsix/refactor-typesafe-var.

Lancelot.
> 

> Simon
Lancelot SIX via Gdb-patches July 24, 2021, 2:04 a.m. | #15
> Yes, the usage seems to be quite close (from the command writer point of

> view).  The major diferences at the momment is:

> - Your setter returns a bool to let know the caller that the setting has

>   been updated where mine does not.  I should change that.


Returning a bool is necessary to be able to tell whether we should emit
an MI =param-changed notification or not.  I think that's the only use.

> Other than that, you should be able to fit your set_inferior_tty_command

> and get_inferior_tty (among others) in my framework quite easily, in a

> very similar way.

> 

> Your add_setshow_*_cmd are changed from something like

> 

>   commands.set->function.set_cmd.set_func.string = set_func;

>   commands.show->function.show_cmd.get_func.string = get_func;

> 

> to something like

> 

>   commands.set->var.set_accessor<var_filename> (set_func, get_func);

>   commands.show->var.set_accessor<var_filename> (set_func, get_func);


Great!

> (I should probably not set both accessors with one function, only one

> will ever be used).


Just for the sake of discussion:

Conceptually, I find it a little bit wrong that settings are "part" of
the set and show commands.  Ideally, I think a setting (or parameter,
the two terms are used almost interchangeably) should be decoupled from
the set/show commands.  There are various ways to read setting values,
show commands are just one of them.  So, every time some part of GDB
needs to look up a setting, it actually looks up the corresponding show
command.  I think that settings should be kept in their own registry,
indexed by name.  You would have one setting instance for "non-stop",
for example, with a getter and a setter function.  The "set non-stop"
and "show non-stop" cmd_list_element objects would both point to this
instance and use the setting's setter or getter function.

Other parts of GDB that need to access settings, like the gdb.parameter
Python function, would look up the settings registry.  For example,
gdb.parameter('non-stop') would look up 'non-stop' in that registry, and
not look up the 'show non-stop' command as it does today.

>> Also, could you provide a git branch with your latest patch?  It will be

>> easier than figuring out on what to apply it.

> 

> I have pushed my current work (still a work in progress) to the

> following branch on sourceware: users/lsix/refactor-typesafe-var.


Thanks!

Simon

Patch

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 5ff0b77eb68f..a6b7ef637ab3 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -232,7 +232,7 @@  with_command_1 (const char *set_cmd_prefix,
 					  /*ignore_help_classes=*/ 1);
   gdb_assert (set_cmd != nullptr);
 
-  if (set_cmd->var == nullptr)
+  if (!set_cmd->var.has_value ())
     error (_("Cannot use this setting with the \"with\" command"));
 
   std::string temp_value
@@ -2093,26 +2093,26 @@  value_from_setting (const cmd_list_element *cmd, struct gdbarch *gdbarch)
   switch (cmd->var_type)
     {
     case var_integer:
-      if (*(int *) cmd->var == INT_MAX)
+      if (*cmd->var->int_var == INT_MAX)
 	return value_from_longest (builtin_type (gdbarch)->builtin_int,
 				   0);
       else
 	return value_from_longest (builtin_type (gdbarch)->builtin_int,
-				   *(int *) cmd->var);
+				   *cmd->var->int_var);
     case var_zinteger:
       return value_from_longest (builtin_type (gdbarch)->builtin_int,
-				 *(int *) cmd->var);
+				 *cmd->var->int_var);
     case var_boolean:
       return value_from_longest (builtin_type (gdbarch)->builtin_int,
-				 *(bool *) cmd->var ? 1 : 0);
+				 *cmd->var->bool_var ? 1 : 0);
     case var_zuinteger_unlimited:
       return value_from_longest (builtin_type (gdbarch)->builtin_int,
-				 *(int *) cmd->var);
+				 *cmd->var->int_var);
     case var_auto_boolean:
       {
 	int val;
 
-	switch (*(enum auto_boolean*) cmd->var)
+	switch (*cmd->var->auto_boolean_var)
 	  {
 	  case AUTO_BOOLEAN_TRUE:
 	    val = 1;
@@ -2130,23 +2130,32 @@  value_from_setting (const cmd_list_element *cmd, struct gdbarch *gdbarch)
 				   val);
       }
     case var_uinteger:
-      if (*(unsigned int *) cmd->var == UINT_MAX)
+      if (*cmd->var->unsigned_int_var == UINT_MAX)
 	return value_from_ulongest
 	  (builtin_type (gdbarch)->builtin_unsigned_int, 0);
       else
 	return value_from_ulongest
 	  (builtin_type (gdbarch)->builtin_unsigned_int,
-	   *(unsigned int *) cmd->var);
+	   *cmd->var->unsigned_int_var);
     case var_zuinteger:
       return value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int,
-				  *(unsigned int *) cmd->var);
+				  *cmd->var->unsigned_int_var);
     case var_string:
     case var_string_noescape:
     case var_optional_filename:
     case var_filename:
+      if (*cmd->var->char_ptr_var != nullptr)
+	return value_cstring (*cmd->var->char_ptr_var,
+			      strlen (*cmd->var->char_ptr_var) + 1,
+			      builtin_type (gdbarch)->builtin_char);
+      else
+	return value_cstring ("", 1,
+			      builtin_type (gdbarch)->builtin_char);
+
     case var_enum:
-      if (*(char **) cmd->var)
-	return value_cstring (*(char **) cmd->var, strlen (*(char **) cmd->var),
+      if (*cmd->var->const_char_ptr_var != nullptr)
+	return value_cstring (*cmd->var->const_char_ptr_var,
+			      strlen (*cmd->var->const_char_ptr_var) + 1,
 			      builtin_type (gdbarch)->builtin_char);
       else
 	return value_cstring ("", 1,
@@ -2206,13 +2215,22 @@  str_value_from_setting (const cmd_list_element *cmd, struct gdbarch *gdbarch)
     case var_string_noescape:
     case var_optional_filename:
     case var_filename:
-    case var_enum:
       /* For these cases, we do not use get_setshow_command_value_string,
 	 as this function handle some characters specially, e.g. by
 	 escaping quotes.  So, we directly use the cmd->var string value,
 	 similarly to the value_from_setting code for these cases.  */
-      if (*(char **) cmd->var)
-	return value_cstring (*(char **) cmd->var, strlen (*(char **) cmd->var),
+      if (*cmd->var->char_ptr_var)
+	return value_cstring (*cmd->var->char_ptr_var,
+			      strlen (*cmd->var->char_ptr_var) + 1,
+			      builtin_type (gdbarch)->builtin_char);
+      else
+	return value_cstring ("", 1,
+			      builtin_type (gdbarch)->builtin_char);
+
+    case var_enum:
+      if (*cmd->var->const_char_ptr_var)
+	return value_cstring (*cmd->var->const_char_ptr_var,
+			      strlen (*cmd->var->const_char_ptr_var) + 1,
 			      builtin_type (gdbarch)->builtin_char);
       else
 	return value_cstring ("", 1,
diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 06f3de0f038a..2db3ddc0551b 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -459,7 +459,6 @@  empty_func (const char *args, int from_tty, cmd_list_element *c)
    TYPE is set_cmd or show_cmd.
    CLASS is as in add_cmd.
    VAR_TYPE is the kind of thing we are setting.
-   VAR is address of the variable being controlled by this command.
    DOC is the documentation string.  */
 
 static struct cmd_list_element *
@@ -467,7 +466,6 @@  add_set_or_show_cmd (const char *name,
 		     enum cmd_types type,
 		     enum command_class theclass,
 		     var_types var_type,
-		     void *var,
 		     const char *doc,
 		     struct cmd_list_element **list)
 {
@@ -476,7 +474,6 @@  add_set_or_show_cmd (const char *name,
   gdb_assert (type == set_cmd || type == show_cmd);
   c->type = type;
   c->var_type = var_type;
-  c->var = var;
   /* This needs to be something besides NULL so that this isn't
      treated as a help class.  */
   c->func = empty_func;
@@ -485,8 +482,7 @@  add_set_or_show_cmd (const char *name,
 
 /* Add element named NAME to both the command SET_LIST and SHOW_LIST.
    CLASS is as in add_cmd.  VAR_TYPE is the kind of thing we are
-   setting.  VAR is address of the variable being controlled by this
-   command.  SET_FUNC and SHOW_FUNC are the callback functions (if
+   setting.  SET_FUNC and SHOW_FUNC are the callback functions (if
    non-NULL).  SET_DOC, SHOW_DOC and HELP_DOC are the documentation
    strings.
 
@@ -495,7 +491,7 @@  add_set_or_show_cmd (const char *name,
 static set_show_commands
 add_setshow_cmd_full (const char *name,
 		      enum command_class theclass,
-		      var_types var_type, void *var,
+		      var_types var_type,
 		      const char *set_doc, const char *show_doc,
 		      const char *help_doc,
 		      cmd_func_ftype *set_func,
@@ -518,14 +514,14 @@  add_setshow_cmd_full (const char *name,
       full_set_doc = xstrdup (set_doc);
       full_show_doc = xstrdup (show_doc);
     }
-  set = add_set_or_show_cmd (name, set_cmd, theclass, var_type, var,
+  set = add_set_or_show_cmd (name, set_cmd, theclass, var_type,
 			     full_set_doc, set_list);
   set->doc_allocated = 1;
 
   if (set_func != NULL)
     set->func = set_func;
 
-  show = add_set_or_show_cmd (name, show_cmd, theclass, var_type, var,
+  show = add_set_or_show_cmd (name, show_cmd, theclass, var_type,
 			      full_show_doc, show_list);
   show->doc_allocated = 1;
   show->show_value_func = show_func;
@@ -555,11 +551,17 @@  add_setshow_enum_cmd (const char *name,
 		      struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    =  add_setshow_cmd_full (name, theclass, var_enum, var,
+    =  add_setshow_cmd_full (name, theclass, var_enum,
 			     set_doc, show_doc, help_doc,
 			     set_func, show_func,
 			     set_list, show_list);
+
+  commands.set->var.emplace ();
+  commands.set->var->const_char_ptr_var = var;
   commands.set->enums = enumlist;
+  commands.show->var.emplace ();
+  commands.show->var->const_char_ptr_var = var;
+
   return commands;
 }
 
@@ -583,12 +585,16 @@  add_setshow_auto_boolean_cmd (const char *name,
 			      struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_auto_boolean, var,
+    = add_setshow_cmd_full (name, theclass, var_auto_boolean,
 			    set_doc, show_doc, help_doc,
 			    set_func, show_func,
 			    set_list, show_list);
 
+  commands.set->var.emplace ();
+  commands.set->var->auto_boolean_var = var;
   commands.set->enums = auto_boolean_enums;
+  commands.show->var.emplace ();
+  commands.show->var->auto_boolean_var = var;
 
   return commands;
 }
@@ -612,12 +618,16 @@  add_setshow_boolean_cmd (const char *name, enum command_class theclass, bool *va
 			 struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_boolean, var,
+    = add_setshow_cmd_full (name, theclass, var_boolean,
 			    set_doc, show_doc, help_doc,
 			    set_func, show_func,
 			    set_list, show_list);
 
+  commands.set->var.emplace ();
+  commands.set->var->bool_var = var;
   commands.set->enums = boolean_enums;
+  commands.show->var.emplace ();
+  commands.show->var->bool_var = var;
 
   return commands;
 }
@@ -636,13 +646,18 @@  add_setshow_filename_cmd (const char *name, enum command_class theclass,
 			  struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_filename, var,
+    = add_setshow_cmd_full (name, theclass, var_filename,
 			    set_doc, show_doc, help_doc,
 			    set_func, show_func,
 			    set_list, show_list);
 
   set_cmd_completer (commands.set, filename_completer);
 
+  commands.set->var.emplace ();
+  commands.set->var->char_ptr_var = var;
+  commands.show->var.emplace ();
+  commands.show->var->char_ptr_var = var;
+
   return commands;
 }
 
@@ -660,7 +675,7 @@  add_setshow_string_cmd (const char *name, enum command_class theclass,
 			struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_string, var,
+    = add_setshow_cmd_full (name, theclass, var_string,
 			    set_doc, show_doc, help_doc,
 			    set_func, show_func,
 			    set_list, show_list);
@@ -668,6 +683,11 @@  add_setshow_string_cmd (const char *name, enum command_class theclass,
   /* Disable the default symbol completer.  */
   set_cmd_completer (commands.set, nullptr);
 
+  commands.set->var.emplace ();
+  commands.set->var->char_ptr_var = var;
+  commands.show->var.emplace ();
+  commands.show->var->char_ptr_var = var;
+
   return commands;
 }
 
@@ -685,7 +705,7 @@  add_setshow_string_noescape_cmd (const char *name, enum command_class theclass,
 				 struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_string_noescape, var,
+    = add_setshow_cmd_full (name, theclass, var_string_noescape,
 			    set_doc, show_doc, help_doc,
 			    set_func, show_func,
 			    set_list, show_list);
@@ -693,6 +713,11 @@  add_setshow_string_noescape_cmd (const char *name, enum command_class theclass,
   /* Disable the default symbol completer.  */
   set_cmd_completer (commands.set, nullptr);
 
+  commands.set->var.emplace ();
+  commands.set->var->char_ptr_var = var;
+  commands.show->var.emplace ();
+  commands.show->var->char_ptr_var = var;
+
   return commands;
 }
 
@@ -710,13 +735,18 @@  add_setshow_optional_filename_cmd (const char *name, enum command_class theclass
 				   struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_optional_filename, var,
+    = add_setshow_cmd_full (name, theclass, var_optional_filename,
 			    set_doc, show_doc, help_doc,
 			    set_func, show_func,
 			    set_list, show_list);
 		
   set_cmd_completer (commands.set, filename_completer);
 
+  commands.set->var.emplace ();
+  commands.set->var->char_ptr_var = var;
+  commands.show->var.emplace ();
+  commands.show->var->char_ptr_var = var;
+
   return commands;
 }
 
@@ -754,13 +784,18 @@  add_setshow_integer_cmd (const char *name, enum command_class theclass,
 			 struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_integer, var,
+    = add_setshow_cmd_full (name, theclass, var_integer,
 			    set_doc, show_doc, help_doc,
 			    set_func, show_func,
 			    set_list, show_list);
 
   set_cmd_completer (commands.set, integer_unlimited_completer);
 
+  commands.set->var.emplace ();
+  commands.set->var->int_var = var;
+  commands.show->var.emplace ();
+  commands.show->var->int_var = var;
+
   return commands;
 }
 
@@ -780,13 +815,18 @@  add_setshow_uinteger_cmd (const char *name, enum command_class theclass,
 			  struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_uinteger, var,
+    = add_setshow_cmd_full (name, theclass, var_uinteger,
 			    set_doc, show_doc, help_doc,
 			    set_func, show_func,
 			    set_list, show_list);
 
   set_cmd_completer (commands.set, integer_unlimited_completer);
 
+  commands.set->var.emplace ();
+  commands.set->var->unsigned_int_var = var;
+  commands.show->var.emplace ();
+  commands.show->var->unsigned_int_var = var;
+
   return commands;
 }
 
@@ -805,10 +845,18 @@  add_setshow_zinteger_cmd (const char *name, enum command_class theclass,
 			  struct cmd_list_element **set_list,
 			  struct cmd_list_element **show_list)
 {
-  return add_setshow_cmd_full (name, theclass, var_zinteger, var,
-			       set_doc, show_doc, help_doc,
-			       set_func, show_func,
-			       set_list, show_list);
+  set_show_commands commands
+    = add_setshow_cmd_full (name, theclass, var_zinteger,
+			    set_doc, show_doc, help_doc,
+			    set_func, show_func,
+			    set_list, show_list);
+
+  commands.set->var.emplace ();
+  commands.set->var->int_var = var;
+  commands.show->var.emplace ();
+  commands.show->var->int_var = var;
+
+  return commands;
 }
 
 set_show_commands
@@ -824,13 +872,18 @@  add_setshow_zuinteger_unlimited_cmd (const char *name,
 				     struct cmd_list_element **show_list)
 {
   set_show_commands commands
-    = add_setshow_cmd_full (name, theclass, var_zuinteger_unlimited, var,
+    = add_setshow_cmd_full (name, theclass, var_zuinteger_unlimited,
 			    set_doc, show_doc, help_doc,
 			    set_func, show_func,
 			    set_list, show_list);
 
   set_cmd_completer (commands.set, integer_unlimited_completer);
 
+  commands.set->var.emplace ();
+  commands.set->var->int_var = var;
+  commands.show->var.emplace ();
+  commands.show->var->int_var = var;
+
   return commands;
 }
 
@@ -849,10 +902,18 @@  add_setshow_zuinteger_cmd (const char *name, enum command_class theclass,
 			   struct cmd_list_element **set_list,
 			   struct cmd_list_element **show_list)
 {
-  return add_setshow_cmd_full (name, theclass, var_zuinteger, var,
-			       set_doc, show_doc, help_doc,
-			       set_func, show_func,
-			       set_list, show_list);
+  set_show_commands commands
+    = add_setshow_cmd_full (name, theclass, var_zuinteger,
+			    set_doc, show_doc, help_doc,
+			    set_func, show_func,
+			    set_list, show_list);
+
+  commands.set->var.emplace ();
+  commands.set->var->unsigned_int_var = var;
+  commands.show->var.emplace ();
+  commands.show->var->unsigned_int_var = var;
+
+  return commands;
 }
 
 /* Remove the command named NAME from the command list.  Return the
diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h
index 651d1ef8abb7..ba07d6a69f17 100644
--- a/gdb/cli/cli-decode.h
+++ b/gdb/cli/cli-decode.h
@@ -228,9 +228,34 @@  struct cmd_list_element
      used to finalize the CONTEXT field, if needed.  */
   void (*destroyer) (struct cmd_list_element *self, void *context) = nullptr;
 
-  /* Pointer to variable affected by "set" and "show".  Doesn't
-     matter if type is not_set.  */
-  void *var = nullptr;
+  /* Pointer to variable affected by "set" and "show".
+
+     Not used if TYPE is not_set.
+
+     The effective member of the union depends on VAR_TYPE.  */
+  union setting_variable
+  {
+    /* Used by var_integer, var_zinteger and var_zuinteger_unlimited.  */
+    int *int_var;
+
+    /* Used by var_uinteger and var_zuinteger.  */
+    unsigned int *unsigned_int_var;
+
+    /* Used by var_boolean.  */
+    bool *bool_var;
+
+    /* Used by var_auto_boolean.  */
+    auto_boolean *auto_boolean_var;
+
+    /* Used by var_filename, var_string, var_string_noescape and
+       var_optional_filename.  */
+    char **char_ptr_var;
+
+    /* Used by var_enum.  */
+    const char **const_char_ptr_var;
+  };
+
+  gdb::optional<setting_variable> var;
 
   /* Pointer to NULL terminated list of enumerated values (like
      argv).  */
diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index 0290acede7fa..8f456b6fc14b 100644
--- a/gdb/cli/cli-setshow.c
+++ b/gdb/cli/cli-setshow.c
@@ -353,11 +353,11 @@  do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	*q++ = '\0';
 	newobj = (char *) xrealloc (newobj, q - newobj);
 
-	if (*(char **) c->var == NULL
-	    || strcmp (*(char **) c->var, newobj) != 0)
+	if (*c->var->char_ptr_var == NULL
+	    || strcmp (*c->var->char_ptr_var, newobj) != 0)
 	  {
-	    xfree (*(char **) c->var);
-	    *(char **) c->var = newobj;
+	    xfree (*c->var->char_ptr_var);
+	    *c->var->char_ptr_var = newobj;
 
 	    option_changed = 1;
 	  }
@@ -366,10 +366,10 @@  do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
       }
       break;
     case var_string_noescape:
-      if (*(char **) c->var == NULL || strcmp (*(char **) c->var, arg) != 0)
+      if (*c->var->char_ptr_var == NULL || strcmp (*c->var->char_ptr_var, arg) != 0)
 	{
-	  xfree (*(char **) c->var);
-	  *(char **) c->var = xstrdup (arg);
+	  xfree (*c->var->char_ptr_var);
+	  *c->var->char_ptr_var = xstrdup (arg);
 
 	  option_changed = 1;
 	}
@@ -398,11 +398,11 @@  do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	else
 	  val = xstrdup ("");
 
-	if (*(char **) c->var == NULL
-	    || strcmp (*(char **) c->var, val) != 0)
+	if (*c->var->char_ptr_var == NULL
+	    || strcmp (*c->var->char_ptr_var, val) != 0)
 	  {
-	    xfree (*(char **) c->var);
-	    *(char **) c->var = val;
+	    xfree (*c->var->char_ptr_var);
+	    *c->var->char_ptr_var = val;
 
 	    option_changed = 1;
 	  }
@@ -416,9 +416,9 @@  do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 
 	if (val < 0)
 	  error (_("\"on\" or \"off\" expected."));
-	if (val != *(bool *) c->var)
+	if (val != *c->var->bool_var)
 	  {
-	    *(bool *) c->var = val;
+	    *c->var->bool_var = val;
 
 	    option_changed = 1;
 	  }
@@ -428,9 +428,9 @@  do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
       {
 	enum auto_boolean val = parse_auto_binary_operation (arg);
 
-	if (*(enum auto_boolean *) c->var != val)
+	if (*c->var->auto_boolean_var != val)
 	  {
-	    *(enum auto_boolean *) c->var = val;
+	    *c->var->auto_boolean_var = val;
 
 	    option_changed = 1;
 	  }
@@ -441,9 +441,9 @@  do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
       {
 	unsigned int val = parse_cli_var_uinteger (c->var_type, &arg, true);
 
-	if (*(unsigned int *) c->var != val)
+	if (*c->var->unsigned_int_var != val)
 	  {
-	    *(unsigned int *) c->var = val;
+	    *c->var->unsigned_int_var = val;
 
 	    option_changed = 1;
 	  }
@@ -477,9 +477,9 @@  do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 		 || (c->var_type == var_zinteger && val > INT_MAX))
 	  error (_("integer %s out of range"), plongest (val));
 
-	if (*(int *) c->var != val)
+	if (*c->var->int_var != val)
 	  {
-	    *(int *) c->var = val;
+	    *c->var->int_var = val;
 
 	    option_changed = 1;
 	  }
@@ -495,9 +495,9 @@  do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	if (*after != '\0')
 	  error (_("Junk after item \"%.*s\": %s"), len, arg, after);
 
-	if (*(const char **) c->var != match)
+	if (*c->var->const_char_ptr_var != match)
 	  {
-	    *(const char **) c->var = match;
+	    *c->var->const_char_ptr_var = match;
 
 	    option_changed = 1;
 	  }
@@ -507,9 +507,9 @@  do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
       {
 	int val = parse_cli_var_zuinteger_unlimited (&arg, true);
 
-	if (*(int *) c->var != val)
+	if (*c->var->int_var != val)
 	  {
-	    *(int *) c->var = val;
+	    *c->var->int_var = val;
 	    option_changed = 1;
 	  }
       }
@@ -584,19 +584,23 @@  do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	case var_string_noescape:
 	case var_filename:
 	case var_optional_filename:
+	  gdb::observers::command_param_changed.notify
+	    (name, *c->var->char_ptr_var);
+	  break;
 	case var_enum:
-	  gdb::observers::command_param_changed.notify (name, *(char **) c->var);
+	  gdb::observers::command_param_changed.notify
+	    (name, *c->var->const_char_ptr_var);
 	  break;
 	case var_boolean:
 	  {
-	    const char *opt = *(bool *) c->var ? "on" : "off";
+	    const char *opt = *c->var->bool_var ? "on" : "off";
 
 	    gdb::observers::command_param_changed.notify (name, opt);
 	  }
 	  break;
 	case var_auto_boolean:
 	  {
-	    const char *s = auto_boolean_enums[*(enum auto_boolean *) c->var];
+	    const char *s = auto_boolean_enums[*c->var->auto_boolean_var];
 
 	    gdb::observers::command_param_changed.notify (name, s);
 	  }
@@ -606,7 +610,7 @@  do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	  {
 	    char s[64];
 
-	    xsnprintf (s, sizeof s, "%u", *(unsigned int *) c->var);
+	    xsnprintf (s, sizeof s, "%u", *c->var->unsigned_int_var);
 	    gdb::observers::command_param_changed.notify (name, s);
 	  }
 	  break;
@@ -616,7 +620,7 @@  do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
 	  {
 	    char s[64];
 
-	    xsnprintf (s, sizeof s, "%d", *(int *) c->var);
+	    xsnprintf (s, sizeof s, "%d", *c->var->int_var);
 	    gdb::observers::command_param_changed.notify (name, s);
 	  }
 	  break;
@@ -635,21 +639,24 @@  get_setshow_command_value_string (const cmd_list_element *c)
   switch (c->var_type)
     {
     case var_string:
-      if (*(char **) c->var)
-	stb.putstr (*(char **) c->var, '"');
+      if (*c->var->char_ptr_var)
+	stb.putstr (*c->var->char_ptr_var, '"');
       break;
     case var_string_noescape:
     case var_optional_filename:
     case var_filename:
+      if (*c->var->char_ptr_var)
+	stb.puts (*c->var->char_ptr_var);
+      break;
     case var_enum:
-      if (*(char **) c->var)
-	stb.puts (*(char **) c->var);
+      if (*c->var->const_char_ptr_var)
+	stb.puts (*c->var->const_char_ptr_var);
       break;
     case var_boolean:
-      stb.puts (*(bool *) c->var ? "on" : "off");
+      stb.puts (*c->var->bool_var ? "on" : "off");
       break;
     case var_auto_boolean:
-      switch (*(enum auto_boolean*) c->var)
+      switch (*c->var->auto_boolean_var)
 	{
 	case AUTO_BOOLEAN_TRUE:
 	  stb.puts ("on");
@@ -668,25 +675,25 @@  get_setshow_command_value_string (const cmd_list_element *c)
     case var_uinteger:
     case var_zuinteger:
       if (c->var_type == var_uinteger
-	  && *(unsigned int *) c->var == UINT_MAX)
+	  && *c->var->unsigned_int_var == UINT_MAX)
 	stb.puts ("unlimited");
       else
-	stb.printf ("%u", *(unsigned int *) c->var);
+	stb.printf ("%u", *c->var->unsigned_int_var);
       break;
     case var_integer:
     case var_zinteger:
       if (c->var_type == var_integer
-	  && *(int *) c->var == INT_MAX)
+	  && *c->var->int_var == INT_MAX)
 	stb.puts ("unlimited");
       else
-	stb.printf ("%d", *(int *) c->var);
+	stb.printf ("%d", *c->var->int_var);
       break;
     case var_zuinteger_unlimited:
       {
-	if (*(int *) c->var == -1)
+	if (*c->var->int_var == -1)
 	  stb.puts ("unlimited");
 	else
-	  stb.printf ("%d", *(int *) c->var);
+	  stb.printf ("%d", *c->var->int_var);
       }
       break;
     default:
diff --git a/gdb/guile/scm-param.c b/gdb/guile/scm-param.c
index 44ea167be277..57a8c7ec68a2 100644
--- a/gdb/guile/scm-param.c
+++ b/gdb/guile/scm-param.c
@@ -133,7 +133,8 @@  static SCM unlimited_keyword;
 
 static int pascm_is_valid (param_smob *);
 static const char *pascm_param_type_name (enum var_types type);
-static SCM pascm_param_value (enum var_types type, void *var,
+static SCM pascm_param_value (enum var_types type,
+			      const pascm_variable *var,
 			      int arg_pos, const char *func_name);
 
 /* Administrivia for parameter smobs.  */
@@ -563,10 +564,13 @@  pascm_param_type_name (enum var_types param_type)
 }
 
 /* Return the value of a gdb parameter as a Scheme value.
-   If TYPE is not supported, then a <gdb:exception> object is returned.  */
+   If TYPE is not supported, then a <gdb:exception> object is returned.
+
+   This overload can be used on Guile-created parameters, even if they
+   haven't been registered yet.  */
 
 static SCM
-pascm_param_value (enum var_types type, void *var,
+pascm_param_value (enum var_types type, const pascm_variable *param_value,
 		   int arg_pos, const char *func_name)
 {
   /* Note: We *could* support var_integer here in case someone is trying to get
@@ -579,9 +583,18 @@  pascm_param_value (enum var_types type, void *var,
     case var_string_noescape:
     case var_optional_filename:
     case var_filename:
+      {
+	const char *str = param_value->stringval;
+
+	if (str == NULL)
+	  str = "";
+
+	return gdbscm_scm_from_host_string (str, strlen (str));
+      }
+
     case var_enum:
       {
-	const char *str = *(char **) var;
+	const char *str = param_value->cstringval;
 
 	if (str == NULL)
 	  str = "";
@@ -590,7 +603,7 @@  pascm_param_value (enum var_types type, void *var,
 
     case var_boolean:
       {
-	if (* (bool *) var)
+	if (param_value->boolval)
 	  return SCM_BOOL_T;
 	else
 	  return SCM_BOOL_F;
@@ -598,7 +611,7 @@  pascm_param_value (enum var_types type, void *var,
 
     case var_auto_boolean:
       {
-	enum auto_boolean ab = * (enum auto_boolean *) var;
+	enum auto_boolean ab = param_value->autoboolval;
 
 	if (ab == AUTO_BOOLEAN_TRUE)
 	  return SCM_BOOL_T;
@@ -609,19 +622,19 @@  pascm_param_value (enum var_types type, void *var,
       }
 
     case var_zuinteger_unlimited:
-      if (* (int *) var == -1)
+      if (param_value->intval == -1)
 	return unlimited_keyword;
-      gdb_assert (* (int *) var >= 0);
+      gdb_assert (param_value->intval >= 0);
       /* Fall through.  */
     case var_zinteger:
-      return scm_from_int (* (int *) var);
+      return scm_from_int (param_value->intval);
 
     case var_uinteger:
-      if (* (unsigned int *) var == UINT_MAX)
+      if (param_value->uintval == UINT_MAX)
 	return unlimited_keyword;
       /* Fall through.  */
     case var_zuinteger:
-      return scm_from_uint (* (unsigned int *) var);
+      return scm_from_uint (param_value->uintval);
 
     default:
       break;
@@ -632,6 +645,92 @@  pascm_param_value (enum var_types type, void *var,
 					 _("program error: unhandled type"));
 }
 
+/* Return the value of a gdb parameter as a Scheme value.
+   If SHOW_CMD::VAR_TYPE is not supported, then a <gdb:exception> object is
+   returned.
+
+   This overload can be used with any parameter that has an associated show
+   command, such as built-in commands and registered Guile parameters.  */
+
+static SCM
+pascm_param_value (const cmd_list_element *show_cmd,
+		   int arg_pos, const char *func_name)
+{
+  /* Note: We *could* support var_integer here in case someone is trying to get
+     the value of a Python-created parameter (which is the only place that
+     still supports var_integer).  To further discourage its use we do not.  */
+
+  gdb_assert (show_cmd->type == cmd_types::show_cmd);
+
+  switch (show_cmd->var_type)
+    {
+    case var_string:
+    case var_string_noescape:
+    case var_optional_filename:
+    case var_filename:
+      {
+	const char *str = *show_cmd->var->char_ptr_var;
+
+	if (str == NULL)
+	  str = "";
+
+	return gdbscm_scm_from_host_string (str, strlen (str));
+      }
+
+    case var_enum:
+      {
+	const char *str = *show_cmd->var->const_char_ptr_var;
+
+	if (str == NULL)
+	  str = "";
+
+	return gdbscm_scm_from_host_string (str, strlen (str));
+      }
+
+    case var_boolean:
+      {
+	if (*show_cmd->var->bool_var)
+	  return SCM_BOOL_T;
+	else
+	  return SCM_BOOL_F;
+      }
+
+    case var_auto_boolean:
+      {
+	enum auto_boolean ab = *show_cmd->var->auto_boolean_var;
+
+	if (ab == AUTO_BOOLEAN_TRUE)
+	  return SCM_BOOL_T;
+	else if (ab == AUTO_BOOLEAN_FALSE)
+	  return SCM_BOOL_F;
+	else
+	  return auto_keyword;
+      }
+
+    case var_zuinteger_unlimited:
+      if (*show_cmd->var->int_var == -1)
+	return unlimited_keyword;
+      gdb_assert (*show_cmd->var->int_var >= 0);
+      /* Fall through.  */
+    case var_zinteger:
+      return scm_from_int (*show_cmd->var->int_var);
+
+    case var_uinteger:
+      if (*show_cmd->var->unsigned_int_var == UINT_MAX)
+	return unlimited_keyword;
+      /* Fall through.  */
+    case var_zuinteger:
+      return scm_from_uint (*show_cmd->var->unsigned_int_var);
+
+    default:
+      break;
+    }
+
+  return gdbscm_make_out_of_range_error (func_name, arg_pos,
+					 scm_from_int (show_cmd->var_type),
+					 _("program error: unhandled type"));
+}
+
 /* Set the value of a parameter of type TYPE in VAR from VALUE.
    ENUMERATION is the list of enum values for enum parameters, otherwise NULL.
    Throws a Scheme exception if VALUE_SCM is invalid for TYPE.  */
@@ -1062,13 +1161,13 @@  gdbscm_parameter_value (SCM self)
 	  gdbscm_out_of_range_error (FUNC_NAME, SCM_ARG1, self,
 				     _("parameter not found"));
 	}
-      if (cmd->var == NULL)
+      if (!cmd->var.has_value ())
 	{
 	  gdbscm_out_of_range_error (FUNC_NAME, SCM_ARG1, self,
 				     _("not a parameter"));
 	}
 
-      return pascm_param_value (cmd->var_type, cmd->var, SCM_ARG1, FUNC_NAME);
+      return pascm_param_value (cmd, SCM_ARG1, FUNC_NAME);
     }
 }
 
diff --git a/gdb/python/py-param.c b/gdb/python/py-param.c
index f9dcb076c603..b83e3d080eb3 100644
--- a/gdb/python/py-param.c
+++ b/gdb/python/py-param.c
@@ -86,6 +86,9 @@  struct parmpy_object
      allocated with xmalloc, as is each element.  It is
      NULL-terminated.  */
   const char **enumeration;
+
+  /* The commands created as a result of creating this parameter.  */
+  set_show_commands commands;
 };
 
 extern PyTypeObject parmpy_object_type
@@ -110,7 +113,7 @@  get_attr (PyObject *obj, PyObject *attr_name)
     {
       parmpy_object *self = (parmpy_object *) obj;
 
-      return gdbpy_parameter_value (self->type, &self->value);
+      return gdbpy_parameter_value (self->commands.show);
     }
 
   return PyObject_GenericGetAttr (obj, attr_name);
@@ -572,6 +575,9 @@  add_setshow_generic (int parmclass, enum command_class cmdclass,
   commands.set->set_context (self);
   commands.show->set_context (self);
 
+  /* Save the created set/show commands.  */
+  self->commands = commands;
+
   /* We (unfortunately) currently leak the command name.  */
   cmd_name.release ();
 }
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 690d2fb43c06..db467540c473 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -438,7 +438,7 @@  PyObject *gdbpy_create_ptid_object (ptid_t ptid);
 PyObject *gdbpy_selected_thread (PyObject *self, PyObject *args);
 PyObject *gdbpy_selected_inferior (PyObject *self, PyObject *args);
 PyObject *gdbpy_string_to_argv (PyObject *self, PyObject *args);
-PyObject *gdbpy_parameter_value (enum var_types type, void *var);
+PyObject * gdbpy_parameter_value (const cmd_list_element *show_cmd);
 gdb::unique_xmalloc_ptr<char> gdbpy_parse_command_name
   (const char *name, struct cmd_list_element ***base_list,
    struct cmd_list_element **start_list);
diff --git a/gdb/python/python.c b/gdb/python/python.c
index e42cbc4fd5ee..c5641c4db8f7 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -448,26 +448,38 @@  python_command (const char *arg, int from_tty)
    NULL (and set a Python exception) on error.  Helper function for
    get_parameter.  */
 PyObject *
-gdbpy_parameter_value (enum var_types type, void *var)
+gdbpy_parameter_value (const cmd_list_element *show_cmd)
 {
-  switch (type)
+  gdb_assert (show_cmd->type == cmd_types::show_cmd);
+
+  switch (show_cmd->var_type)
     {
     case var_string:
     case var_string_noescape:
     case var_optional_filename:
     case var_filename:
+      {
+	const char *str = *show_cmd->var->char_ptr_var;
+
+	if (str == nullptr)
+	  str = "";
+
+	return host_string_to_python_string (str).release ();
+      }
+
     case var_enum:
       {
-	const char *str = *(char **) var;
+	const char *str = *show_cmd->var->const_char_ptr_var;
 
-	if (! str)
+	if (str == nullptr)
 	  str = "";
+
 	return host_string_to_python_string (str).release ();
       }
 
     case var_boolean:
       {
-	if (* (bool *) var)
+	if (*show_cmd->var->bool_var)
 	  Py_RETURN_TRUE;
 	else
 	  Py_RETURN_FALSE;
@@ -475,7 +487,7 @@  gdbpy_parameter_value (enum var_types type, void *var)
 
     case var_auto_boolean:
       {
-	enum auto_boolean ab = * (enum auto_boolean *) var;
+	enum auto_boolean ab = *show_cmd->var->auto_boolean_var;
 
 	if (ab == AUTO_BOOLEAN_TRUE)
 	  Py_RETURN_TRUE;
@@ -486,16 +498,16 @@  gdbpy_parameter_value (enum var_types type, void *var)
       }
 
     case var_integer:
-      if ((* (int *) var) == INT_MAX)
+      if (*show_cmd->var->int_var == INT_MAX)
 	Py_RETURN_NONE;
       /* Fall through.  */
     case var_zinteger:
     case var_zuinteger_unlimited:
-      return gdb_py_object_from_longest (* (int *) var).release ();
+      return gdb_py_object_from_longest (*show_cmd->var->int_var).release ();
 
     case var_uinteger:
       {
-	unsigned int val = * (unsigned int *) var;
+	unsigned int val = *show_cmd->var->unsigned_int_var;
 
 	if (val == UINT_MAX)
 	  Py_RETURN_NONE;
@@ -504,7 +516,7 @@  gdbpy_parameter_value (enum var_types type, void *var)
 
     case var_zuinteger:
       {
-	unsigned int val = * (unsigned int *) var;
+	unsigned int val = *show_cmd->var->unsigned_int_var;
 	return gdb_py_object_from_ulongest (val).release ();
       }
     }
@@ -544,7 +556,8 @@  gdbpy_parameter (PyObject *self, PyObject *args)
   if (! cmd->var)
     return PyErr_Format (PyExc_RuntimeError,
 			 _("`%s' is not a parameter."), arg);
-  return gdbpy_parameter_value (cmd->var_type, cmd->var);
+
+  return gdbpy_parameter_value (cmd);
 }
 
 /* Wrapper for target_charset.  */
diff --git a/gdb/remote.c b/gdb/remote.c
index e2a08c9a113d..87981330ce87 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2244,7 +2244,7 @@  show_remote_protocol_packet_cmd (struct ui_file *file, int from_tty,
        packet < &remote_protocol_packets[PACKET_MAX];
        packet++)
     {
-      if (&packet->detect == c->var)
+      if (&packet->detect == c->var->auto_boolean_var)
 	{
 	  show_packet_config_cmd (packet);
 	  return;