[Tarantool-patches] [PATCH luajit v6 1/2] debug: generalized extension
Maxim Kokryashkin
m.kokryashkin at tarantool.org
Thu Apr 18 01:42:54 MSK 2024
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
More information about the Tarantool-patches
mailing list