[PATCH 1/2] gdb/scripts: lx-dmesg: Cast log_buf to void* for addr fetch

[PATCH 1/2] gdb/scripts: lx-dmesg: Cast log_buf to void* for addr fetch

[Leonard Crestez]

In some cases it is possible for the str() conversion here to throw
encoding errors because log_buf might not point to valid ascii. For
example:

(gdb) python print str(gdb.parse_and_eval("log_buf"))
Traceback (most recent call last):
  File "<string>", line 1, in <module>
UnicodeEncodeError: 'ascii' codec can't encode character u'\u0303' in
	position 24: ordinal not in range(128)

Avoid this by explicitly casting to (void *) inside the gdb expression.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 scripts/gdb/linux/dmesg.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/gdb/linux/dmesg.py b/scripts/gdb/linux/dmesg.py
index 5afd109..6f8d2b2 100644
--- a/scripts/gdb/linux/dmesg.py
+++ b/scripts/gdb/linux/dmesg.py
@@ -24,7 +24,7 @@ class LxDmesg(gdb.Command):
 
     def invoke(self, arg, from_tty):
         log_buf_addr = int(str(gdb.parse_and_eval(
-            "'printk.c'::log_buf")).split()[0], 16)
+            "(void*)'printk.c'::log_buf")).split()[0], 16)
         log_first_idx = int(gdb.parse_and_eval("'printk.c'::log_first_idx"))
         log_next_idx = int(gdb.parse_and_eval("'printk.c'::log_next_idx"))
         log_buf_len = int(gdb.parse_and_eval("'printk.c'::log_buf_len"))
-- 
2.7.4

[PATCH 2/2] scripts/gdb: lx-dmesg: Use errors=replace for decoding

[Leonard Crestez]

It is never desirable lx-dmesg to fail on string decoding errors, not
even if the log buffer is corrupt.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 scripts/gdb/linux/dmesg.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/gdb/linux/dmesg.py b/scripts/gdb/linux/dmesg.py
index 6f8d2b2..d0cac58 100644
--- a/scripts/gdb/linux/dmesg.py
+++ b/scripts/gdb/linux/dmesg.py
@@ -52,13 +52,13 @@ class LxDmesg(gdb.Command):
                 continue
 
             text_len = utils.read_u16(log_buf[pos + 10:pos + 12])
-            text = log_buf[pos + 16:pos + 16 + text_len].decode()
+            text = log_buf[pos + 16:pos + 16 + text_len].decode(errors='replace')
             time_stamp = utils.read_u64(log_buf[pos:pos + 8])
 
             for line in text.splitlines():
                 gdb.write("[{time:12.6f}] {line}\n".format(
                     time=time_stamp / 1000000000.0,
-                    line=line))
+                    line=line.encode(errors='replace')))
 
             pos += length
 
-- 
2.7.4

Re: [PATCH 1/2] gdb/scripts: lx-dmesg: Cast log_buf to void* for addr fetch

[Jan Kiszka]

On 2017-06-23 16:20, Leonard Crestez wrote:
> In some cases it is possible for the str() conversion here to throw
> encoding errors because log_buf might not point to valid ascii. For
> example:
> 
> (gdb) python print str(gdb.parse_and_eval("log_buf"))
> Traceback (most recent call last):
>   File "<string>", line 1, in <module>
> UnicodeEncodeError: 'ascii' codec can't encode character u'\u0303' in
> 	position 24: ordinal not in range(128)
> 
> Avoid this by explicitly casting to (void *) inside the gdb expression.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  scripts/gdb/linux/dmesg.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/gdb/linux/dmesg.py b/scripts/gdb/linux/dmesg.py
> index 5afd109..6f8d2b2 100644
> --- a/scripts/gdb/linux/dmesg.py
> +++ b/scripts/gdb/linux/dmesg.py
> @@ -24,7 +24,7 @@ class LxDmesg(gdb.Command):
>  
>      def invoke(self, arg, from_tty):
>          log_buf_addr = int(str(gdb.parse_and_eval(
> -            "'printk.c'::log_buf")).split()[0], 16)
> +            "(void*)'printk.c'::log_buf")).split()[0], 16)

