[RFA] Handle var_zuinteger and var_zuinteger_unlimited from Python

Message ID 20180426222003.9059-1-tom@tromey.com
State New
Headers show
Series
  • [RFA] Handle var_zuinteger and var_zuinteger_unlimited from Python
Related show

Commit Message

Tom Tromey April 26, 2018, 10:20 p.m.
PR python/20084 points out that the Python API doesn't handle the
var_zuinteger and var_zuinteger_unlimited parameter types.

This patch adds support for these types.

Regression tested on x86-64 Fedora 26.

gdb/ChangeLog
2018-04-26  Tom Tromey  <tom@tromey.com>

	PR python/20084:
	* python/python.c (gdbpy_parameter_value): Handle var_zuinteger
	and var_zuinteger_unlimited.
	* python/py-param.c (struct parm_constant): Add PARAM_ZUINTEGER
	and PARAM_ZUINTEGER_UNLIMITED.
	(set_parameter_value): Handle var_zuinteger and
	var_zuinteger_unlimited.
	(add_setshow_generic): Likewise.
	(parmpy_init): Likewise.

gdb/doc/ChangeLog
2018-04-26  Tom Tromey  <tom@tromey.com>

	PR python/20084:
	* python.texi (Parameters In Python): Document PARAM_ZUINTEGER and
	PARAM_ZUINTEGER_UNLIMITED.

gdb/testsuite/ChangeLog
2018-04-26  Tom Tromey  <tom@tromey.com>

	PR python/20084:
	* gdb.python/py-parameter.exp: Add PARAM_ZUINTEGER and
	PARAM_ZUINTEGER_UNLIMITED tests.
---
 gdb/ChangeLog                             | 12 +++++++
 gdb/doc/ChangeLog                         |  6 ++++
 gdb/doc/python.texi                       | 14 ++++++++
 gdb/python/py-param.c                     | 54 +++++++++++++++++++++++++------
 gdb/python/python.c                       |  7 ++++
 gdb/testsuite/ChangeLog                   |  6 ++++
 gdb/testsuite/gdb.python/py-parameter.exp | 21 ++++++++++++
 7 files changed, 111 insertions(+), 9 deletions(-)

-- 
2.13.6

Comments

Eli Zaretskii April 27, 2018, 6:35 a.m. | #1
> From: Tom Tromey <tom@tromey.com>

> Cc: Tom Tromey <tom@tromey.com>

> Date: Thu, 26 Apr 2018 16:20:03 -0600

> 

> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi

> index ebd48fffe7..ca9114864b 100644

> --- a/gdb/doc/python.texi

> +++ b/gdb/doc/python.texi

> @@ -3800,6 +3800,20 @@ The value is a filename.  This is just like

>  The value is an integer.  This is like @code{PARAM_INTEGER}, except 0

>  is interpreted as itself.

>  

> +@findex PARAM_ZUINTEGER

> +@findex gdb.PARAM_ZUINTEGER

> +@item gdb.PARAM_ZUINTEGER

> +The value is an unsigned integer.  This is like @code{PARAM_INTEGER},

> +except 0 is interpreted as itself, and the value cannot be negative.

> +

> +@findex PARAM_ZUINTEGER_UNLIMITED

> +@findex gdb.PARAM_ZUINTEGER_UNLIMITED

> +@item gdb.PARAM_ZUINTEGER_UNLIMITED

> +The value is an unsigned integer.  This is like @code{PARAM_ZUINTEGER},

> +except 0 is interpreted as itself, and the special value -1 should be

> +interpreted to mean ``unlimited''.  Other negative values are not

> +allowed.

> +


This part is approved, but it sounds like only 1 of the 3 differences
from PARAM_ZUINTEGER should actually be mentioned, as the other 2 --
that 0 is interpreted as itself and negative values are generally not
allowed -- are already present in PARAM_ZUINTEGER.  Or did I miss
something?

Thanks.
Joel Brobecker April 30, 2018, 9:52 p.m. | #2
Hi Tom,

> PR python/20084 points out that the Python API doesn't handle the

> var_zuinteger and var_zuinteger_unlimited parameter types.

> 

> This patch adds support for these types.

> 

> Regression tested on x86-64 Fedora 26.

> 

> gdb/ChangeLog

> 2018-04-26  Tom Tromey  <tom@tromey.com>

> 

> 	PR python/20084:

