Subject: Re: [PATCH] Fix issue with dmesg.py and python 3.X
To: Dom Cote <buzdelabuz2@gmail.com>, jan.kiszka@siemens.com
References: <20160406023831.GA12697@iMacLub>
Cc: linux-kernel@vger.kernel.org
From: Kieran Bingham <kieran.bingham@linaro.org>
Message-ID: <57071F4A.90404@linaro.org>
Date: Thu, 7 Apr 2016 20:02:34 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <20160406023831.GA12697@iMacLub>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4817
Lines: 142

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

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

>  
>  
>  def read_u32(buffer):
> 


--
Regards

Kieran