Re: [Tarantool-patches] [PATCH luajit 1/3] dbg: introduce lj-ir, lj-jslots, lj-trace dumpers

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the review!
Fixed your comments and force-pushed the branch.

On 30.06.26, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for the patch! LGTM with minor comments.
> 
> Sergey
> 
> On 6/25/26 23:29, Sergey Kaplun wrote:
> > This patch adds dumpers for a single IR instruction (`lj-ir`), as well
> > as for all bytecodes inside one trace (`lj-trace`). Its dump is quite
> > similar to the -jdump flag but also reports types of register operands
> > (`ref`, `lit`, `cst`) and operation mode (`N`, `A`, `W`, etc.).
> > The `lj-trace` command accepts optional /rs flags to dump registers
> > associated with IR and snapshots for the trace correspondingly.
> > The `lj-ir` command can be used for dumping IR constants as well.
> > The `lj-jslots` command dumps the content of `J->slot`. It is useful to
> > simplify debugging of `rec_check_slots()` assertion failures.
> >
> > For LLDB value, the `__getitem__` metamethod now accepts bool keys.
> > Also, `__index__` is set to allow lldb.value to be used as an index
> > without explicit conversion to int. Old GDB versions (below 7.12) are
> > not supported because of the gdb.Value lacks the `__index__` metamethod
> > and can't be monkey-patched. The support for these versions may be added
> > by demand.
> >
> > Part of tarantool/tarantool#4808
> > ---
> >   src/luajit_dbg.py                             | 1216 ++++++++++++++++-
> >   .../debug-extension-tests.py                  |  365 +++++
> >   2 files changed, 1570 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/luajit_dbg.py b/src/luajit_dbg.py
> > index 2edb199a..fd6ca8a5 100644
> > --- a/src/luajit_dbg.py
> > +++ b/src/luajit_dbg.py
> > @@ -58,6 +58,26 @@ class Debugger(object):
> >               self.LLDB = True
> >               return super(Debugger, self).__new__(_LLDBDebugger)
> >   
> > +    def parse_flags(self, raw_flags, permitted_flags):
> > +        flags = {}
> > +        for flag in raw_flags:
> > +            if flag not in permitted_flags:
> > +                raise self.error('Unrecongnized option: "{}"'.format(flag))
> typo: s/Unrecongnized/Unrecognized/

Fixed, thanks!

===================================================================
diff --git a/src/luajit_dbg.py b/src/luajit_dbg.py
index 19cefaed..6412575c 100644
--- a/src/luajit_dbg.py
+++ b/src/luajit_dbg.py
@@ -62,7 +62,7 @@ class Debugger(object):
         flags = {}
         for flag in raw_flags:
             if flag not in permitted_flags:
-                raise self.error('Unrecongnized option: "{}"'.format(flag))
+                raise self.error('Unrecognized option: "{}"'.format(flag))
             flags[flag] = True
         return flags
 
===================================================================

> > +            flags[flag] = True
> > +        return flags
> > +

<snipped>

> > +    def member_by_offset(self, tp, offset, prev_name=None):
> > +        if isinstance(tp, str):
> > +            tp = self._dbgtype(tp)
> > +        assert offset < tp.sizeof, 'offset is bigger than object size'
> > +        if tp.code == gdb.TYPE_CODE_TYPEDEF:
> > +            tp = tp.strip_typedefs()
> > +        if tp.code == gdb.TYPE_CODE_STRUCT:
> > +            fields = tp.fields()
> > +            for n_field in range(len(fields)):
> > +                islast = n_field == (len(fields) - 1)
> > +                field = fields[n_field]
> > +                start_field = field.bitpos / 8
> may be //?

I'm not sure that this is crucial. In case when the bitpos isn't a
multiple of 8 we will get an error, so we may find the bug earlier
instead of an incorrect result. I'd prefer to ignore it if you don't
insist.