> 	* python/python.c (gdbpy_parameter_value): Handle var_zuinteger

> 	and var_zuinteger_unlimited.

> 	* python/py-param.c (struct parm_constant): Add PARAM_ZUINTEGER

> 	and PARAM_ZUINTEGER_UNLIMITED.

> 	(set_parameter_value): Handle var_zuinteger and

> 	var_zuinteger_unlimited.

> 	(add_setshow_generic): Likewise.

> 	(parmpy_init): Likewise.

> 

> gdb/doc/ChangeLog

> 2018-04-26  Tom Tromey  <tom@tromey.com>

> 

> 	PR python/20084:

> 	* python.texi (Parameters In Python): Document PARAM_ZUINTEGER and

> 	PARAM_ZUINTEGER_UNLIMITED.

> 

> gdb/testsuite/ChangeLog

> 2018-04-26  Tom Tromey  <tom@tromey.com>

> 

> 	PR python/20084:

> 	* gdb.python/py-parameter.exp: Add PARAM_ZUINTEGER and

> 	PARAM_ZUINTEGER_UNLIMITED tests.


This patch looks good to me, and you can push as is. It was surprisingly
harder to read (I picked this patch as a way to spend my time constructively,
while I wait for a build to finish ;-)), in particular the part with
the fall through in the case statement, but that's probably me having
a slightly different type of brain... I managed to convince myself
that the patch looks correct to me.

One thought: How about testing the value of the setting after setting
its value to -1?

-- 
Joel
Tom Tromey May 2, 2018, 4:01 p.m. | #3
>> +@findex PARAM_ZUINTEGER_UNLIMITED

>> +@findex gdb.PARAM_ZUINTEGER_UNLIMITED

>> +@item gdb.PARAM_ZUINTEGER_UNLIMITED

>> +The value is an unsigned integer.  This is like @code{PARAM_ZUINTEGER},

>> +except 0 is interpreted as itself, and the special value -1 should be

>> +interpreted to mean ``unlimited''.  Other negative values are not

>> +allowed.

>> +


Eli> This part is approved, but it sounds like only 1 of the 3 differences
Eli> from PARAM_ZUINTEGER should actually be mentioned, as the other 2 --
Eli> that 0 is interpreted as itself and negative values are generally not
Eli> allowed -- are already present in PARAM_ZUINTEGER.  Or did I miss
Eli> something?

No, you didn't, that seems correct to me.
Also the value is actually signed.

I've changed it to:

    The value is an signed integer.  This is like @code{PARAM_ZUINTEGER},
    except the special value -1 should be interpreted to mean
    ``unlimited''.  Other negative values are not allowed.

Tom
Tom Tromey May 2, 2018, 4:05 p.m. | #4
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:


Joel> This patch looks good to me, and you can push as is. It was surprisingly
Joel> harder to read (I picked this patch as a way to spend my time constructively,
Joel> while I wait for a build to finish ;-)), in particular the part with
Joel> the fall through in the case statement, but that's probably me having
Joel> a slightly different type of brain... I managed to convince myself
Joel> that the patch looks correct to me.

Yeah, that bit is really unclear in the patch, but clearer (IMO) in the
code:

	  case var_uinteger:
	    if (l == 0)
	      l = UINT_MAX;
	    /* Fall through.  */
	  case var_zuinteger:
	    ok = (l >= 0 && l <= UINT_MAX);
	    break;

Joel> One thought: How about testing the value of the setting after setting
Joel> its value to -1?

I've added a test like so:

	gdb_test "python print(gdb.parameter('test-$kind'))" "-1" \
	    "check that PARAM_ZUINTEGER value is -1 after setting"

Tom
Tom Tromey May 2, 2018, 4:06 p.m. | #5
Eli> This part is approved, but it sounds like only 1 of the 3 differences
Eli> from PARAM_ZUINTEGER should actually be mentioned, as the other 2 --
Eli> that 0 is interpreted as itself and negative values are generally not
Eli> allowed -- are already present in PARAM_ZUINTEGER.  Or did I miss
Eli> something?

Tom> No, you didn't, that seems correct to me.
Tom> Also the value is actually signed.

Tom> I've changed it to:

Tom>     The value is an signed integer.  This is like @code{PARAM_ZUINTEGER},
Tom>     except the special value -1 should be interpreted to mean
Tom>     ``unlimited''.  Other negative values are not allowed.

