[PATCHv2] gdb/python: don't use the 'p' format for parsing args

[Simon Marchi]

On 2021-11-30 09:38, Andrew Burgess wrote:
> * Simon Marchi <simon.marchi@polymtl.ca> [2021-11-30 07:40:27 -0500]:
> 
>> On 2021-11-29 10:21, Andrew Burgess via Gdb-patches wrote:
>>> When running the gdb.python/py-arch.exp tests on a GDB built
>>> against Python 2 I ran into some errors.  The problem is that this
>>> test script exercises the gdb.Architecture.integer_type method, and
>>> this method uses 'p' as an argument format specifier in a call to
>>> gdb_PyArg_ParseTupleAndKeywords.
>>>
>>> Unfortunately this specified was only added in Python 3.3, so will
>>> cause an error for earlier versions of Python.
>>>
>>> This commit switches to using the 'i' specifier which should be good
>>> enough.
>>>
>>> There should be no user visible changes after this commit.
>>
>> I don't know if it's important, but there is user visible change.
>> Basically if the user passed any object other than an int, expecting it
>> to be converted to bool:
>>
>> Before:
>>
>>     >>> gdb.selected_inferior().architecture().integer_type(32, None)
>>     <gdb.Type object at 0x7f1ce94086c0>
>>     >>> gdb.selected_inferior().architecture().integer_type(32, "foo")
>>     <gdb.Type object at 0x7f1ce95f7750>
>>
>> After:
>>
>>     >>> gdb.selected_inferior().architecture().integer_type(32, None)
>>     Traceback (most recent call last):
>>       File "<stdin>", line 1, in <module>
>>     TypeError: an integer is required (got type NoneType)
>>     >>> gdb.selected_inferior().architecture().integer_type(32, "foo")
>>     Traceback (most recent call last):
>>       File "<stdin>", line 1, in <module>
>>     TypeError: an integer is required (got type str)
>>
>> I would suggest capturing the argument as an object (specifier 'O') and
>> using PyObject_IsTrue to convert it to bool.  That should work
>> everywhere.
> 
> Thanks for the feedback.  An updated version of the patch is included
> below.
> 
> Andrew
> 
> ---
> 
> commit 6bc532d238825893baf465cb0ac49fd6681224ff
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Mon Nov 29 13:53:06 2021 +0000
> 
>     gdb/python: don't use the 'p' format for parsing args
>     
>     When running the gdb.python/py-arch.exp tests on a GDB built
>     against Python 2 I ran into some errors.  The problem is that this
>     test script exercises the gdb.Architecture.integer_type method, and
>     this method uses 'p' as an argument format specifier in a call to
>     gdb_PyArg_ParseTupleAndKeywords.
>     
>     Unfortunately this specified was only added in Python 3.3, so will
>     cause an error for earlier versions of Python.
>     
>     This commit switches to use the 'O' specifier to collect a PyObject,
>     and then uses PyObject_IsTrue to convert the object to a boolean.
>     
>     An earlier version of this patch incorrectly switched from using 'p'
>     to use 'i', however, it was pointed out during review that this would
>     cause some changes in behaviour, for example both of these will work
>     with 'p', but not with 'i':
>     
>       gdb.selected_inferior().architecture().integer_type(32, None)
>       gdb.selected_inferior().architecture().integer_type(32, "foo")
>     
>     The new approach of using 'O' works fine with these cases.  I've added
>     some new tests to cover both of the above.
>     
>     There should be no user visible changes after this commit.
> 
> diff --git a/gdb/python/py-arch.c b/gdb/python/py-arch.c
> index aa9a652ef26..824ad0bfeab 100644
> --- a/gdb/python/py-arch.c
> +++ b/gdb/python/py-arch.c
> @@ -276,12 +276,17 @@ static PyObject *
>  archpy_integer_type (PyObject *self, PyObject *args, PyObject *kw)
>  {
>    static const char *keywords[] = { "size", "signed", NULL };
> -  int size, is_signed = 1;
> +  int size;
> +  PyObject *is_signed_obj = nullptr;
>  
> -  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "i|p", keywords,
> -					&size, &is_signed))
> +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "i|O", keywords,
> +					&size, &is_signed_obj))
>      return nullptr;
>  
> +  /* Assume signed by default.  */
> +  bool is_signed = (is_signed_obj == nullptr
> +		    || PyObject_IsTrue (is_signed_obj));
> +
>    struct gdbarch *gdbarch;
>    ARCHPY_REQUIRE_VALID (self, gdbarch);
>  
> diff --git a/gdb/testsuite/gdb.python/py-arch.exp b/gdb/testsuite/gdb.python/py-arch.exp
> index f3bf01d2828..14dc1bf85ee 100644
> --- a/gdb/testsuite/gdb.python/py-arch.exp
> +++ b/gdb/testsuite/gdb.python/py-arch.exp
> @@ -64,7 +64,7 @@ if { ![is_address_zero_readable] } {
>  }
>  
>  foreach size {0 1 2 3 4 8 16} {
> -    foreach sign {"" ", True" ", False"} {
> +    foreach sign {"" ", True" ", False" ", None" ", \"blah\""} {
>  	set fullsize [expr 8 * $size]
>  	gdb_test_no_output "python t = arch.integer_type($fullsize$sign)" \
>  	    "get integer type for $size$sign"
> 

LGTM, thanks.

Simon