Re: [PATCH] Fix issue with dmesg.py and python 3.X

[Kieran Bingham]

On 29/04/16 20:00, Dom Cote wrote:
> Hi Kieran,

Hi Dom, sorry for the delay, we had a 3 day weekend over here :)

> 
> Thanks for pinging me on this. 
> 
> I had to rebuild a version of GDB with python 2.7 (mine system has 3.X).
> I had to dig into the configure.ac <http://configure.ac> script to find
> out what I needed to do to get the gdb build to pick up the python 2.7
> build. 
> But I got it to work now.

Excellent!


> Back to the issue:
> 
> With python 2.7, we end up with log_buf elements being of type 'str'
> (still), where as with python 3.X, they are of type 'byte'.
> 
> So we could solve this issue by checking the type of the arguments
> inside read_u16(), in utils.py (see below).
> 
> It would be more generic than checking for versions of python.

Actually I really like this (in small quantities). Instead of checking
an arbitrary version - it is checking the article it actually needs to
adapt for.

I fear the day that we may have conditionals littered everywhere due to
Python API change, - but actually we seem to be ok for the most part,
and that day is not today.

> I tested this both on 2.7 and 3.X (after having reproduced the problem
> you reported with my previous patch), and it works on both.
> 
> The changes made to dmesg.py would stay as they are, and it wouldn't
> have version dependent code in it.

Perfect. Generic dmesg :) with the read helpers doing the help to adapt!
Sounds like a good approach to me.

Will this have an effect on read_{u32,u64}? Or will they work as
expected (they call down to read_u16, so I think they will be ok)

> What do you think ? Should I submit another patch  using that approach ?

Yes, please do!

If you can, separate out the functional changes into their own commits,
it makes the integration process much easier. (changes to utils in one
patch, any improvements to dmesg in another)

Thanks

Kieran

