Fix issue with dmesg.py and python 3.X

PATCH In review Dom Cote 3 participants 4 messages Apr 6, 2016 – May 3, 2016 2 messages not in the archive GDB kernel debugging helper scripts

lore

Share & tools

lore

Dom Cote

author

2016-04-06 02:38 UTC #0 lore raw

PATCH 1/1 Fix issue with dmesg.py and python 3.X

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.

Change the read_u16 function so it doesn't need to use ord() anymore.

Tested with Python 3.4 and gdb 7.7

Signed-off-by Dom Cote <[email protected]>

---
 scripts/gdb/linux/dmesg.py | 9 +++++----
 scripts/gdb/linux/utils.py | 9 +++++++--
 2 files changed, 12 insertions(+), 6 deletions(-)

scripts/gdb/linux/dmesg.py +5 -4

index 927d0d2a3145..96f4732157d8 100644
@@ -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()
         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)
+            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():
                 gdb.write("[{time:12.6f}] {line}\n".format(
                     time=time_stamp / 1000000000.0,
                     line=line))

scripts/gdb/linux/utils.py +7 -2

index 0893b326a28b..c2b779e7bd26 100644
@@ -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))
+
+
 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)
 
 
 def read_u32(buffer):

Signature · 1 line

1.9.1

Kieran Bingham

2016-04-08 03:02 UTC #1 lore raw

Hi Dom,

I've just tested your patch quickly, and it generates an error on Python 2.7

Quoted 8 lines · Dom Cote

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.

Quoted 5 lines

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?

Quoted 2 lines

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.

Quoted 13 lines

Tested with Python 3.4 and gdb 7.7
Signed-off-by Dom Cote <[email protected]>
---
 scripts/gdb/linux/dmesg.py | 9 +++++----
 scripts/gdb/linux/utils.py | 9 +++++++--
 2 files changed, 12 insertions(+), 6 deletions(-)
scripts/gdb/linux/dmesg.py +0 -0
index 927d0d2a3145..96f4732157d8 100644

scripts/gdb/linux/dmesg.py class LxDmesg(gdb.Command): · line 36

Quoted 5 lines

scripts/gdb/linux/dmesg.py +1 -1

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

scripts/gdb/linux/dmesg.py class LxDmesg(gdb.Command): · line 40

Quoted 6 lines

scripts/gdb/linux/dmesg.py +2 -2

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

scripts/gdb/linux/dmesg.py class LxDmesg(gdb.Command): · line 57

Quoted 10 lines

scripts/gdb/linux/dmesg.py +2 -1

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

Quoted 8 lines

scripts/gdb/linux/dmesg.py +0 -0

                 gdb.write("[{time:12.6f}] {line}\n".format(
                     time=time_stamp / 1000000000.0,
                     line=line))

scripts/gdb/linux/utils.py +0 -0

index 0893b326a28b..c2b779e7bd26 100644

scripts/gdb/linux/utils.py def get_target_endianness(): · line 92

Quoted 6 lines

scripts/gdb/linux/utils.py +3 -0

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

scripts/gdb/linux/utils.py def get_target_endianness(): · line 99

Quoted 9 lines

scripts/gdb/linux/utils.py +4 -2

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

scripts/gdb/linux/utils.py def get_target_endianness(): · line 101

Quoted 4 lines

scripts/gdb/linux/utils.py +0 -0
 def read_u32(buffer):
 

--
Regards

Kieran

A message that is not in the archive (sent off-list or not yet received): CA+Bh5YxOw_NcrMk7MhN22nX35kc2bFgMrHZo_aFh7gk4o-5cmg@mail.gmail.com

Kieran Bingham

2016-04-29 15:03 UTC #3 lore raw

Hi Dom,

Quoted 9 lines · Dom Cote

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)

Quoted 63 lines

Regards
Dom
On Thu, Apr 7, 2016 at 11:02 PM, Kieran Bingham <[email protected] <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>> > ---
> 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()

Quoted 1 line

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.

Quoted 69 lines

scripts/gdb/linux/dmesg.py class LxDmesg(gdb.Command): · line 40

Quoted 7 lines

else:
log_buf_2nd_half = log_buf_len - log_first_idx

scripts/gdb/linux/dmesg.py +2 -2

-            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

Quoted 8 lines

    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.

Signature · 3 lines

Regards

Kieran Bingham

A message that is not in the archive (sent off-list or not yet received): CA+Bh5YwYRKVywgKY5Jf+Hy33Y9mTCL+_LdUKBuXuXHvr6bcZxw@mail.gmail.com

Kieran Bingham

2016-05-03 08:34 UTC #5 lore raw

Quoted 1 line · Dom Cote

Hi Kieran,

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

Quoted 8 lines

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!

Quoted 9 lines

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.

Quoted 5 lines

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)

Quoted 1 line

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

Quoted 238 lines

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 <[email protected] <mailto:[email protected]>> 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
> <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> 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 <[email protected]
<mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>>
> > ---
> > 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

Signature · 3 lines

Regards

Kieran Bingham