Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753732AbcD2PD2 (ORCPT ); Fri, 29 Apr 2016 11:03:28 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:32869 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752819AbcD2PD0 (ORCPT ); Fri, 29 Apr 2016 11:03:26 -0400 Subject: Re: [PATCH] Fix issue with dmesg.py and python 3.X To: Dom Cote References: <20160406023831.GA12697@iMacLub> <57071F4A.90404@linaro.org> Cc: "J. Kiszka" , linux-kernel@vger.kernel.org From: Kieran Bingham Message-ID: <572377B6.9020608@bingham.xyz> Date: Fri, 29 Apr 2016 16:03:18 +0100 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: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6789 Lines: 187 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 > > 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 > > > --- > > 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