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

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

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