[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gdb-patches
Subject:    Re: [PATCHv2] gdb/python: don't use the 'p' format for parsing args
From:       Andrew Burgess via Gdb-patches <gdb-patches () sourceware ! org>
Date:       2021-11-30 15:46:57
Message-ID: 20211130154657.GA2662946 () redhat ! com
[Download RAW message or body]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-11-30 10:02:57 -0500]:

> 
> 
> 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.

Thanks, pushed.

Andrew

[prev in list] [next in list] [prev in thread] [next in thread]