Re: [RFAv3 0/3] Convenience functions $_gdb_setting/$_gdb_int_setting

[Pedro Alves <palves@redhat.com>]

On 7/8/19 11:33 PM, Philippe Waroquiers wrote:
> On Mon, 2019-07-08 at 17:34 +0100, Pedro Alves wrote:
>> Hi Philippe,
>>
>> Some overall design comments below.
>>
>> On 7/6/19 11:49 AM, Philippe Waroquiers wrote:
>>> As part of the discussion of 'show | set may-call-functions [on|off]',
>>> Eli suggested to have a way to access the GDB settings from
>>> user defined commands.
>>>
>>
>> As I had mentioned back then:
>>
>>  https://sourceware.org/ml/gdb-patches/2019-04/msg00562.html
>>
>> we can already access the settings via Python.  E.g. see it
>> done here from a gdbinit script:
>>
>>  https://gcc.gnu.org/ml/gcc-patches/2019-01/msg00135.html
>>
>> Copied here for convenience:
>>
>>  python __gcc_prev_pagination=gdb.parameter("pagination")
>>  set pagination off
>>  ...
>>  python if __gcc_prev_pagination: gdb.execute("set pagination on")
>>
>> Given that, and the existence of the "with" command now for the
>> common case, I'd like to at least see some mention/rationale/argument
>> in the commit logs about why this is worth having/maintaining
>> over just using gdb.parameter.
> 
> Here is some background about the rational:
> 
> The patch was developed following a comment of Eli about the
> 'set may-call-functions'. Eli said that user-defined functions
> should have a way to change their behavior according to this setting.
> Rather than have a specialized $_may_call_functions, this patch
> implements a general way to access any GDB setting.
> 
> Compared to the above access via Python:
> * The 'with' command is much better than the above python usage:
>   if the user types C-c or an error happens between the set pagination off
>   and the python "set pagination on", the above python 
>   does not restory the original setting.
> 
> * Effectively, with the above python one liner, it is possible to do
>   simple 'if' conditions, such as set and restore pagination.
>   But as far as I can see, mixing the "python if" within canned
>   sequence of commands is cumbersome for non trivial combinations.
>   E.g. if several commands have to be done for a certain condition
>   accessed from python, I guess something like will be needed:
>      python if __some_setting: gdb.execute("some command")
>      python if __some_setting: gdb.execute("some other command")
>      python if __some_setting: gdb.execute("some different command")
>   (without speaking about nested "if-s").
> 
>   With the convenience function:
>      if $_gdb_setting("some_setting")
>         some command
>         some other command
>         some different command
>      end
>   Integer settings (for example print elements) will also be more difficult
>   to use.
>   For example, a user defined function that scans and prints a linked list
>   might want to use the value of "set print elements" to stop printing
>   the linked list.
>   Doing that by mixing python expression/if is I guess doable, but seems
>   not easy with the above one liners.
> 
> So, in summary, the $_gdb_setting=$_gdb_int_setting avoids to have the
> heterogenous mix of python and GDB commands in one single script
> (and of course, it works even if python is not configured, but that
> must be an unusual setup I guess).

Alright, sounds good.  That's the sort of rationale that I'm looking
for.

> 
> 
>>
>> BTW, did you look into how gdb.parameter is implemented, see if
>> anything could be shared?
> I quickly looked at the implementation of python gdb.parameter,
> but could not find anything significant that can be shared.
> Note that $_gdb_setting is (now) re-using the function
> get_setshow_command_value_string that was developed for
> the "with" command.
> 
>>
>> BTW², kind of unfortunate that Python used a different naming
>> here (settings vs parameters).
>>
>>> So, this patch series implements this.
>>>
>>> 2 functions are provided:
>>>   * $_gdb_setting that gives access to all settings, giving back a string value.
>>>   * $_gdb_int_setting, giving access to boolean, auto-boolean and integers.
>>>     This is easier to use for such types than the string version.
>>
>> The naming of the functions kind of seems a bit reversed to me.  Off hand, I'd
>> expect $_gdb_setting to give me the setting in its native type, and then
>> something like $_gdb_setting_str to give me a string version/representation.
> Probably my Ada background that pushes me to have strongly typed functions :).
> 
> I think it is easy to have $_gdb_setting returning 'int' (for types that can
> be converted to integers) and strings for the others,
> and have $_gdb_setting_str always returning a string.
> 
>>
>> Also, it seems like a design issue that settings that are unsigned
>> internally get their values exposed as signed.
> Not sure I understand the comment.
> $_gdb_int_setting returns an integer value that is the same as what
> the user can type.  If the user can type -1 (as a synonym for unlimited),
> then $_gdb_int_setting returns -1, otherwise it will show 0 for unlimited.

For example:

> +    case var_uinteger:
> +      if (*(unsigned int *) cmd->var == UINT_MAX)
> +	return value_from_longest (builtin_type (gdbarch)->builtin_int,
> +				   0);
> +      else
> +	return value_from_longest (builtin_type (gdbarch)->builtin_int,
> +				   *(unsigned int *) cmd->var);

