Re: [PATCH v8 4/6] GDB: Allow arbitrary keywords in integer set commands

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

On 11/24/22 06:22, Maciej W. Rozycki 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.
> 
> Remove 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".
> 
> Add a new structure, `literal_def', the entries of which define extra 
> keywords allowed for a command and numerical values they correspond to.  
> Those values are not verified against the basic range supported by the 
> underlying variable type, allowing extra values to be allowed outside 
> that range, which may or may not be individually made visible to the 
> user.  An optional value translation is possible with the structure to 
> follow the existing practice for some commands where user-entered 0 is 
> internally translated to UINT_MAX or INT_MAX.  Such translation can now 
> be arbitrary.  Literals defined by this structure are automatically used 
> for completion as necessary.
> 
> So for example:
> 
> const literal_def integer_unlimited_literals[] =
>   {
>     { "unlimited", INT_MAX, 0 },
>     { nullptr }
>   };
> 
> defines an extra `unlimited' keyword and a user-visible 0 value, both of 
> which get translated to INT_MAX for the setting to be used with.
> 
> Similarly:
> 
> const literal_def zuinteger_unlimited_literals[] =
>   {
>     { "unlimited", -1, -1 },
>     { nullptr }
>   };
> 
> defines the same keyword and a corresponding user-visible -1 value that 
> is used for the requested setting.  If the last member were omitted (or 
> set to `{}') here, then only the keyword would be allowed for the user 
> to enter and while -1 would still be used internally trying to enter it 
> as a part of a command would result in an "integer -1 out of range" 
> error.
> 
> Use said error message in all cases (citing the invalid value requested) 
> replacing "only -1 is allowed to set as unlimited" previously used for 
> `var_zuinteger_unlimited' settings only rather than propagating it to 
> `var_pinteger' type.  It could only be used for the specific case where 
> a single extra `unlimited' keyword was defined standing for -1 and the 
> use of numeric equivalents is discouraged anyway as it is for historical 
> reasons only that they expose GDB internals, confusingly different 
> across variable types.  Similarly update the "must be >= -1" Guile error 
> message.
> 
> Redefine Guile and Python parameter types in terms of the new variable 
> types and interpret extra keywords as Scheme keywords and Python strings 
> used to communicate corresponding parameter values.  Do not add a new
> PARAM_INTEGER Guile parameter type, however do handle the `var_integer' 
> variable type now, permitting existing parameters defined by GDB proper, 
> such as `listsize', to be accessed from Scheme code.
> 
> With these changes in place it should be trivial for a Scheme or Python 
> programmer to expand the syntax of the `make-parameter' command and the 
> `gdb.Parameter' class initializer to have arbitrary extra literals along 
> with their internal representation supplied.
> 
> Update the testsuite accordingly.

This is a nice cleanup I think.  I didn't want to delay this further, so
I did the best review I could given the time I had.  I'm fine with you
merging this (you can add my Approved-By), we can always follow up with
more fixes.  If others would like to review it, it's always appreciated,
but given the time it's been sitting unreviewed, I kind of doubt that
will happen.

I noted a few minor things:

> ---
> Changes from v7:
> 
> - Handle the special case of returning -1 for the value of `unlimited' 
>   with `var_pinteger' parameters in Python code.
> 
> - Update Python documentation accordingly, including `unlimited' case 
>   descriptions from what used to be in 1/4.
> 
> - Pass extra literals outside erased args with `add_set_or_show_cmd' and 
>   up the call chain.
> 
> No change from v6.
> 
> Changes from v5:
> 
> - Add a translation layer from Guile and Python parameter types to new
>   GDB variable types and remove `var_zuinteger', `var_uinteger', and 
>   `var_zuinteger_unlimited' variable types altogether now.
> 
> - Add an optional `extra_literals' initialiser to the `setting' class
>   constructor.
> 
> - Remove the "only -1 is allowed to set as unlimited" error message
>   altogether now rather than propagating it to `var_pinteger' type.
> 
> - Make the `val' member of `struct literal_def' optional and remove the
>   `allow' member; simplify processing accordingly.
> 
> - Rename `zuinteger_unlimited_literals' to `pinteger_unlimited_literals',
>   making the names of all `*_unlimited_literals' arrays consistent with 
>   the corresponding `var_*' variable types.
> 
> - Rename `struct integer_option_def' to `struct pinteger_option_def',
>   observing it's come from `struct zuinteger_unlimited_option_def' and 
>   what used to be the `var_zuinteger_unlimited' now has `var_pinteger' 
>   semantics.
> 
> - Update the names of test flags used by `maint test-options' accordingly.
> 
> - Add constructor variants to `struct uinteger_option_def' and `struct
>   pinteger_option_def' that allow one to skip the `extra_literals' 
>   initialiser altogether rather than having to pass in `nullptr'.
> 
> - Update Python documentation mentioning the use of literal `unlimited'
>   with the respective parameter types.
> 
> No change from v4.
> 
> New change in v4.
> ---
>  gdb/cli/cli-cmds.c                        |   59 ++---
>  gdb/cli/cli-decode.c                      |  321 +++++++++++++++++++++++-------
>  gdb/cli/cli-option.c                      |  117 +++++++---
>  gdb/cli/cli-option.h                      |   54 +++--
>  gdb/cli/cli-setshow.c                     |  245 ++++++++++------------
>  gdb/cli/cli-setshow.h                     |   20 -
>  gdb/command.h                             |  110 +++++++---
>  gdb/doc/python.texi                       |   28 +-
>  gdb/guile/scm-param.c                     |  319 +++++++++++++++++++----------
>  gdb/maint-test-options.c                  |   44 ++--
>  gdb/python/py-param.c                     |  286 ++++++++++++++++----------
>  gdb/python/python.c                       |   52 +++-
>  gdb/testsuite/gdb.base/max-value-size.exp |    2 
>  gdb/testsuite/gdb.base/options.exp        |   47 ++--
>  gdb/testsuite/gdb.base/settings.exp       |    2 
>  gdb/testsuite/gdb.base/with.exp           |    2 
>  gdb/testsuite/gdb.guile/scm-parameter.exp |   23 --
>  gdb/testsuite/gdb.python/py-parameter.exp |   15 +
>  gdb/valprint.c                            |    9 
>  19 files changed, 1133 insertions(+), 622 deletions(-)
> 
> gdb-setshow-cmd-extra-literals.diff
> Index: src/gdb/cli/cli-cmds.c
> ===================================================================
> --- src.orig/gdb/cli/cli-cmds.c
> +++ src/gdb/cli/cli-cmds.c
> @@ -2213,22 +2213,40 @@ value_from_setting (const setting &var,
>  {
>    switch (var.type ())
>      {
> +    case var_uinteger:
>      case var_integer:
> -      if (var.get<int> () == INT_MAX)
> -	return value_from_longest (builtin_type (gdbarch)->builtin_int,
> -				   0);
> -      else
> -	return value_from_longest (builtin_type (gdbarch)->builtin_int,
> -				   var.get<int> ());
> -    case var_zinteger:
> -      return value_from_longest (builtin_type (gdbarch)->builtin_int,
> -				 var.get<int> ());
> +    case var_pinteger:
> +      {
> +	LONGEST value
> +	  = (var.type () == var_uinteger
> +	     ? static_cast<LONGEST> (var.get<unsigned int> ())
> +	     : static_cast<LONGEST> (var.get<int> ()));
> +
> +	if (var.extra_literals () != nullptr)
> +	  for (const literal_def *l = var.extra_literals ();
> +	       l->literal != nullptr;
> +	       l++)
> +	    if (value == l->use)
> +	      {
> +		if (l->val.has_value ())
> +		  value = *l->val;
> +		else
> +		  return allocate_value (builtin_type (gdbarch)->builtin_void);

I was wondering about this line returning void.  As far as I understand,
this isn't used today.  It would only be used if we defined a
literal_def without a user-visible value.  And the consequence would be
that for this setting, using $_gdb_setting("foo") would yield void for
that setting value.

> +		break;
> +	      }
> +
> +	if (var.type () == var_uinteger)
> +	  return
> +	    value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int,
> +				 static_cast<const unsigned int> (value));
> +	else
> +	  return
> +	    value_from_longest (builtin_type (gdbarch)->builtin_int,
> +				static_cast<const int> (value));

Unnecessary consts in casts.

> Index: src/gdb/cli/cli-option.c
> ===================================================================
> --- src.orig/gdb/cli/cli-option.c
> +++ src/gdb/cli/cli-option.c
> @@ -38,7 +38,7 @@ union option_value
>    /* For var_uinteger options.  */
>    unsigned int uinteger;
>  
> -  /* For var_zuinteger_unlimited options.  */
> +  /* For var_integer and var_pinteger options.  */
>    int integer;
>  
>    /* For var_enum options.  */
> @@ -356,42 +356,55 @@ parse_option (gdb::array_view<const opti
>  	return option_def_and_value {*match, match_ctx, val};
>        }
>      case var_uinteger:
> -    case var_zuinteger_unlimited:
> +    case var_integer:
> +    case var_pinteger:
>        {
>  	if (completion != nullptr)
>  	  {
> -	    if (**args == '\0')
> -	      {
> -		/* Convenience to let the user know what the option
> -		   can accept.  Note there's no common prefix between
> -		   the strings on purpose, so that readline doesn't do
> -		   a partial match.  */
> -		completion->tracker.add_completion
> -		  (make_unique_xstrdup ("NUMBER"));
> -		completion->tracker.add_completion
> -		  (make_unique_xstrdup ("unlimited"));
> -		return {};
> -	      }
> -	    else if (startswith ("unlimited", *args))
> +	    if (match->extra_literals != nullptr)

The two ifs could be merged, it would help reduce the indentation a bit.

>  	      {
> -		completion->tracker.add_completion
> -		  (make_unique_xstrdup ("unlimited"));
> -		return {};
> +		/* Convenience to let the user know what the option can
> +		   accept.  Make sure there's no common prefix between
> +		   "NUMBER" and all the strings when adding new ones,
> +		   so that readline doesn't do a partial match.  */
> +		if (**args == '\0')
> +		  {
> +		    completion->tracker.add_completion
> +		      (make_unique_xstrdup ("NUMBER"));
> +		    for (const literal_def *l = match->extra_literals;
> +			 l->literal != nullptr;
> +			 l++)
> +		      completion->tracker.add_completion
> +			(make_unique_xstrdup (l->literal));
> +		    return {};
> +		  }
> +		else
> +		  {
> +		    bool completions = false;
> +		    for (const literal_def *l = match->extra_literals;
> +			 l->literal != nullptr;
> +			 l++)
> +		      if (startswith (l->literal, *args))
> +			{
> +			  completion->tracker.add_completion
> +			    (make_unique_xstrdup (l->literal));
> +			  completions = true;
> +			}
> +		    if (completions)
> +		      return {};
> +		  }
>  	      }
>  	  }
>  
> -	if (match->type == var_zuinteger_unlimited)
> -	  {
> -	    option_value val;
> -	    val.integer = parse_cli_var_zuinteger_unlimited (args, false);
> -	    return option_def_and_value {*match, match_ctx, val};
> -	  }
> +	LONGEST v = parse_cli_var_integer (match->type,
> +					   match->extra_literals,
> +					   args, false);
> +	option_value val;
> +	if (match->type == var_uinteger)
> +	  val.uinteger = v;
>  	else
> -	  {
> -	    option_value val;
> -	    val.uinteger = parse_cli_var_uinteger (match->type, args, false);
> -	    return option_def_and_value {*match, match_ctx, val};
> -	  }
> +	  val.integer = v;
> +	return option_def_and_value {*match, match_ctx, val};
>        }
>      case var_enum:
>        {
> @@ -593,7 +606,8 @@ save_option_value_in_ctx (gdb::optional<
>        *ov->option.var_address.uinteger (ov->option, ov->ctx)
>  	= ov->value->uinteger;
>        break;
> -    case var_zuinteger_unlimited:
> +    case var_integer:
> +    case var_pinteger:
>        *ov->option.var_address.integer (ov->option, ov->ctx)
>  	= ov->value->integer;
>        break;
> @@ -664,8 +678,20 @@ get_val_type_str (const option_def &opt,
>      case var_boolean:
>        return "[on|off]";
>      case var_uinteger:
> -    case var_zuinteger_unlimited:
> -      return "NUMBER|unlimited";
> +    case var_integer:
> +    case var_pinteger:
> +      {
> +	buffer = "NUMBER";
> +	if (opt.extra_literals != nullptr)
> +	  for (const literal_def *l = opt.extra_literals;
> +	       l->literal != nullptr;
> +	       l++)
> +	    {
> +	      buffer += '|';
> +	      buffer += l->literal;
> +	    }
> +	return buffer.c_str ();
> +      }
>      case var_enum:
>        {
>  	buffer = "";
> @@ -789,20 +815,31 @@ add_setshow_cmds_for_options (command_cl
>  	{
>  	  add_setshow_uinteger_cmd (option.name, cmd_class,
>  				    option.var_address.uinteger (option, data),
> +				    option.extra_literals,
>  				    option.set_doc, option.show_doc,
>  				    option.help_doc,
>  				    nullptr, option.show_cmd_cb,
>  				    set_list, show_list);
>  	}
> -      else if (option.type == var_zuinteger_unlimited)
> +      else if (option.type == var_integer)
>  	{
> -	  add_setshow_zuinteger_unlimited_cmd
> -	    (option.name, cmd_class,
> -	     option.var_address.integer (option, data),
> -	     option.set_doc, option.show_doc,
> -	     option.help_doc,
> -	     nullptr, option.show_cmd_cb,
> -	     set_list, show_list);
> +	  add_setshow_integer_cmd (option.name, cmd_class,
> +				   option.var_address.integer (option, data),
> +				   option.extra_literals,
> +				   option.set_doc, option.show_doc,
> +				   option.help_doc,
> +				   nullptr, option.show_cmd_cb,
> +				   set_list, show_list);
> +	}
> +      else if (option.type == var_pinteger)
> +	{
> +	  add_setshow_pinteger_cmd (option.name, cmd_class,
> +				    option.var_address.integer (option, data),
> +				    option.extra_literals,
> +				    option.set_doc, option.show_doc,
> +				    option.help_doc,
> +				    nullptr, option.show_cmd_cb,
> +				    set_list, show_list);
>  	}
>        else if (option.type == var_enum)
>  	{
> Index: src/gdb/cli/cli-option.h
> ===================================================================
> --- src.orig/gdb/cli/cli-option.h
> +++ src/gdb/cli/cli-option.h
> @@ -49,12 +49,13 @@ struct option_def
>       used to create the option's "set/show" commands.  */
>    constexpr option_def (const char *name_,
>  			var_types var_type_,
> +			const literal_def *extra_literals_,
>  			erased_get_var_address_ftype *erased_get_var_address_,
>  			show_value_ftype *show_cmd_cb_,
>  			const char *set_doc_,
>  			const char *show_doc_,
>  			const char *help_doc_)
> -    : name (name_), type (var_type_),
> +    : name (name_), type (var_type_), extra_literals (extra_literals_),
>        erased_get_var_address (erased_get_var_address_),
>        var_address {},
>        show_cmd_cb (show_cmd_cb_),
> @@ -68,6 +69,9 @@ struct option_def
>    /* The option's type.  */
>    var_types type;
>  
> +  /* Extra literals, such as `unlimited', accepted in lieu of a number.  */
> +  const literal_def *extra_literals;
> +
>    /* A function that gets the controlling variable's address, type
>       erased.  */
>    erased_get_var_address_ftype *erased_get_var_address;
> @@ -160,7 +164,7 @@ struct boolean_option_def : option_def
>  		      const char *set_doc_,
>  		      const char *show_doc_ = nullptr,
>  		      const char *help_doc_ = nullptr)
> -    : option_def (long_option_, var_boolean,
> +    : option_def (long_option_, var_boolean, nullptr,
>  		  (erased_get_var_address_ftype *) get_var_address_cb_,
>  		  show_cmd_cb_,
>  		  set_doc_, show_doc_, help_doc_)
> @@ -207,37 +211,59 @@ 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_)
>    {
>      var_address.uinteger = detail::get_var_address<unsigned int, Context>;
>    }
> +
> +  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_, nullptr,
> +			   show_cmd_cb_, set_doc_, show_doc_, help_doc_)
> +  { /* Nothing.  */ }
>  };
>  
> -/* A var_zuinteger_unlimited command line option.  */
> +/* A var_pinteger command line option.  */
>  
>  template<typename Context>
> -struct zuinteger_unlimited_option_def : option_def
> +struct pinteger_option_def : option_def
>  {
> -  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)
> -    : option_def (long_option_, var_zuinteger_unlimited,
> +  pinteger_option_def (const char *long_option_,
> +		       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_pinteger, extra_literals_,
>  		  (erased_get_var_address_ftype *) get_var_address_cb_,
>  		  show_cmd_cb_,
>  		  set_doc_, show_doc_, help_doc_)
>    {
>      var_address.integer = detail::get_var_address<int, Context>;
>    }
> +
> +  pinteger_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)
> +    : pinteger_option_def (long_option_, get_var_address_cb_, nullptr,
> +			   show_cmd_cb_, set_doc_, show_doc_, help_doc_)
> +  { /* Nothing.  */ }
>  };
>  
>  /* An var_enum command line option.  */
> @@ -252,7 +278,7 @@ struct enum_option_def : option_def
>  		   const char *set_doc_,
>  		   const char *show_doc_ = nullptr,
>  		   const char *help_doc_ = nullptr)
> -    : option_def (long_option_, var_enum,
> +    : option_def (long_option_, var_enum, nullptr,
>  		  (erased_get_var_address_ftype *) get_var_address_cb_,
>  		  show_cmd_cb_,
>  		  set_doc_, show_doc_, help_doc_)
> @@ -273,7 +299,7 @@ struct string_option_def : option_def
>  		     const char *set_doc_,
>  		     const char *show_doc_ = nullptr,
>  		     const char *help_doc_ = nullptr)
> -    : option_def (long_option_, var_string,
> +    : option_def (long_option_, var_string, nullptr,
>  		  (erased_get_var_address_ftype *) get_var_address_cb_,
>  		  show_cmd_cb_,
>  		  set_doc_, show_doc_, help_doc_)
> Index: src/gdb/cli/cli-setshow.c
> ===================================================================
> --- src.orig/gdb/cli/cli-setshow.c
> +++ src/gdb/cli/cli-setshow.c
> @@ -149,10 +149,11 @@ deprecated_show_value_hack (struct ui_fi
>      }
>  }
>  
> -/* Returns true if ARG is "unlimited".  */
> +/* Returns true and the value in VAL if ARG is an accepted literal.  */
>  
>  static bool
> -is_unlimited_literal (const char **arg, bool expression)
> +get_literal_val (LONGEST &val, const literal_def *extra_literals,
> +		 const char **arg, bool expression)
>  {
>    *arg = skip_spaces (*arg);
>  
> @@ -162,85 +163,104 @@ is_unlimited_literal (const char **arg,
>  
>    size_t len = p - *arg;
>  
> -  if (len > 0 && strncmp ("unlimited", *arg, len) == 0)
> -    {
> -      *arg += len;
> -
> -      /* If parsing an expression (i.e., parsing for a "set" command),
> -	 anything after "unlimited" is junk.  For options, anything
> -	 after "unlimited" might be a command argument or another
> -	 option.  */
> -      if (expression)
> +  if (len > 0 && extra_literals != nullptr)
> +    for (const literal_def *l = extra_literals;
> +	 l->literal != nullptr;
> +	 l++)
> +      if (strncmp (l->literal, *arg, len) == 0)
>  	{
> -	  const char *after = skip_spaces (*arg);
> -	  if (*after != '\0')
> -	    error (_("Junk after \"%.*s\": %s"),
> -		   (int) len, unl_start, after);
> -	}
> +	  *arg += len;
>  
> -      return true;
> -    }
> +	  /* If parsing an expression (i.e., parsing for a "set" command),
> +	     anything after the literal is junk.  For options, anything
> +	     after the literal might be a command argument or another
> +	     option.  */
> +	  if (expression)
> +	    {
> +	      const char *after = skip_spaces (*arg);
> +	      if (*after != '\0')
> +		error (_("Junk after \"%.*s\": %s"),
> +		       (int) len, unl_start, after);
> +	    }
> +
> +	  val = l->use;
> +	  return true;
> +	}
>  
>    return false;
>  }
>  
>  /* 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)

The function here could perhaps  take a `const setting &`, instead of var_type
and extra_literals, which are properties of a setting.

> @@ -623,36 +614,32 @@ get_setshow_command_value_string (const
>  	}
>        break;
>      case var_uinteger:
> -    case var_zuinteger:
> -      {
> -	const unsigned int value = var.get<unsigned int> ();
> -
> -	if (var.type () == var_uinteger
> -	    && value == UINT_MAX)
> -	  stb.puts ("unlimited");
> -	else
> -	  stb.printf ("%u", value);
> -      }
> -      break;
>      case var_integer:
> -    case var_zinteger:
> +    case var_pinteger:
>        {
> -	const int value = var.get<int> ();
> +	bool printed = false;
> +	const LONGEST value
> +	  = (var.type () == var_uinteger
> +	     ? static_cast<LONGEST> (var.get<unsigned int> ())
> +	     : static_cast<LONGEST> (var.get<int> ()));
>  
> -	if (var.type () == var_integer
> -	    && value == INT_MAX)
> -	  stb.puts ("unlimited");
> -	else
> -	  stb.printf ("%d", value);
> -      }
> -      break;
> -    case var_zuinteger_unlimited:
> -      {
> -	const int value = var.get<int> ();
> -	if (value == -1)
> -	  stb.puts ("unlimited");
> -	else
> -	  stb.printf ("%d", value);
> +	if (var.extra_literals () != nullptr)
> +	  for (const literal_def *l = var.extra_literals ();
> +	       l->literal != nullptr;
> +	       l++)
> +	    if (value == l->use)
> +	      {
> +		stb.puts (l->literal);
> +		printed = true;
> +		break;
> +	      }
> +	if (!printed)
> +	  {
> +	    if (var.type () == var_uinteger)
> +	      stb.printf ("%u", static_cast<const unsigned int> (value));
> +	    else
> +	      stb.printf ("%d", static_cast<const int> (value));

The consts are unnecessary here.

> @@ -106,22 +108,25 @@ enum var_types
>      var_optional_filename,
>      /* String which stores a filename.  (*VAR) is a std::string.  */
>      var_filename,
> -    /* ZeroableInteger.  *VAR is an int.  Like var_integer except
> -       that zero really means zero.  */
> -    var_zinteger,
> -    /* ZeroableUnsignedInteger.  *VAR is an unsigned int.  Zero really
> -       means zero.  */
> -    var_zuinteger,
> -    /* ZeroableUnsignedInteger with unlimited value.  *VAR is an int,
> -       but its range is [0, INT_MAX].  -1 stands for unlimited and
> -       other negative numbers are not allowed.  */
> -    var_zuinteger_unlimited,
>      /* Enumerated type.  Can only have one of the specified values.
>         *VAR is a char pointer to the name of the element that we
>         find.  */
>      var_enum
>    };
>  
> +/* A structure describing an extra literal accepted and shown in place
> +   of a number.  */
> +struct literal_def
> +  {
> +    /* The literal to define, e.g. "unlimited".  */
> +    const char *literal;
> +    /* The number to substitute internally for LITERAL or VAL;
> +       the use of this number is not allowed (unless the same as VAL).  */
> +    LONGEST use;
> +    /* An optional number accepted that stands for the literal.  */
> +    gdb::optional<LONGEST> val;

Could you please add an empty line between each field?  I find that
makes it easier to read.

> +  };

The body of the struct should have one less indentation level (curly
braces at column 0).

> @@ -218,8 +222,8 @@ struct setting
>  
>       Type T must match the var type VAR_TYPE (see VAR_TYPE_USES).  */
>    template<typename T>
> -  setting (var_types var_type, T *var)
> -    : m_var_type (var_type), m_var (var)
> +  setting (var_types var_type, T *var, const void *extra_literals = nullptr)
> +    : m_var_type (var_type), m_var (var), m_extra_literals (extra_literals)

Any reason why extra_literals and m_extra_literals are void pointers,
rather than literal_def pointers?

> @@ -110,6 +116,50 @@ struct param_smob
>    SCM containing_scm;
>  };
>  
> +/* Guile parameter types as in PARAMETER_TYPES later on.  */
> +
> +typedef enum param_types
> +  {
> +    param_boolean,
> +    param_auto_boolean,
> +    param_zinteger,
> +    param_uinteger,
> +    param_zuinteger,
> +    param_zuinteger_unlimited,
> +    param_string,
> +    param_string_noescape,
> +    param_optional_filename,
> +    param_filename,
> +    param_enum,
> +  }
> +param_types;

Just "enum param_types, no need for the typedef.  Braces at column 0.

> +
> +/* Translation from Guile parameters to GDB variable types.  Keep in the
> +   same order as PARAM_TYPES due to C++'s lack of designated initializers.  */
> +
> +static const struct
> +  {
> +    /* The type of the parameter.  */
> +    enum var_types type;
> +
> +    /* Extra literals, such as `unlimited', accepted in lieu of a number.  */
> +    const literal_def *extra_literals;
> +  }

Braces at column 0 above.

> @@ -631,20 +672,29 @@ pascm_param_value (const setting &var, i
>  	  return auto_keyword;
>        }
>  
> -    case var_zuinteger_unlimited:
> -      if (var.get<int> () == -1)
> -	return unlimited_keyword;
> -      gdb_assert (var.get<int> () >= 0);
> -      /* Fall through.  */
> -    case var_zinteger:
> -      return scm_from_int (var.get<int> ());
> -
>      case var_uinteger:
> -      if (var.get<unsigned int> ()== UINT_MAX)
> -	return unlimited_keyword;
> -      /* Fall through.  */
> -    case var_zuinteger:
> -      return scm_from_uint (var.get<unsigned int> ());
> +    case var_integer:
> +    case var_pinteger:
> +      {
> +	LONGEST value
> +	  = (var.type () == var_uinteger
> +	     ? static_cast<LONGEST> (var.get<unsigned int> ())
> +	     : static_cast<LONGEST> (var.get<int> ()));
> +
> +	if (var.extra_literals () != nullptr)
> +	  for (const literal_def *l = var.extra_literals ();
> +	       l->literal != nullptr;
> +	       l++)
> +	    if (value == l->use)
> +	      return scm_from_latin1_keyword (l->literal);
> +	if (var.type () == var_pinteger)
> +	  gdb_assert (value >= 0);
> +
> +	if (var.type () == var_uinteger)
> +	  return scm_from_uint (static_cast<const unsigned int> (value));
> +	else
> +	  return scm_from_int (static_cast<const int> (value));

Unnecessary consts in casts.

> +      }
>  
>      default:
>        break;
> @@ -735,53 +785,91 @@ pascm_set_param_value_x (param_smob *p_s
>  	var.set<enum auto_boolean> (AUTO_BOOLEAN_FALSE);
>        break;
>  
> -    case var_zinteger:
> +    case var_integer:
>      case var_uinteger:
> -    case var_zuinteger:
> -    case var_zuinteger_unlimited:
> -      if (var.type () == var_uinteger
> -	  || var.type () == var_zuinteger_unlimited)
> -	{
> -	  SCM_ASSERT_TYPE (scm_is_integer (value)
> -			   || scm_is_eq (value, unlimited_keyword),
> -			   value, arg_pos, func_name,
> -			   _("integer or #:unlimited"));
> -	  if (scm_is_eq (value, unlimited_keyword))
> +    case var_pinteger:
> +      {
> +	const literal_def *extra_literals = p_smob->extra_literals;
> +	enum tribool allowed = TRIBOOL_UNKNOWN;
> +	enum var_types var_type = var.type ();
> +	bool integer = scm_is_integer (value);
> +	bool keyword = scm_is_keyword (value);
> +	std::string buffer = "";
> +	size_t count = 0;
> +	LONGEST val;
> +
> +	if (extra_literals != nullptr)
> +	  for (const literal_def *l = extra_literals;
> +	       l->literal != nullptr;
> +	       l++, count++)
>  	    {
> -	      if (var.type () == var_uinteger)
> -		var.set<unsigned int> (UINT_MAX);
> -	      else
> -		var.set<int> (-1);
> -	      break;
> +	      if (count != 0)
> +		buffer += ", ";
> +	      buffer = buffer + "#:" + l->literal;
> +	      if (keyword
> +		  && allowed == TRIBOOL_UNKNOWN
> +		  && scm_is_eq (value,
> +				scm_from_latin1_keyword (l->literal)))
> +		{
> +		  val = l->use;
> +		  allowed = TRIBOOL_TRUE;
> +		}
>  	    }
> -	}
> -      else
> -	{
> -	  SCM_ASSERT_TYPE (scm_is_integer (value), value, arg_pos, func_name,
> -			   _("integer"));
> -	}
>  
> -      if (var.type () == var_uinteger
> -	  || var.type () == var_zuinteger)
> -	{
> -	  unsigned int u = scm_to_uint (value);
> +	if (allowed == TRIBOOL_UNKNOWN)
> +	  {
> +	    if (extra_literals == nullptr)
> +	      SCM_ASSERT_TYPE (integer, value, arg_pos, func_name,
> +			       _("integer"));
> +	    else if (count > 1)
> +	      SCM_ASSERT_TYPE (integer, value, arg_pos, func_name,
> +			       string_printf (_("integer or one of: %s"),
> +					      buffer.c_str ()).c_str ());
> +	    else
> +	      SCM_ASSERT_TYPE (integer, value, arg_pos, func_name,
> +			       string_printf (_("integer or %s"),
> +					      buffer.c_str ()).c_str ());
>  
> -	  if (var.type () == var_uinteger && u == 0)
> -	    u = UINT_MAX;
> -	  var.set<unsigned int> (u);
> -	}
> -      else
> -	{
> -	  int i = scm_to_int (value);
> +	    val = (var_type == var_uinteger
> +		   ? static_cast<LONGEST> (scm_to_uint (value))
> +		   : static_cast<LONGEST> (scm_to_int (value)));
>  
> -	  if (var.type () == var_zuinteger_unlimited && i < -1)
> -	    {
> -	      gdbscm_out_of_range_error (func_name, arg_pos, value,
> -					 _("must be >= -1"));
> +	    if (extra_literals != nullptr)
> +	      for (const literal_def *l = extra_literals;
> +		   l->literal != nullptr;
> +		   l++)
> +		{
> +		  if (l->val.has_value () && val == *l->val)
> +		    {
> +		      allowed = TRIBOOL_TRUE;
> +		      val = l->use;
> +		      break;
> +		    }
> +		  else if (val == l->use)
> +		    allowed = TRIBOOL_FALSE;
> +		}
>  	    }
> -	  var.set<int> (i);
> -	}
> -      break;
> +
> +	if (allowed == TRIBOOL_UNKNOWN)
> +	  {
> +	    if (val > UINT_MAX || val < INT_MIN
> +		|| (var_type == var_uinteger && val < 0)
> +		|| (var_type == var_integer && val > INT_MAX)
> +		|| (var_type == var_pinteger && val < 0)
> +		|| (var_type == var_pinteger && val > INT_MAX))
> +	      allowed = TRIBOOL_FALSE;
> +	  }
> +	if (allowed == TRIBOOL_FALSE)
> +	  gdbscm_out_of_range_error (func_name, arg_pos, value,
> +				     _("integer out of range"));
> +
> +	if (var_type == var_uinteger)
> +	  var.set<unsigned int> (static_cast<const unsigned int> (val));
> +	else
> +	  var.set<int> (static_cast<const int> (val));

Unnecessary consts in casts.

> Index: src/gdb/python/py-param.c
> ===================================================================
> --- src.orig/gdb/python/py-param.c
> +++ src/gdb/python/py-param.c
> @@ -28,24 +28,70 @@
>  #include "language.h"
>  #include "arch-utils.h"
>  
> +/* Python parameter types as in PARM_CONSTANTS below.  */
> +
> +typedef enum param_types
> +  {
> +    param_boolean,
> +    param_auto_boolean,
> +    param_uinteger,
> +    param_integer,
> +    param_string,
> +    param_string_noescape,
> +    param_optional_filename,
> +    param_filename,
> +    param_zinteger,
> +    param_zuinteger,
> +    param_zuinteger_unlimited,
> +    param_enum,
> +  }
> +param_types;

Just "enum param_types, no need for the typedef.  Braces at column 0.

I think that having two `param_types` enums (here and in the Guile
support) technically violates the one definition rule.  Not sure if that
matters in practice.

> +
> +/* Translation from Python parameters to GDB variable types.  Keep in the
> +   same order as PARAM_TYPES due to C++'s lack of designated initializers.  */
> +
> +static const struct
> +  {
> +    /* The type of the parameter.  */
> +    enum var_types type;
> +
> +    /* Extra literals, such as `unlimited', accepted in lieu of a number.  */
> +    const literal_def *extra_literals;
> +  }

Braces at column 0.

> Index: src/gdb/python/python.c
> ===================================================================
> --- src.orig/gdb/python/python.c
> +++ src/gdb/python/python.c
> @@ -504,27 +504,45 @@ gdbpy_parameter_value (const setting &va
>  	  Py_RETURN_NONE;
>        }
>  
> -    case var_integer:
> -      if (var.get<int> () == INT_MAX)
> -	Py_RETURN_NONE;
> -      /* Fall through.  */
> -    case var_zinteger:
> -    case var_zuinteger_unlimited:
> -      return gdb_py_object_from_longest (var.get<int> ()).release ();
> -
>      case var_uinteger:
> +    case var_integer:
> +    case var_pinteger:
>        {
> -	unsigned int val = var.get<unsigned int> ();
> +	LONGEST value
> +	  = (var.type () == var_uinteger
> +	     ? static_cast<LONGEST> (var.get<unsigned int> ())
> +	     : static_cast<LONGEST> (var.get<int> ()));
>  
> -	if (val == UINT_MAX)
> -	  Py_RETURN_NONE;
> -	return gdb_py_object_from_ulongest (val).release ();
> -      }
> +	if (var.extra_literals () != nullptr)
> +	  for (const literal_def *l = var.extra_literals ();
> +	       l->literal != nullptr;
> +	       l++)
> +	    if (value == l->use)
> +	      {
> +		if (strcmp (l->literal, "unlimited") == 0)
> +		  {
> +		    /* Compatibility hack for API brokennes.  */

brokenness

> +		    if (var.type () == var_pinteger
> +			&& l->val.has_value ()
> +			&& *l->val == -1)
> +		      value = -1;
> +		    else
> +		      Py_RETURN_NONE;
> +		  }
> +		else if (l->val.has_value ())
> +		  value = *l->val;
> +		else
> +		  return host_string_to_python_string (l->literal).release ();
> +	      }
>  
> -    case var_zuinteger:
> -      {
> -	unsigned int val = var.get<unsigned int> ();
> -	return gdb_py_object_from_ulongest (val).release ();
> +	if (var.type () == var_uinteger)
> +	  return
> +	    gdb_py_object_from_ulongest
> +	      (static_cast<const unsigned int> (value)).release ();
> +	else
> +	  return
> +	    gdb_py_object_from_longest
> +	      (static_cast<const int> (value)).release ();

Unnecessary const in casts.

Simon