> > +                end_field = fields[n_field + 1].bitpos / 8 if not islast \
> > +                    else tp.sizeof
> > +                if start_field <= offset and offset < end_field:
> > +                    next_name = self.member_by_offset(
> > +                        field.type,
> > +                        offset - start_field,
> > +                        prev_name=field.name
> > +                    )
> > +                    return '.{field}{suffix}'.format(
> > +                        field=field.name,
> > +                        suffix=next_name if next_name else ''
> > +                    )

<snipped>

> > @@ -322,8 +428,26 @@ class _LLDBDebugger(Debugger):
> >           def lldb__getitem__(lldbval, key):
> >               if type(key) is lldb.value:
> >                   key = int(key)
> > +            if type(key) is bool:
> > +                key = int(key)
> >               if type(key) is int:
> >                   # Allow array access.
> > +                ltp = lldbval.sbvalue.GetType()
> > +                # XXX: LLDB in versions 17 - 19 can't use an array
> > +                # object as the initializer for `lldb.value` since
> > +                # `GetValue()` for it returns `None` leading to
> > +                # the invalid result. See
> > +                #https://github.com/llvm/llvm-project/pull/90144.
> > +                if (self.version < 17 or self.version > 19) or \
> > +                   ltp.GetTypeClass() != lldb.eTypeClassArray:
> > +                    pass
> probably it is better to invert condition and remove section with "pass"

The same condition is used for loading the global symbol, so it is more
bulletproof, I suppose. Also, this attracts an attention about the
mentioned issue. I'd prefer to leave it as is if you don't insist.

> > +
> > +
> > +def ir_kptr(ir):
> > +    irname = IRS[ir['o']]
> > +    assert irname == 'KPTR' or irname == 'KKPTR', 'wrong IR for ir_iptr()'
> typo: s/ir_iptr()/ir_kptr() or ir_kkptr()/

Fixed, thanks:

===================================================================
diff --git a/src/luajit_dbg.py b/src/luajit_dbg.py
index 6412575c..d5967716 100644
--- a/src/luajit_dbg.py
+++ b/src/luajit_dbg.py
@@ -2001,7 +2001,7 @@ def irt_toitype(irt):
 
 def ir_kptr(ir):
     irname = IRS[ir['o']]
-    assert irname == 'KPTR' or irname == 'KKPTR', 'wrong IR for ir_iptr()'
+    assert irname == 'KPTR' or irname == 'KKPTR', 'wrong IR for ir_kptr()'
     return mref('void *', dbg.cast('IRIns *', dbg.address(ir))[LJ_GC64]['ptr'])
 
 
===================================================================

> > +    return mref('void *', dbg.cast('IRIns *', dbg.address(ir))[LJ_GC64]['ptr'])

<snipped>

> > +def ir_kint64(ir):
> > +    irname = IRS[ir['o']]
> > +    assert irname == 'KINT64', 'wrong IR for ir_knum()'
> typo: s/ir_knum/ir_kint64/

Fixed, thanks:

===================================================================
diff --git a/src/luajit_dbg.py b/src/luajit_dbg.py
index d5967716..68f7970b 100644
--- a/src/luajit_dbg.py
+++ b/src/luajit_dbg.py
@@ -2019,7 +2019,7 @@ def ir_knum(ir):
 
 def ir_kint64(ir):
     irname = IRS[ir['o']]
-    assert irname == 'KINT64', 'wrong IR for ir_knum()'
+    assert irname == 'KINT64', 'wrong IR for ir_kint64()'
     return dbg.address(dbg.cast('IRIns *', dbg.address(ir))[1]['tv'])
 
 
===================================================================

<snipped>

> > +    for slot in range(0, snap['nslots']):
> > +        dump += ' '
> > +        snap_entry = int(snap_map[snap_entry_num])
> > +        if snap_entry_num < snap['nent'] and snap_entry >> TREF_SHIFT == slot:
> > +            snap_entry_num += 1
> > +            ref = int((snap_entry & TREF_REFMASK) - REF_BIAS)
> > +            if ref < 0:
> > +                if int(snap_entry) == 0x1057fff:
> magic number