Nit: (void *)

>          log_first_idx = int(gdb.parse_and_eval("'printk.c'::log_first_idx"))
>          log_next_idx = int(gdb.parse_and_eval("'printk.c'::log_next_idx"))
>          log_buf_len = int(gdb.parse_and_eval("'printk.c'::log_buf_len"))
> 

Looks good, makes sense to me.

Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com>

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] scripts/gdb: lx-dmesg: Use errors=replace for decoding

[Jan Kiszka]

On 2017-06-23 16:20, Leonard Crestez wrote:
> It is never desirable lx-dmesg to fail on string decoding errors, not
> even if the log buffer is corrupt.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  scripts/gdb/linux/dmesg.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/gdb/linux/dmesg.py b/scripts/gdb/linux/dmesg.py
> index 6f8d2b2..d0cac58 100644
> --- a/scripts/gdb/linux/dmesg.py
> +++ b/scripts/gdb/linux/dmesg.py
> @@ -52,13 +52,13 @@ class LxDmesg(gdb.Command):
>                  continue
>  
>              text_len = utils.read_u16(log_buf[pos + 10:pos + 12])
> -            text = log_buf[pos + 16:pos + 16 + text_len].decode()
> +            text = log_buf[pos + 16:pos + 16 + text_len].decode(errors='replace')

pep8 should complain.

>              time_stamp = utils.read_u64(log_buf[pos:pos + 8])
>  
>              for line in text.splitlines():
>                  gdb.write("[{time:12.6f}] {line}\n".format(
>                      time=time_stamp / 1000000000.0,
> -                    line=line))
> +                    line=line.encode(errors='replace')))

You only talk about "decoding" in the commit log, but here you encode
back. An short explanation why this is also needed would be nice.

>  
>              pos += length
>  
> 

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] scripts/gdb: lx-dmesg: Use errors=replace for decoding

[Leonard Crestez]

On Fri, 2017-06-23 at 18:02 +0200, Jan Kiszka wrote:
> On 2017-06-23 16:20, Leonard Crestez wrote:
> > 
> > It is never desirable lx-dmesg to fail on string decoding errors,
> > not
> > even if the log buffer is corrupt.
> > 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > ---
> >  scripts/gdb/linux/dmesg.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/scripts/gdb/linux/dmesg.py
> > b/scripts/gdb/linux/dmesg.py
> > index 6f8d2b2..d0cac58 100644
> > --- a/scripts/gdb/linux/dmesg.py
> > +++ b/scripts/gdb/linux/dmesg.py
> > @@ -52,13 +52,13 @@ class LxDmesg(gdb.Command):
> >                  continue
> >  
> >              text_len = utils.read_u16(log_buf[pos + 10:pos + 12])
> > -            text = log_buf[pos + 16:pos + 16 + text_len].decode()
> > +            text = log_buf[pos + 16:pos + 16 +
> > text_len].decode(errors='replace')
> pep8 should complain.
> 
> > 
> >              time_stamp = utils.read_u64(log_buf[pos:pos + 8])
> >  
> >              for line in text.splitlines():
> >                  gdb.write("[{time:12.6f}] {line}\n".format(
> >                      time=time_stamp / 1000000000.0,
> > -                    line=line))
> > +                    line=line.encode(errors='replace')))
> You only talk about "decoding" in the commit log, but here you encode
> back. An short explanation why this is also needed would be nice.
> 
Apparently .decode(errors='replace') will return an unicode string
where invalid characters are replaced with U+FFFD REPLACEMENT
CHARACTER. Attempting to encode that back to the default ascii encoding
of python2 throws an error, using errors='replace' results in a '?'
instead.

See: https://docs.python.org/2/library/codecs.html#codec-base-classes

