Re: [PATCH v5 6/8] GDB: Allow arbitrary keywords in integer set commands

["Maciej W. Rozycki" <macro@embecosm.com>]

On Tue, 28 Jun 2022, Andrew Burgess wrote:

> > Rather than just `unlimited' allow the integer set commands (or command 
> > options) to define arbitrary keywords for the user to use, removing 
> > hardcoded arrangements for the `unlimited' keyword.
> >
> > Make obsolete the confusingly named `var_zinteger', `var_zuinteger' and 
> > `var_zuinteger_unlimited' `set'/`show' command variable types redefining 
> > them in terms of `var_uinteger', `var_integer' and `var_pinteger', which 
> > have the range of [0;UINT_MAX], [INT_MIN;INT_MAX], and [0;INT_MAX] each.
> >
> > Following existing practice `var_pinteger' allows extra negative values 
> > to be used, however unlike `var_zuinteger_unlimited' any number of such 
> > values can be defined rather than just `-1'.
> >
> > The "p" in `var_pinteger' stands for "positive", for the lack of a more 
> > appropriate unambiguous letter, even though 0 obviously is not positive; 
> > "n" would be confusing as to whether it stands for "non-negative" or 
> > "negative".
> 
> While looking at this patch I was thinking to myself, I wonder why we
> need var_pinteger, can't we achieve the same thing using var_uinteger
> and different literals?

 We probably could.  We'd have to forbid values within (INT_MAX;UINT_MAX] 
however somehow, and it's not clear to me such an arrangement is worth the 
complication.

 Mind that `var_pinteger' is a generalisation of `var_zuinteger_unlimited' 
and whoever came with `var_zuinteger_unlimited' must have had a reason not 
to define it in terms of an unsigned integer in the first place (and use 
UINT_MAX for `unlimited'), most likely to avoid values beyond INT_MAX.

> So I think I've convinced myself that you don't actually make use of
> var_pinteger in this patch, and I'm pretty sure you don't use it in any
> of the later patches either.
> 
> Maybe I'm wrong, so could you point out where it's used?

 See `add_setshow_zuinteger_unlimited_cmd' (and `add_setshow_pinteger_cmd' 
and `add_setshow_cmds_for_options' obviously as well; the idea here is to 
gradually move away from `add_setshow_zuinteger_unlimited_cmd' as parts of 
code are converted to using `add_setshow_cmds_for_options' referring to 
`zuinteger_unlimited_literals' as extra literals rather than individual 
calls to `add_setshow_*_cmd' functions).

> If it's not used then I think we should remove it.
> 
> If it's only used in a later commit then I think we should split its
> addition out into a later commit.
> 
> If its used in this commit, then I'm sorry for missing it!

 Your concerns however have prompted me to actually make one step further 
and remove `var_zinteger', `var_zuinteger' and `var_zuinteger_unlimited' 
enumeration constants altogether, even from the Guile/Scheme and Python 
interface modules, which should hopefully reduce any confusion that may 
remain.  This has actually made arrangements for extra literals more 
straightforward in the interface modules and while I have not implemented 
Scheme or Python syntax for defining new settings with arbitrary extra 
literals, it should now be pretty straightforward for anyone fluent in 
these languages.  And it made another pair of places where `var_pinteger' 
is referred from.