Why is this value_from_longest + builtin_int instead of
value_from_ulongest + builtin_unsigned_int ?

Also, looking at the values / mapping done for auto-boolean
settings:

+    case var_auto_boolean:
+      {
+	int val = *(int *) cmd->var;
+
+	if (val == 0)
+	  val = 1;
+	else if (val == 1)
+	  val = 0;
+	return value_from_longest (builtin_type (gdbarch)->builtin_int,
+				   val);
+      }

and:

+@item $_gdb_int_setting (@var{setting})
+@findex $_gdb_int_setting@r{, convenience function}
+Return the value of the @value{GDBN} @var{setting} as an integer.
+This only works for boolean, auto boolean and integer settings.
+The boolean values @code{off} and @code{on} are converted to
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+the integer values @code{0} and @code{1}.  The value @code{auto} is
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+converted to the value @code{2}.
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

and:

 enum auto_boolean
 {
   AUTO_BOOLEAN_TRUE,   /* 0, not 1! */
   AUTO_BOOLEAN_FALSE,  /* 1, not 0! */
   AUTO_BOOLEAN_AUTO    /* 2 */
 };

(comments mine)

and:
\f
static enum auto_boolean
parse_auto_binary_operation (const char *arg)
{
  if (arg != NULL && *arg != '\0')
    {
      int length = strlen (arg);

      while (isspace (arg[length - 1]) && length > 0)
	length--;

      /* Note that "o" is ambiguous.  */

      if ((length == 2 && strncmp (arg, "on", length) == 0)
	  || strncmp (arg, "1", length) == 0
	  || strncmp (arg, "yes", length) == 0
	  || strncmp (arg, "enable", length) == 0)
	return AUTO_BOOLEAN_TRUE;
      else if ((length >= 2 && strncmp (arg, "off", length) == 0)
	       || strncmp (arg, "0", length) == 0
	       || strncmp (arg, "no", length) == 0
	       || strncmp (arg, "disable", length) == 0)
	return AUTO_BOOLEAN_FALSE;
      else if (strncmp (arg, "auto", length) == 0
	       || (length > 1 && strncmp (arg, "-1", length) == 0))
	return AUTO_BOOLEAN_AUTO;
    }
  error (_("\"on\", \"off\" or \"auto\" expected."));
  return AUTO_BOOLEAN_AUTO; /* Pacify GCC.  */
}

we see quite a mismatch.  Particularly, note that the auto-boolean
set commands accept -1 for auto, not 2:

      else if (strncmp (arg, "auto", length) == 0
	       || (length > 1 && strncmp (arg, "-1", length) == 0))
	return AUTO_BOOLEAN_AUTO;

Vis:

 (gdb) set breakpoint pending 1
 (gdb) show breakpoint pending 
 Debugger's behavior regarding pending breakpoints is on.

 (gdb) set breakpoint pending -1
 (gdb) show breakpoint pending 
 Debugger's behavior regarding pending breakpoints is auto.

 (gdb) set breakpoint pending 2
 "on", "off" or "auto" expected.

I think that $_gdb_int_setting should use the same convention
as the "set" command.  I.e., conceptually, this should leave
the setting unmodified:

 (gdb) set breakpoint pending $_gdb_int_setting("breakpoint pending")

Should we normalize that throughout by changing auto_boolean to this:

 enum auto_boolean
 {
   AUTO_BOOLEAN_TRUE = 1
   AUTO_BOOLEAN_FALSE = 0,
   AUTO_BOOLEAN_AUTO = -1
 };

?

I wonder whether gdb.parameter's handling of auto-booleans is
also mismatched.

On the testing side, would it be possible to convert the testing
to use the "maint set/show test-settings" subcommands, and thus
be able to test all the different kinds of settings, in the
same spirit as gdb.base/settings.exp and gdb.base/with.exp?  

I think the only impediment is accessing the maint settings,
which you could do by adding a $_gdb_maint_setting function,
much like I had added "maint with" for testing the "with"
command's infrastructure.  It's bound to be useful anyway,
so that scripts can access the main settings.

> 
>>
>> I guess it could be even better if the setting's types were some new built-in
>> types specific for the settings, and then if you wanted to get a string
>> representation, you'd use '$_as_string($_gdb_setting(...))'.  (*)
> Maybe I miss the idea: not too clear what this new built-in
> 'setting type' would bring, as the user will have in any case to convert
> it to a 'basic' type  (e.g. int or string) to use it e.g. in an expression.
> So, that seems significantly more work without clear gain.

If $_gdb_setting(...) returned a "var_uinteger int" type, then
$_as_string would be able to return "unlimited" instead of "0".
Certainly something that could be added / experimented later on.
A $_gdb_setting_str function seems fine with me as well.

Thanks,
Pedro Alves