[Tarantool-patches] [PATCH luajit v6 1/2] debug: generalized extension

[Sergey Kaplun]

Hi, Maxim!
Thanks for the patch!
Please consider my comments below.

On 04.04.24, Maxim Kokryashkin wrote:

<snipped>

> ---
>  src/luajit-gdb.py                     | 885 --------------------------
>  src/{luajit_lldb.py => luajit_dbg.py} | 616 ++++++++++++------

Since luajit_lldb.py is gone, please change the comment in <.flake8rc>.

>  2 files changed, 416 insertions(+), 1085 deletions(-)
>  delete mode 100644 src/luajit-gdb.py
>  rename src/{luajit_lldb.py => luajit_dbg.py} (63%)
> 
> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
> deleted file mode 100644

<snipped>

> diff --git a/src/luajit_lldb.py b/src/luajit_dbg.py
> similarity index 63%
> rename from src/luajit_lldb.py
> rename to src/luajit_dbg.py
> index 5ac11b65..a42d8f25 100644
> --- a/src/luajit_lldb.py
> +++ b/src/luajit_dbg.py
> @@ -1,10 +1,230 @@
> -# LLDB extension for LuaJIT post-mortem analysis.
> -# To use, just put 'command script import <path-to-repo>/src/luajit_lldb.py'
> -# in lldb.
> +# Debug extension for LuaJIT post-mortem analysis.
> +# To use in LLDB: 'command script import <path-to-repo>/src/luajit_dbg.py'
> +# To use in GDB: 'source <path-to-repo>/src/luajit_dbg.py'
>  
>  import abc
>  import re
> -import lldb
> +import sys
> +import types
> +
> +from importlib import import_module
> +
> +# make script compatible with the ancient Python {{{

Typo: s/make script/Make the script/

<snipped>

> +class Debugger(object):
> +    def __init__(self):
> +        self.GDB = False
> +        self.LLDB = False
> +
> +        debuggers = {
> +            'gdb': lambda lib: True,
> +            'lldb': lambda lib: lib.debugger is not None,
> +        }
> +        for name, healthcheck in debuggers.items():
> +            lib = None
> +            try:
> +                lib = import_module(name)
> +                if healthcheck(lib):

Why do we need this healthcheck? Why just import of the module isn't
enough?
Please add a comment near `debuggers` definition.

> +                    setattr(self, name.upper(), True)
> +                    globals()[name] = lib
> +                    self.name = name
> +            except Exception:
> +                continue
> +
> +        assert self.LLDB != self.GDB

<snipped>

> +
> +    def find_type(self, typename):
> +        if self.LLDB:
> +            return target.FindFirstType(typename)
> +        else:
> +            return gdb.lookup_type(typename)

Why do you drop the cache for types here? It may be critical when
running scripts for the search of objects on big coredumps or the
attached process.

> +
> +    def type_to_pointer_type(self, tp):
> +        if self.LLDB:
> +            return tp.GetPointerType()
> +        else:
> +            return tp.pointer()

<snipped>

> +
> +    def type_member_offset(self, member):
> +        if self.LLDB:
> +            return member.GetOffsetInBytes()
> +        else:
> +            return member.bitpos / 8

Should it be `//`?

<snipped>

> +class Struct(object):

Should we do this part for GDB too? I thought that this class generation
may be skipped for GDB.

>      def __init__(self, value):
>          self.value = value
>  
>      def __getitem__(self, name):

<snipped>

> +def make_property_from_metadata(field, tp):
> +    builtin = {
> +        'uint':   dbg.to_unsigned,
> +        'int':    dbg.to_signed,
> +        'string': dbg.to_str,
> +    }
> +    if tp in builtin.keys():
> +        return lambda self: builtin[tp](self[field])
> +    else:
> +        return lambda self: globals()[tp](self[field])
> +
> +
> +for cls, metainfo in c_structs.items():
> +    cls_dict = {}
> +    for field in metainfo:

May you please name field[0], field[1] as local variables for better
readability?

> +        if not isinstance(field[0], str):
> +            cls_dict[field[1]] = field[0]
> +        else:
> +            cls_dict[field[1]] = property(
> +                make_property_from_metadata(field[1], field[0])
> +            )
> +    globals()[cls] = type(cls, (Struct, ), cls_dict)
>  
>  
>  for cls in Struct.__subclasses__():
>      ptr_name = cls.__name__ + 'Ptr'
>  


<snipped>

> -
> -        ret = frame.EvaluateExpression(command)
> -        return ret
> +        return dbg.to_unsigned(dbg.eval(command))

Why do we need return unsigned here?

>  
>      @abc.abstractproperty
>      def command(self):
> @@ -270,7 +491,7 @@ class Command(object):

<snipped>

> @@ -278,6 +499,11 @@ class Command(object):
>          properly routed to LLDB frontend. Any unhandled exception will be
>          automatically transformed into proper errors.
>          """
> +    def invoke(self, arg, from_tty):
> +        try:
> +            self.execute(arg)
> +        except Exception as e:
> +            dbg.write(e)

Why do we need this change?

The error message for such situation is changed and non informative:

| Breakpoint 1, lj_cf_dofile (L=0x2) at /home/burii/reviews/luajit/lj-dbg/src/lib_base.c:429
| 429     {
| (gdb) lj-stack L
| Python Exception <class 'TypeError'>: unsupported operand type(s) for +: 'MemoryError' and 'str'
| Error occurred in Python: unsupported operand type(s) for +: 'MemoryError' and 'str'

Within the following implementation all works as expected.
|    def invoke(self, arg, from_tty):
|        self.execute(arg)

This produces more understandable reason of an error:
| (gdb) lj-stack L
| Python Exception <class 'gdb.MemoryError'>: Cannot access memory at address 0x26
| Error occurred in Python: Cannot access memory at address 0x26

Also, maybe it is good to add a test for this error.

<snipped>

-- 
Best regards,
Sergey Kaplun