> > Index: src/gdb/cli/cli-option.h
> > ===================================================================
> > --- src.orig/gdb/cli/cli-option.h
> > +++ src/gdb/cli/cli-option.h
[...]
> > @@ -207,11 +211,12 @@ struct uinteger_option_def : option_def
> >  {
> >    uinteger_option_def (const char *long_option_,
> >  		       unsigned int *(*get_var_address_cb_) (Context *),
> > +		       const literal_def *extra_literals_,
> >  		       show_value_ftype *show_cmd_cb_,
> >  		       const char *set_doc_,
> >  		       const char *show_doc_ = nullptr,
> >  		       const char *help_doc_ = nullptr)
> > -    : option_def (long_option_, var_uinteger,
> > +    : option_def (long_option_, var_uinteger, extra_literals_,
> >  		  (erased_get_var_address_ftype *) get_var_address_cb_,
> >  		  show_cmd_cb_,
> >  		  set_doc_, show_doc_, help_doc_)
> 
> I think we should add a second constructor for uinteger_option_def like
> this:
> 
>   uinteger_option_def (const char *long_option_,
>                       unsigned int *(*get_var_address_cb_) (Context *),
>                       show_value_ftype *show_cmd_cb_,
>                       const char *set_doc_,
>                       const char *show_doc_ = nullptr,
>                       const char *help_doc_ = nullptr)
>     : uinteger_option_def (long_option_, get_var_address_cb_,
>                           uinteger_unlimited_literals, show_cmd_cb_,
>                           set_doc_, show_doc_, help_doc_)
>   { /* Nothing.  */ }
> 
> This would keep the existing '0 == unlimited' as the default behaviour,
> but still give the option of overriding the literals if needed.

 I think this would be going against the spirit of my change though where 
`uinteger' is just a plain unsigned integer with no extra semantic 
features implied, which is what any software engineer not familiar with 
GDB's convoluted history would assume.  Myself I have had to double-check 
the description of `var_types' enumeration constants every time while 
working on this change so as not to get the meanings confused.

 With your proposed update one would have to pass `nullptr' explicitly as 
`extra_literals' to get a plain unsigned integer setting and I find it 
backwards.  I find it more natural for a missing initialiser to mean 
`nullptr' and I have added suitable extra constructors now.

 NB eventually we'll need an `integer_option_def' class here as well, but 
I'm leaving it out until there's a user for it; I could perhaps convert 
say gdb/source.c for demonstrational purposes, but I'm not that inclined.

> > @@ -220,18 +225,19 @@ struct uinteger_option_def : option_def
> >    }
> >  };
> >  
> > -/* A var_zuinteger_unlimited command line option.  */
> > +/* A var_integer command line option.  */
> >  
> >  template<typename Context>
> > -struct zuinteger_unlimited_option_def : option_def
> 
> If instead of deleting zuinteger_unlimited_option_def we redefined the
> class like this:
> 
>   /* A var_zuinteger_unlimited command line option.  */
>   
>   template<typename Context>
>   struct zuinteger_unlimited_option_def : integer_option_def<Context>
>   {
>     zuinteger_unlimited_option_def (const char *long_option_,
>                                    int *(*get_var_address_cb_) (Context *),
>                                    show_value_ftype *show_cmd_cb_,
>                                    const char *set_doc_,
>                                    const char *show_doc_ = nullptr,
>                                    const char *help_doc_ = nullptr)
>       : integer_option_def<Context> (long_option_, get_var_address_cb_,
>                                     zuinteger_unlimited_literals, show_cmd_cb_,
>                                     set_doc_, show_doc_, help_doc_)
>     { /* Nothing.  */ }
>   };
> 
> We could then still make use of zuinteger_unlimited_option_def if we
> wanted, which would be an integer with zuinteger_unlimited_literals.

 Like above, I'd prefer to get rid of the convoluted name (and this needs 
to be called `pinteger_option_def'; so much for my confusion avoidance).

 Also the more I think about it the more I am convinced all the three 
variables holding the "unlimited" extra literal simply ought to match the 
name of the corresponding `var_types' enumeration constant, so 
`pinteger_unlimited_literals' here and elsewhere.

 Ultimately we'll need `integer_option_def' too, but there's no user 
currently (settings of the `var_integer' type are only created via the 
legacy `add_setshow_integer_cmd' function, which I think should eventually 
go away).