Indeed, fixed:

===================================================================
diff --git a/src/luajit_dbg.py b/src/luajit_dbg.py
index 68f7970b..9534dfad 100644
--- a/src/luajit_dbg.py
+++ b/src/luajit_dbg.py
@@ -1956,6 +1956,7 @@ RID_SUNK = (RID_INIT - 2)
 SPS_NONE = 0
 
 REF_BIAS = 0x8000
+REF_NIL = REF_BIAS - 1
 
 TREF_SHIFT = 24
 
@@ -1964,8 +1965,11 @@ TREF_FRAME = 0x00010000
 TREF_CONT = 0x00020000
 # Snapshot flags and masks.
 SNAP_FRAME = 0x010000
+SNAP_NORESTORE = 0x040000
 SNAP_SOFTFPNUM = 0x080000
 
+SNAP_FR2_SLOT = (1 << TREF_SHIFT) | SNAP_FRAME | SNAP_NORESTORE + REF_NIL
+
 def irt_type(t):
     return dbg.cast('IRType', t['irt'] & IRT_TYPE)
 
@@ -2810,7 +2813,7 @@ def dump_snap(trace, snapno, snap):
             snap_entry_num += 1
             ref = int((snap_entry & TREF_REFMASK) - REF_BIAS)
             if ref < 0:
-                if int(snap_entry) == 0x1057fff:
+                if int(snap_entry) == SNAP_FR2_SLOT:
                     dump += '----'
                     continue
                 elif (snap_entry & TREF_CONT):
===================================================================

> <snipped>
> > +# Assume not cross-platform debugging.
> > +machine = os.uname().machine
> > +if machine == 'x86_64':
> > +    RX_GPR = r'r\w\w'
> > +    RX_FPR = r'xmm\d+'
> > +elif machine == 'arm64' or machine == 'aarch64':
> > +    RX_GPR = r'x\d+'
> > +    RX_FPR = r'd\d+'
> > +else:
> > +    raise Exception('Unknown archeticture in testing')
> typo: s/archeticture/architecture/

Fixed, thanks!
===================================================================
diff --git a/test/tarantool-debugger-tests/debug-extension-tests.py b/test/tarantool-debugger-tests/debug-extension-tests.py
index 71b763d2..895171a4 100644
--- a/test/tarantool-debugger-tests/debug-extension-tests.py
+++ b/test/tarantool-debugger-tests/debug-extension-tests.py
@@ -124,7 +124,7 @@ elif machine == 'arm64' or machine == 'aarch64':
     RX_GPR = r'x\d+'
     RX_FPR = r'd\d+'
 else:
-    raise Exception('Unknown archeticture in testing')
+    raise Exception('Unknown architecture in testing')
 
 
 class TestCaseBase(unittest.TestCase):
===================================================================


> <snipped>
> > +
> > +class TestLJIRConst(TestCaseBase):
> > +    location = 'trace_stop'
> > +
> > +    # No narrowing of 42.
> > +    if IS_DUALNUM:
> > +        # KNUM occupies 2 slots.
> > +        _knum_irnum = '6'
> > +        _kgc_irnum = '8' if IS_GC64 else '7'
> > +        _kptr_irnum = '10' if IS_GC64 else '8'
> > +    else:
> > +        # KNUM occupies 2 slots.
> > +        _knum_irnum = '8'
> > +        _kgc_irnum = '10' if IS_GC64 else '9'
> > +        _kptr_irnum = '12' if IS_GC64 else '10'
> both branches contains the same comment, is it a typo or not?

It is intentional. KNUM occupies 2 slots anyway.

> > <snipped>
> >   

-- 
Best regards,
Sergey Kaplun