So what I actually wrote and what you wrote that I agreed with are not
the same: I left in the text about other negative values.  I thought
this still made sense given that the value is actually signed, but let
me know which way you'd prefer.

thanks,
Tom
Joel Brobecker May 2, 2018, 4:24 p.m. | #6
> Yeah, that bit is really unclear in the patch, but clearer (IMO) in the

> code:

> 

> 	  case var_uinteger:

> 	    if (l == 0)

> 	      l = UINT_MAX;

> 	    /* Fall through.  */

> 	  case var_zuinteger:

> 	    ok = (l >= 0 && l <= UINT_MAX);

> 	    break;


That's also how I ended up reviewing the patch :).

> Joel> One thought: How about testing the value of the setting after setting

> Joel> its value to -1?

> 

> I've added a test like so:

> 

> 	gdb_test "python print(gdb.parameter('test-$kind'))" "-1" \

> 	    "check that PARAM_ZUINTEGER value is -1 after setting"


Looks good!

Thanks Tom,
-- 
Joel
Eli Zaretskii May 2, 2018, 4:26 p.m. | #7
> From: Tom Tromey <tom@tromey.com>

> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org

> Date: Wed, 02 May 2018 10:01:34 -0600

> 

>     The value is an signed integer.  This is like @code{PARAM_ZUINTEGER},

                   ^^^^^^^^^^^^^^^^^
"a signed integer"?

>     except the special value -1 should be interpreted to mean

>     ``unlimited''.  Other negative values are not allowed.


Thanks, sounds good.
Tom Tromey May 2, 2018, 4:31 p.m. | #8
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:


>> From: Tom Tromey <tom@tromey.com>

>> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org

>> Date: Wed, 02 May 2018 10:01:34 -0600

>> 

>> The value is an signed integer.  This is like @code{PARAM_ZUINTEGER},

Eli>                    ^^^^^^^^^^^^^^^^^
Eli> "a signed integer"?

Thanks, I've fixed this.