> > Index: src/gdb/cli/cli-setshow.c
> > ===================================================================
> > --- src.orig/gdb/cli/cli-setshow.c
> > +++ src/gdb/cli/cli-setshow.c
[...]
> >  /* See cli-setshow.h.  */
> >  
> > -unsigned int
> > -parse_cli_var_uinteger (var_types var_type, const char **arg,
> > -			bool expression)
> > +LONGEST
> > +parse_cli_var_integer (var_types var_type, const literal_def *extra_literals,
> > +		       const char **arg, bool expression)
> >  {
> >    LONGEST val;
> >  
> >    if (*arg == nullptr || **arg == '\0')
> >      {
> > -      if (var_type == var_uinteger)
> > -	error_no_arg (_("integer to set it to, or \"unlimited\""));
> > -      else
> > +      if (extra_literals == nullptr)
> >  	error_no_arg (_("integer to set it to"));
> > -    }
> > -
> > -  if (var_type == var_uinteger && is_unlimited_literal (arg, expression))
> > -    val = 0;
> > -  else if (expression)
> > -    val = parse_and_eval_long (*arg);
> > -  else
> > -    val = get_ulongest (arg);
> > -
> > -  if (var_type == var_uinteger && val == 0)
> > -    val = UINT_MAX;
> > -  else if (val < 0
> > -	   /* For var_uinteger, don't let the user set the value
> > -	      to UINT_MAX directly, as that exposes an
> > -	      implementation detail to the user interface.  */
> > -	   || (var_type == var_uinteger && val >= UINT_MAX)
> > -	   || (var_type == var_zuinteger && val > UINT_MAX))
> > -    error (_("integer %s out of range"), plongest (val));
> > -
> > -  return val;
> > -}
> > -
> > -/* See cli-setshow.h.  */
> > +      else
> > +	{
> > +	  std::string buffer = "";
> > +	  size_t count = 0;
> >  
> > -int
> > -parse_cli_var_zuinteger_unlimited (const char **arg, bool expression)
> > -{
> > -  LONGEST val;
> > +	  for (const literal_def *l = extra_literals;
> > +	       l->literal != nullptr;
> > +	       l++, count++)
> > +	    {
> > +	      if (count != 0)
> > +		buffer += ", ";
> > +	      buffer = buffer + '"' + l->literal + '"';
> > +	    }
> > +	  if (count > 1)
> > +	    error_no_arg
> > +	      (string_printf (_("integer to set it to, or one of: %s"),
> > +			      buffer.c_str ()).c_str ());
> > +	  else
> > +	    error_no_arg
> > +	      (string_printf (_("integer to set it to, or %s"),
> > +			      buffer.c_str ()).c_str ());
> > +	}
> > +    }
> >  
> > -  if (*arg == nullptr || **arg == '\0')
> > -    error_no_arg (_("integer to set it to, or \"unlimited\""));
> > +  if (!get_literal_val (val, extra_literals, arg, expression))
> > +    {
> > +      if (expression)
> > +	val = parse_and_eval_long (*arg);
> > +      else
> > +	val = get_ulongest (arg);
> >  
> > -  if (is_unlimited_literal (arg, expression))
> > -    val = -1;
> > -  else if (expression)
> > -    val = parse_and_eval_long (*arg);
> > -  else
> > -    val = get_ulongest (arg);
> > +      enum tribool allowed = TRIBOOL_UNKNOWN;
> > +      if (extra_literals != nullptr)
> > +	{
> > +	  for (const literal_def *l = extra_literals;
> > +	       l->literal != nullptr;
> > +	       l++)
> > +	    if (val == l->val)
> > +	      {
> > +		allowed = l->allow ? TRIBOOL_TRUE : TRIBOOL_FALSE;
> > +		val = l->use;
> > +		break;
> > +	      }
> > +	    else if (val == l->use)
> > +	      {
> > +		allowed = TRIBOOL_FALSE;
> > +		break;
> > +	      }
> > +	}
> 
> I haven't run the testsuite on this, so maybe there's issues, but I'd
> like to propose that this loop be rewritten as:
> 
>       if (extra_literals != nullptr)
> 	{
> 	  for (const literal_def *l = extra_literals;
> 	       l->literal != nullptr;
> 	       l++)
> 	    if (val == l->val && l->allow)
> 	      {
> 		allowed = TRIBOOL_TRUE;
> 		val = l->use;
> 		break;
> 	      }
> 	    else if (val == l->use)
> 	      allowed = TRIBOOL_FALSE;
> 	}

 This has a problem where `l->val' is not allowed with one entry and it 
doesn't match any other.  In that case execution will fall through to 
TRIBOOL_UNKNOWN below and the value will be incorrectly accepted as long 
as within the range of the requested `var_type'.

> The 'if (val == l->val && l->allow)' would be improved if l->val was
> changed to an optional.

 I like this idea (mind that I'm a C old-timer really and I haven't still 
soaked with all the C++ features we have at hand) and I have updated code 
accordingly.

>  The change to the 'val == l->use' block allows
> I think another improvement, this:
> 
>   static const literal_def print_characters_literals[] =
>     {
>       { "elements", 0, 0, false },
>       { "unlimited", 0, UINT_MAX, true },
>       { nullptr }
>     };
> 
> The change here is that:
> 
>   set print characters 0
> 
> now behaves the same as:
> 
>   set print elements 0
> 
> In both cases, this sets the value to "unlimited".  I like the
> consistency.

 You needn't change the loop to achieve this effect, you can just swap the 
entries in the array or use UINT_MAX as `val' for "elements".  And with 
optional `l->val' you would actually still have to swap the entries as 
`l->use' would match against 0 first and reject it.

 The idea with my change was not to expose the internal value for new 
additions.  I guess I'm not that attached to it, so if you think such 
consistency matters (mind that we have different numeric values for 
"unlimited" across various settings anyway), then I can allow 0 here.

> > Index: src/gdb/command.h
> > ===================================================================
> > --- src.orig/gdb/command.h
> > +++ src/gdb/command.h
[...]
> > @@ -123,6 +126,22 @@ typedef enum var_types
> >    }
> >  var_types;
> >  
> > +/* A structure describing an extra literal accepted and shown in place
> > +   of a number.  */
> > +typedef struct literal_defs
> 
> You don't need the typedef trick as this is C++.

 We still have them in several places though.  I have now removed it from 
my change.

> > +  {
> > +    /* The literal to define, e.g. "unlimited".  */
> > +    const char *literal;
> > +    /* A number optionally accepted that stands for the literal.  */
> > +    LONGEST val;
> > +    /* The number to substitute internally for LITERAL or VAL;
> > +       the use of this number is not allowed (unless the same as VAL).  */
> > +    LONGEST use;
> > +    /* True if the use of VAL is allowed; otherwise only the literal is.  */
> > +    bool allow;
> 
> Could we replace VAL and ALLOW with something like:
> 
>   gdb::optional<LONGEST> ....

 Done now.

> 
> > +  }
> > +literal_def;
> > +
> >  /* Return true if a setting of type VAR_TYPE is backed with type T.
> >  
> >     This function is left without definition intentionally.  This template is
> > @@ -153,15 +172,14 @@ inline bool var_type_uses<enum auto_bool
> >  template<>
> >  inline bool var_type_uses<unsigned int> (var_types t)
> >  {
> > -  return (t == var_uinteger || t == var_zinteger || t == var_zuinteger);
> > +  return t == var_uinteger;
> >  }
> 
> Should we really remove all these uses of var_zinteger, etc?  The enum
> values are still around, right?  So surely if you passed in that enum
> field the true/false response shouldn't change...

 Not relevant anymore since I have removed `var_zinteger', etc. altogether 
as the attempt to leave them for Guile/Python was clearly hardly less if 
not more confusing than the original arrangement.

 Thank you for your review, I'll be posting v6 shortly.  While making this 
update I have discovered a few further issues, so we're back at 8 patches.

  Maciej