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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the review!
See my answers below.
On Wed, Apr 17, 2024 at 07:00:00PM +0300, Sergey Kaplun wrote:
> 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>.
Fixed.
>
> >  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>
Fixed.
>
> > +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.
Added.
>
> > +                    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.
Well, I believe I just forgot about that. Added.
Side note: if only we could do the python3-only version of that,
the fix would just be a single decorator in front of the function
definition...
> > +
> > +    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 `//`?
Doesn't really matter, but ok. Fixed.
>
> <snipped>
>
> > +class Struct(object):
>
> Should we do this part for GDB too? I thought that this class generation
> may be skipped for GDB.
No, the whole idea is to encapsulate the debugger-specific things into
the debugger object and these classes. So we cannot skip them, they have
crucial role in this adapter.
>
> >      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?
Fixed.
>
> > +        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?
Because all of our commands accept either pointers, or numbers as
argumnents and lldb's eval may return a string instead.
>
> >
> >      @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)
Fixed.
>
> 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
>
Without the explicit exception output LLDB command just fails silently.
Choosing from a less readable error and not understanding what happened
(and whether it even happened at all) I would personally prefer the
first option.

> Also, maybe it is good to add a test for this error.
Kind of a strange idea to test whether the extension throws an unhandled
exception. It's like checking if Tarantool gives a segfault with a
certain backtrace after a NULL dereference.
>
> <snipped

Here is the diff with changes:
===
diff --git a/.flake8rc b/.flake8rc
index 6766ed41..6c1200fc 100644
--- a/.flake8rc
+++ b/.flake8rc
@@ -1,7 +1,7 @@
 [flake8]
 extend-ignore =
   # XXX: Suppress F821, since we have autogenerated names for
-  # 'ptr' type complements in luajit_lldb.py.
+  # 'ptr' type complements in luajit_dbg.py.
   F821
 per-file-ignores =
   # XXX: Flake considers regexp special characters to be
diff --git a/src/luajit_dbg.py b/src/luajit_dbg.py
index a42d8f25..e800b514 100644
--- a/src/luajit_dbg.py
+++ b/src/luajit_dbg.py
@@ -9,7 +9,7 @@ import types

 from importlib import import_module

-# make script compatible with the ancient Python {{{
+# Make the script compatible with the ancient Python {{{


 LEGACY = re.match(r'^2\.', sys.version)
@@ -31,7 +31,13 @@ class Debugger(object):
     def __init__(self):
         self.GDB = False
         self.LLDB = False
+        self.type_cache = {}

+        # XXX: While the `gdb` library is only available inside
+        # a debug session, the `lldb` library can be loaded in
+        # any Python script. To address that, we need to perform
+        # an additional check to ensure a debug session is
+        # actually running.
         debuggers = {
             'gdb': lambda lib: True,
             'lldb': lambda lib: lib.debugger is not None,
@@ -116,10 +122,12 @@ class Debugger(object):
         return val.value if self.LLDB else str(val)

     def find_type(self, typename):
-        if self.LLDB:
-            return target.FindFirstType(typename)
-        else:
-            return gdb.lookup_type(typename)
+        if typename not in self.type_cache:
+            if self.LLDB:
+                self.type_cache[typename] = target.FindFirstType(typename)
+            else:
+                self.type_cache[typename] = gdb.lookup_type(typename)
+        return self.type_cache[typename]

     def type_to_pointer_type(self, tp):
         if self.LLDB:
@@ -190,7 +198,7 @@ class Debugger(object):
         if self.LLDB:
             return member.GetOffsetInBytes()
         else:
-            return member.bitpos / 8
+            return member.bitpos // 8

     def get_member(self, value, member_name):
         if self.LLDB:
@@ -437,11 +445,13 @@ def make_property_from_metadata(field, tp):
 for cls, metainfo in c_structs.items():
     cls_dict = {}
     for field in metainfo:
-        if not isinstance(field[0], str):
-            cls_dict[field[1]] = field[0]
+        prop_constructor = field[0]
+        prop_name = field[1]
+        if not isinstance(prop_constructor, str):
+            cls_dict[prop_name] = prop_constructor
         else:
-            cls_dict[field[1]] = property(
-                make_property_from_metadata(field[1], field[0])
+            cls_dict[prop_name] = property(
+                make_property_from_metadata(prop_name, prop_constructor)
             )
     globals()[cls] = type(cls, (Struct, ), cls_dict)

===
>
> --
> Best regards,
> Sergey Kaplun