Tom
Pedro Alves May 2, 2018, 6:16 p.m. | #9
On 04/26/2018 11:20 PM, Tom Tromey wrote:
> +    if {$kind == "PARAM_ZUINTEGER"} {

> +	gdb_test "python test_param_$kind.value = -1" "RuntimeError: Range exceeded.*"

> +    } else {

> +	gdb_test_no_output "python test_param_$kind.value = -1" ""


Passing "" to gdb_test_no_output case suppresses the test
name/message in gdb.sum.  I suspect that was not intended.

Thanks,
Pedro Alves
Tom Tromey May 2, 2018, 9:07 p.m. | #10
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> On 04/26/2018 11:20 PM, Tom Tromey wrote:
>> +    if {$kind == "PARAM_ZUINTEGER"} {

>> +	gdb_test "python test_param_$kind.value = -1" "RuntimeError: Range exceeded.*"

>> +    } else {

>> +	gdb_test_no_output "python test_param_$kind.value = -1" ""


Pedro> Passing "" to gdb_test_no_output case suppresses the test
Pedro> name/message in gdb.sum.  I suspect that was not intended.

How's this?

Tom

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 5bbcb2f420..9addcbb9e9 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,9 @@
 2018-05-02  Tom Tromey  <tom@tromey.com>
 
+	* gdb.python/py-parameter.exp: Set test message.
+
+2018-05-02  Tom Tromey  <tom@tromey.com>
+
 	PR python/20084:
 	* gdb.python/py-parameter.exp: Add PARAM_ZUINTEGER and
 	PARAM_ZUINTEGER_UNLIMITED tests.
diff --git a/gdb/testsuite/gdb.python/py-parameter.exp b/gdb/testsuite/gdb.python/py-parameter.exp
index b9ff9f1ec0..1ea79b8946 100644
--- a/gdb/testsuite/gdb.python/py-parameter.exp
+++ b/gdb/testsuite/gdb.python/py-parameter.exp
@@ -197,7 +197,8 @@ foreach kind {PARAM_ZUINTEGER PARAM_ZUINTEGER_UNLIMITED} {
     if {$kind == "PARAM_ZUINTEGER"} {
 	gdb_test "python test_param_$kind.value = -1" "RuntimeError: Range exceeded.*"
     } else {
-	gdb_test_no_output "python test_param_$kind.value = -1" ""
+	gdb_test_no_output "python test_param_$kind.value = -1" \
+	    "check that PARAM_ZUINTEGER value can be set to -1"
 	gdb_test "python print(gdb.parameter('test-$kind'))" "-1" \
 	    "check that PARAM_ZUINTEGER value is -1 after setting"
     }
Pedro Alves May 2, 2018, 9:37 p.m. | #11
On 05/02/2018 10:07 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

> 

> Pedro> On 04/26/2018 11:20 PM, Tom Tromey wrote:

>>> +    if {$kind == "PARAM_ZUINTEGER"} {

>>> +	gdb_test "python test_param_$kind.value = -1" "RuntimeError: Range exceeded.*"

>>> +    } else {

>>> +	gdb_test_no_output "python test_param_$kind.value = -1" ""

> 

> Pedro> Passing "" to gdb_test_no_output case suppresses the test

> Pedro> name/message in gdb.sum.  I suspect that was not intended.

> 

> How's this?


OK.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index ebd48fffe7..ca9114864b 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -3800,6 +3800,20 @@  The value is a filename.  This is just like
 The value is an integer.  This is like @code{PARAM_INTEGER}, except 0
 is interpreted as itself.
 
+@findex PARAM_ZUINTEGER
+@findex gdb.PARAM_ZUINTEGER
+@item gdb.PARAM_ZUINTEGER
+The value is an unsigned integer.  This is like @code{PARAM_INTEGER},
+except 0 is interpreted as itself, and the value cannot be negative.
+
+@findex PARAM_ZUINTEGER_UNLIMITED
+@findex gdb.PARAM_ZUINTEGER_UNLIMITED
+@item gdb.PARAM_ZUINTEGER_UNLIMITED
+The value is an unsigned integer.  This is like @code{PARAM_ZUINTEGER},
+except 0 is interpreted as itself, and the special value -1 should be
+interpreted to mean ``unlimited''.  Other negative values are not
+allowed.
+
 @findex PARAM_ENUM
 @findex gdb.PARAM_ENUM
 @item gdb.PARAM_ENUM
diff --git a/gdb/python/py-param.c b/gdb/python/py-param.c
index 0f8d9b6b42..22b26b26da 100644
--- a/gdb/python/py-param.c
+++ b/gdb/python/py-param.c
@@ -47,6 +47,8 @@  struct parm_constant parm_constants[] =
   { "PARAM_OPTIONAL_FILENAME", var_optional_filename },
   { "PARAM_FILENAME", var_filename },
   { "PARAM_ZINTEGER", var_zinteger },
+  { "PARAM_ZUINTEGER", var_zuinteger },
+  { "PARAM_ZUINTEGER_UNLIMITED", var_zuinteger_unlimited },
   { "PARAM_ENUM", var_enum },
   { NULL, 0 }
 };
@@ -225,6 +227,8 @@  set_parameter_value (parmpy_object *self, PyObject *value)
     case var_integer:
     case var_zinteger:
     case var_uinteger:
+    case var_zuinteger:
+    case var_zuinteger_unlimited:
       {
 	long l;
 	int ok;
@@ -239,20 +243,33 @@  set_parameter_value (parmpy_object *self, PyObject *value)
 	if (! gdb_py_int_as_long (value, &l))
 	  return -1;
 
-	if (self->type == var_uinteger)
+	switch (self->type)
 	  {
-	    ok = (l >= 0 && l <= UINT_MAX);
+	  case var_uinteger:
 	    if (l == 0)
 	      l = UINT_MAX;
-	  }
-	else if (self->type == var_integer)
-	  {
+	    /* Fall through.  */
+	  case var_zuinteger:
+	    ok = (l >= 0 && l <= UINT_MAX);
+	    break;
+
+	  case var_zuinteger_unlimited:
+	    ok = (l >= -1 && l <= INT_MAX);
+	    break;
+
+	  case var_integer:
 	    ok = (l >= INT_MIN && l <= INT_MAX);
 	    if (l == 0)
 	      l = INT_MAX;
+	    break;
+
+	  case var_zinteger:
+	    ok = (l >= INT_MIN && l <= INT_MAX);
+	    break;
+
+	  default:
+	    gdb_assert_not_reached ("unknown var_ constant");
 	  }
-	else
-	  ok = (l >= INT_MIN && l <= INT_MAX);
 
 	if (! ok)
 	  {
@@ -261,7 +278,10 @@  set_parameter_value (parmpy_object *self, PyObject *value)
 	    return -1;
 	  }
 
-	self->value.intval = (int) l;
+	if (self->type == var_uinteger || self->type == var_zuinteger)
+	  self->value.uintval = (unsigned) l;
+	else
+	  self->value.intval = (int) l;
 	break;
       }
 
@@ -526,6 +546,21 @@  add_setshow_generic (int parmclass, enum command_class cmdclass,
 				set_list, show_list);
       break;
 
+    case var_zuinteger:
+      add_setshow_zuinteger_cmd (cmd_name, cmdclass,
+				&self->value.uintval, set_doc, show_doc,
+				help_doc, get_set_value, get_show_value,
+				set_list, show_list);
+      break;
+
+    case var_zuinteger_unlimited:
+      add_setshow_zuinteger_unlimited_cmd (cmd_name, cmdclass,
+					   &self->value.intval, set_doc,
+					   show_doc, help_doc, get_set_value,
+					   get_show_value,
+					   set_list, show_list);
+      break;
+
     case var_enum:
       add_setshow_enum_cmd (cmd_name, cmdclass, self->enumeration,
 			    &self->value.cstringval, set_doc, show_doc,
@@ -658,7 +693,8 @@  parmpy_init (PyObject *self, PyObject *args, PyObject *kwds)
       && parmclass != var_uinteger && parmclass != var_integer
       && parmclass != var_string && parmclass != var_string_noescape
       && parmclass != var_optional_filename && parmclass != var_filename
-      && parmclass != var_zinteger && parmclass != var_enum)
+      && parmclass != var_zinteger && parmclass != var_zuinteger
+      && parmclass != var_zuinteger_unlimited && parmclass != var_enum)
     {
       PyErr_SetString (PyExc_RuntimeError,
 		       _("Invalid parameter class argument."));
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 9eae8a1aef..5aa78cc25a 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -467,6 +467,7 @@  gdbpy_parameter_value (enum var_types type, void *var)
 	Py_RETURN_NONE;
       /* Fall through.  */
     case var_zinteger:
+    case var_zuinteger_unlimited:
       return PyLong_FromLong (* (int *) var);
 
     case var_uinteger:
@@ -477,6 +478,12 @@  gdbpy_parameter_value (enum var_types type, void *var)
 	  Py_RETURN_NONE;
 	return PyLong_FromUnsignedLong (val);
       }
+
+    case var_zuinteger:
+      {
+	unsigned int val = * (unsigned int *) var;
+	return PyLong_FromUnsignedLong (val);
+      }
     }
 
   return PyErr_Format (PyExc_RuntimeError,
diff --git a/gdb/testsuite/gdb.python/py-parameter.exp b/gdb/testsuite/gdb.python/py-parameter.exp
index 3cd1198462..111dbedcbc 100644
--- a/gdb/testsuite/gdb.python/py-parameter.exp
+++ b/gdb/testsuite/gdb.python/py-parameter.exp
@@ -179,3 +179,24 @@  gdb_test "python print (test_param.value)" "False" "test parameter value"
 gdb_test "help show print test-param" "State of the Test Parameter.*" "test show help"
 gdb_test "help set print test-param" "Set the state of the Test Parameter.*" "test set help"
 gdb_test "help set print" "set print test-param -- Set the state of the Test Parameter.*" "test general help"
+
+foreach kind {PARAM_ZUINTEGER PARAM_ZUINTEGER_UNLIMITED} {
+    gdb_py_test_multiple "Simple gdb $kind" \
+	"python" "" \
+	"class TestNodocParam (gdb.Parameter):" "" \
+	"   def __init__ (self, name):" "" \
+	"      super (TestNodocParam, self).__init__ (name, gdb.COMMAND_DATA, gdb.$kind)" "" \
+	"      self.value = 0" "" \
+	"test_param_$kind = TestNodocParam ('test-$kind')" "" \
+	"end"
+
+    gdb_test "python print(gdb.parameter('test-$kind'))" "0"
+
+    gdb_test "python test_param_$kind.value = -5" "RuntimeError: Range exceeded.*"
+
+    if {$kind == "PARAM_ZUINTEGER"} {
+	gdb_test "python test_param_$kind.value = -1" "RuntimeError: Range exceeded.*"
+    } else {
+	gdb_test_no_output "python test_param_$kind.value = -1" ""
+    }
+}