In python3 the default encoding seems to be utf8 and errors='replace'
is not obviously required on the encode step.

I don't actually have a gdb version compiled with python3 support and
don't know if gdb.write always properly handles unicode in all cases.
Perhaps it might be better to also explicitly specify 'utf8' as the
encoding?

Linux does occasionally print unicode, for example the jffs2 driver
shows an copyright symbol at startup. Using errors='replace' everywhere
on python2 results in this output from lx-dmesg:

[    0.367578] jffs2: version 2.2. (NAND) ?? 2001-2006 Red Hat, Inc.

In theory if we use decode('utf8', errors='replace') and encode('utf8')
then errors='replace' would not be required on the encode side.
Honestly for debug code it might be preferable to do the safest
possible thing and go 'ascii' everywhere.

--
Regards,
Leonard

Re: [PATCH 2/2] scripts/gdb: lx-dmesg: Use errors=replace for decoding

[Jan Kiszka]

On 2017-06-23 19:29, Leonard Crestez wrote:
> On Fri, 2017-06-23 at 18:02 +0200, Jan Kiszka wrote:
>> On 2017-06-23 16:20, Leonard Crestez wrote:
>>>
>>> It is never desirable lx-dmesg to fail on string decoding errors,
>>> not
>>> even if the log buffer is corrupt.
>>>
>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>> ---
>>>  scripts/gdb/linux/dmesg.py | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/scripts/gdb/linux/dmesg.py
>>> b/scripts/gdb/linux/dmesg.py
>>> index 6f8d2b2..d0cac58 100644
>>> --- a/scripts/gdb/linux/dmesg.py
>>> +++ b/scripts/gdb/linux/dmesg.py
>>> @@ -52,13 +52,13 @@ class LxDmesg(gdb.Command):
>>>                  continue
>>>  
>>>              text_len = utils.read_u16(log_buf[pos + 10:pos + 12])
>>> -            text = log_buf[pos + 16:pos + 16 + text_len].decode()
>>> +            text = log_buf[pos + 16:pos + 16 +
>>> text_len].decode(errors='replace')
>> pep8 should complain.
>>
>>>
>>>              time_stamp = utils.read_u64(log_buf[pos:pos + 8])
>>>  
>>>              for line in text.splitlines():
>>>                  gdb.write("[{time:12.6f}] {line}\n".format(
>>>                      time=time_stamp / 1000000000.0,
>>> -                    line=line))
>>> +                    line=line.encode(errors='replace')))
>> You only talk about "decoding" in the commit log, but here you encode
>> back. An short explanation why this is also needed would be nice.
>>
> Apparently .decode(errors='replace') will return an unicode string
> where invalid characters are replaced with U+FFFD REPLACEMENT
> CHARACTER. Attempting to encode that back to the default ascii encoding
> of python2 throws an error, using errors='replace' results in a '?'
> instead.
> 
> See: https://docs.python.org/2/library/codecs.html#codec-base-classes
> 
> In python3 the default encoding seems to be utf8 and errors='replace'
> is not obviously required on the encode step.
> 
> I don't actually have a gdb version compiled with python3 support and
> don't know if gdb.write always properly handles unicode in all cases.
> Perhaps it might be better to also explicitly specify 'utf8' as the
> encoding?
> 
> Linux does occasionally print unicode, for example the jffs2 driver
> shows an copyright symbol at startup. Using errors='replace' everywhere
> on python2 results in this output from lx-dmesg:
> 
> [    0.367578] jffs2: version 2.2. (NAND) ?? 2001-2006 Red Hat, Inc.
> 
> In theory if we use decode('utf8', errors='replace') and encode('utf8')
> then errors='replace' would not be required on the encode side.
> Honestly for debug code it might be preferable to do the safest
> possible thing and go 'ascii' everywhere.

I just wanted to ensure that this information is saved. If you fix the
overlong line and add something like the first paragraph to your commit
log, you can add my reviewed-by tag.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux