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

[Kieran Bingham]

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