> Regards
> 
> Dom
> 
> --------------
> 
> def read_u16(buffer):
>     value = [0, 0]
> 
>     if type(buffer[0]) is str:
>         value[0] = ord(buffer[0])
>         value[1] = ord(buffer[1])
>     else:
>         value[0] = buffer[0]
>         value[1] = buffer[1]
> 
>     if get_target_endianness() == LITTLE_ENDIAN:
>         return value[0] + (value[1] << 8)
>     else:
>         return value[1] + (value[0] << 8)
> 
> 
> 
> On Fri, Apr 29, 2016 at 11:03 AM, Kieran Bingham <kieran@xxxxxxxxxxx
> <mailto:kieran@xxxxxxxxxxx>> wrote:
> 
>     Hi Dom,
> 
>     On 08/04/16 04:33, Dom Cote wrote:
>     > Hi Kieran,
>     >
>     > Thanks for the feedback.
>     >
>     > I am going to need to build a version of gdb with python 2.7 to see what
>     > the differences are, and try to abstract them in the best possible way.
>     >
>     > Then I will send a new patch and incorporate what you just highlighted
>     > in it.
> 
>     I was just wondering if you had chance to look at any of this ? (I was
>     looking at it again myself)
> 
>     > Regards
>     >
>     > Dom
>     >
>     >
>     > On Thu, Apr 7, 2016 at 11:02 PM, Kieran Bingham
>     > <kieran.bingham@xxxxxxxxxx <mailto:kieran.bingham@xxxxxxxxxx>
>     <mailto:kieran.bingham@xxxxxxxxxx
>     <mailto:kieran.bingham@xxxxxxxxxx>>> wrote:
>     >
>     >     Hi Dom,
>     >
>     >     I've just tested your patch quickly, and it generates an error on
>     >     Python 2.7
>     >
>     >
>     >     On 05/04/16 19:38, Dom Cote wrote:
>     >     > When using GDB built with python 2.7,
>     >     >
>     >     > Inferior.read_memory (address, length)
>     >     >
>     >     > returns a buffer object. When using GDB built with python 3.X,
>     >     > it returns a memoryview object, which cannot be added to another
>     >     > memoryview object.
>     >     >
>     >     > Replace the addition (+) of 2 python memoryview objects
>     >     > with the addition of 2 'bytes' objects.
>     >     >
>     >     > Create a read_memoryview() function that always return a
>     memoryview
>     >     > object.
>     >
>     >     The change to memoryview appears to work - but I don't think
>     it needs to
>     >     be indirected by a function definition?
>     >
>     >
>     >     > Change the read_u16 function so it doesn't need to use ord()
>     >     > anymore.
>     >
>     >     This change is separate to the memoryview object, and should
>     be in it's
>     >     own patch. One patch should fix one thing independently.
>     >
>     >     For example, the change to memoryview object appears to be
>     functional,
>     >     and the read_u16 is not. If these changes are in separate
>     patches, then
>     >     working changes can be accepted sooner, and tested easier.
>     >
>     >
>     >     > Tested with Python 3.4 and gdb 7.7
>     >     >
>     >     > Signed-off-by: Dom Cote <buzdelabuz2@xxxxxxxxx
>     <mailto:buzdelabuz2@xxxxxxxxx> <mailto:buzdelabuz2@xxxxxxxxx
>     <mailto:buzdelabuz2@xxxxxxxxx>>>
>     >     > ---
>     >     >  scripts/gdb/linux/dmesg.py | 9 +++++----
>     >     >  scripts/gdb/linux/utils.py | 9 +++++++--
>     >     >  2 files changed, 12 insertions(+), 6 deletions(-)
>     >     >
>     >     > diff --git a/scripts/gdb/linux/dmesg.py b/scripts/gdb/linux/dmesg.py
>     >     > index 927d0d2a3145..96f4732157d8 100644
>     >     > --- a/scripts/gdb/linux/dmesg.py
>     >     > +++ b/scripts/gdb/linux/dmesg.py
>     >     > @@ -33,11 +33,12 @@ class LxDmesg(gdb.Command):
>     >     >          if log_first_idx < log_next_idx:
>     >     >              log_buf_2nd_half = -1
>     >     >              length = log_next_idx - log_first_idx
>     >     > -            log_buf = inf.read_memory(start, length)
>     >     > +            log_buf = utils.read_memoryview(inf, start, length).tobytes()
>     >
>     >     This looks like it could just call memoryview() directly ...
> 
>     I just looked at the line sizes:
> 
>     - log_buf = inf.read_memory(start, length)
>     + log_buf = utils.read_memoryview(inf, start, length).tobytes()
>     + log_buf = memoryview(inf.read_memory(start, length)).tobytes()
> 
>     There's not a lot in it, and saving one char is probably not enough
>     reason to add a function call.
> 
>     >
>     >     >          else:
>     >     >              log_buf_2nd_half = log_buf_len - log_first_idx
>     >     > -            log_buf = inf.read_memory(start,
>     log_buf_2nd_half) + \
>     >     > -                inf.read_memory(log_buf_addr, log_next_idx)
>     >     > +            a = utils.read_memoryview(inf, start,
>     log_buf_2nd_half)
>     >     > +            b = utils.read_memoryview(inf, log_buf_addr,
>     log_next_idx)
>     >
>     >     Likewise here I presume ...
>     >
>     >     > +            log_buf = a.tobytes() + b.tobytes()
>     >     >
>     >     >          pos = 0
>     >     >          while pos < log_buf.__len__():
>     >     > @@ -53,7 +54,7 @@ class LxDmesg(gdb.Command):
>     >     >              text = log_buf[pos + 16:pos + 16 + text_len]
>     >     >              time_stamp = utils.read_u64(log_buf[pos:pos + 8])
>     >     >
>     >     > -            for line in
>     memoryview(text).tobytes().splitlines():
>     >     > +            for line in text.splitlines():
>     >
>     >     This looks like a separate change, not related to the bug fix?
>     >     If this is an improvement, rather than a fix - it should
>     probably also
>     >     have it's own patch. (at first glance it looks like an
>     improvement :D )
>     >
>     >     I just haven't seen yet if it depends on the other change.
>     >
>     >     >                  gdb.write("[{time:12.6f}] {line}\n".format(
>     >     >                      time=time_stamp / 1000000000.0,
>     >     >                      line=line))
>     >     > diff --git a/scripts/gdb/linux/utils.py
>     b/scripts/gdb/linux/utils.py
>     >     > index 0893b326a28b..c2b779e7bd26 100644
>     >     > --- a/scripts/gdb/linux/utils.py
>     >     > +++ b/scripts/gdb/linux/utils.py
>     >     > @@ -87,11 +87,16 @@ def get_target_endianness():
>     >     >      return target_endianness
>     >     >
>     >     >
>     >     > +# Compat between GDB built with python 2.7 vs 3.X
>     >     > +def read_memoryview(inf, start, length):
>     >     > +    return memoryview(inf.read_memory(start, length))
>     >
>     >     This simply always returns a memoryview object, so why not
>     just change
>     >     the respective lines, which you have already had to modify to
>     call/use
>     >     memoryview directly?
>     >
>     >     > +
>     >     > +
>     >     >  def read_u16(buffer):
>     >     >      if get_target_endianness() == LITTLE_ENDIAN:
>     >     > -        return ord(buffer[0]) + (ord(buffer[1]) << 8)
>     >     > +        return buffer[0] + (buffer[1] << 8)
>     >     >      else:
>     >     > -        return ord(buffer[1]) + (ord(buffer[0]) << 8)
>     >     > +        return buffer[1] + (buffer[0] << 8)
>     >
>     >     This breaks for me, but returning these lines to use ord()
>     shows that
>     >     the memoryview changes appear to work.
>     >
>     >     What was the need for changing these lines? Does it cause a
>     break on
>     >     3.x?
>     >
>     >     This causes the following error on 2.7 for me:
>     >
>     >     (gdb) lx-dmesg
>     >     Traceback (most recent call last):
>     >     File
>     >   
>      "/home/linuxembedded/linaro/lkd/openst-lkd/objects/arm/qemu-arm/linux/scripts/gdb/linux/dmesg.py",
>     >     line 45, in invoke
>     >     length = utils.read_u16(log_buf[pos + 8:pos + 10])
>     >     File
>     >   
>      "/home/linuxembedded/linaro/lkd/openst-lkd/objects/arm/qemu-arm/linux/scripts/gdb/linux/utils.py",
>     >     line 97, in read_u16
>     >     return buffer[0] + (buffer[1] << 8)
>     >     TypeError: unsupported operand type(s) for <<: 'str' and 'int'
>     >     Error occurred in Python command: unsupported operand type(s)
>     for <<:
>     >     'str' and 'int'
> 
>     I guess we're going to have to do something along the lines of
>       PY2 = (sys.version_info[0] == 2)
> 
>       if PY2:
>          # Do Py2 compatible code
>       else:
>          # Do Py3+ stuff
> 
>     to support both targets. (Ugh...) But this isn't the first time I've
>     been asked if we support both Python 2 and Python 3 so I guess it will
>     become an issue.
> 
> 
>     Anyway, let me know if there's anything I can do to help / test.
> 
>     --
>     Regards
> 
>     Kieran Bingham
> 
> 

-- 
Regards

Kieran Bingham