Re: [Tarantool-patches] [PATCH luajit 2/3] dbg: introduce lj-ctype command, extend cdata dump

[Evgeniy Temirgaleev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 9585 bytes --]

Hi, Sergey!

LGTM. And please see my note about renaming issue.

> 
> From: Sergey Kaplun <skaplun@tarantool.org>
> To: Evgeniy Temirgaleev <e.temirgaleev@tarantool.org>
> Cc: tarantool-patches@dev.tarantool.org, Sergey Bronnikov <sergeyb@tarantool.org
> >
> Date: Monday, June 29, 2026 10:21 PM +03:00
> Hi, Evgeniy!
> Thanks for the review!
> Fixed your comments and force-pushed the branch.
> 
> On 29.06.26, Evgeniy Temirgaleev wrote:
> > Hi, Sergey!
> >
> > Thanks for the patch!
> > Please, see the commends below.
> >
> > --
> > Best regards,
> > Evgeniy Temirgaleev
> >
> > >
> > > From: Sergey Kaplun <skaplun@tarantool.org>
> > > To: Sergey Bronnikov <sergeyb@tarantool.org>, Evgeniy Temirgaleev <e.temirgaleev@tarantool.org
> 
> > > >
> > > Cc: tarantool-patches@dev.tarantool.org, Sergey Kaplun <skaplun@tarantool.org
> 
> > > >
> > > Date: Thursday, June 25, 2026 11:29 PM +03:00
> > > This patch extends dumped information for the given cdata object. Now
> > > it resolves the given `CType` and prints it in the format similar to
> the
> > > `__tostring` metamethod. The `lj-ctype` command is introduced to dump
> > > this information where there is only the `CType` pointer but no cdata
> > > associated with it.
> > >
> > > `__or__` and `__ror__` metamethods are monkey-patched for the LLDB
> value
> > > object. In `__sub__` metamethod for LLDB pointers `GetPointeeType()`
> is
> > > used to get the pointee type instead of the incorrectly used
> > > `GetDereferencedType()` which always returns the same type with size
> 8.
> > > Casting from negative values to the unsigned values is supported to
> > > check `CTF_UCHAR`.
> > >
> > > Part of tarantool/tarantool#4808
> > > ---
> > > src/luajit_dbg.py | 333 +++++++++++++++++-
> > > .../debug-extension-tests.py | 208 ++++++++++-
> > > 2 files changed, 535 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/src/luajit_dbg.py b/src/luajit_dbg.py
> > > index fd6ca8a5..62cd65d5 100644
> > > --- a/src/luajit_dbg.py
> > > +++ b/src/luajit_dbg.py
> > > @@ -386,6 +386,8 @@ class _LLDBDebugger(Debugger):
> > > pack_flag = '<q'
> > > else:
> > > pack_flag = '<Q'
> > > + # Cast to unsigned.
> > >
> >
> > Is /unsigned/uint64_t/ clearly?
> 
> Rephrased as you suggested. Also, lowercase the value to be consistent
> with other hexademical values.
> 

Thanks!

> 
> 
> ===================================================================
> diff --git a/src/luajit_dbg.py b/src/luajit_dbg.py
> index 6b0827d9..0769f2ee 100644
> --- a/src/luajit_dbg.py
> +++ b/src/luajit_dbg.py
> @@ -386,8 +386,8 @@ class _LLDBDebugger(Debugger):
> pack_flag = '<q'
> else:
> pack_flag = '<Q'
> - # Cast to unsigned.
> - raw_value &= 0xFFFFFFFFFFFFFFFF
> + # Cast to 64-bit unsigned value in Python.
> + raw_value &= 0xffffffffffffffff
> raw_data = struct.pack(pack_flag, raw_value)
> sbdata = lldb.SBData()
> sbdata.SetData(
> ===================================================================
> 
> >
> > >
> > > + raw_value &= 0xFFFFFFFFFFFFFFFF
> > > raw_data = struct.pack(pack_flag, raw_value)
> > > sbdata = lldb.SBData()
> > > sbdata.SetData(
> 
> <snipped>
> 
> > > +
> > > +
> > > +def ctinfo(ct, flags):
> > >
> >
> > May we name this function ‘CTINFO’ as in ‘lj_ctype.h’? Or leave a
> comment with an original name for quick grep.
> 
> Added a comment since the upper case is used for constants.
> 

Thanks!

> 
> 
> ===================================================================
> diff --git a/src/luajit_dbg.py b/src/luajit_dbg.py
> index 0769f2ee..de83a2b5 100644
> --- a/src/luajit_dbg.py
> +++ b/src/luajit_dbg.py
> @@ -1427,6 +1427,7 @@ def ctype_attrib(info):
> return (info >> CTSHIFT_ATTRIB) & CTMASK_ATTRIB
> 
> 
> +# Implementation of the `CTINFO()` macro.
> def ctinfo(ct, flags):
> return (tou32(ct) << CTSHIFT_NUM) + flags
> 
> ===================================================================
> 
> 
> >
> > >
> > > + return (tou32(ct) << CTSHIFT_NUM) + flags
> > > +
> > > +
> > > +def ctype_isptr(info):
> > > + return ctype_type(info) == CT_PTR
> > > +
> > > +
> > > +def ctype_iscomplex(info):
> > > + return (info & (CTMASK_NUM | CTF_COMPLEX)) == ctinfo(CT_ARRAY,
> > > CTF_COMPLEX)
> > > +
> > > +
> > > +def ctype_isinteger(info):
> > > + return (info & (CTMASK_NUM | CTF_BOOL | CTF_FP)) == ctinfo(CT_NUM,
> 0)
> > > +
> > > +
> > > +def ctype_isrefarray(info):
> > > + return (info & (CTMASK_NUM | CTF_VECTOR | CTF_COMPLEX)) == \
> > > + ctinfo(CT_ARRAY, 0)
> > > +
> > > +
> > > +def ctype_cid(info):
> > >
> >
> > Let’s put these function definitions in the ‘lj_ctype.h’ order?
> > May we group the definitions by corresponding C files also? # lj_ctype.h
> … # lj_cdata.h … # lj_xxx.c …
> 
> 
> Sorted as you suggested. The sorting is the following:
> * lj_ctype.h
> * lj_cdata.h -- `cdata_getptr()`
> * lj_obj.h -- `cdataptr()`
> 

Thanks! We can add a comment with file name before each block to improve readability slightly more. Feel free to ignore.

> 
> 
> 

<snipped>

> 
> 
> >
> > >
> > > + return info & CTMASK_CID
> > > +
> > > +
> > > +def ctype_child(cts, ctype):
> > > + return ctype_get(cts, ctype_cid(ctype['info']))
> > > +
> > > +
> > > +def cdataptr(cd):
> > > + return dbg.cast('void *', (cd + 1))
> > > +
> > > +
> > > +def cdata_getptr(p, size):
> > > + if LJ_64 and size == 4:
> > > + return dbg.cast('void *', dbg.cast('uint32_t *', p)[0])
> > > + else:
> > >
> >
> > assert for size == 8 ?
> 
> Added since it may possibly lead to the incorrect (shrinked) pointer
> result. If we ever see the 16-bit pointers. Also, support the 32-bit
> systems (not LJ_64) as well.
> 

Thanks!

> 
> 
> ================================================================
> diff --git a/src/luajit_dbg.py b/src/luajit_dbg.py
> index 183dda3b..de22d450 100644
> --- a/src/luajit_dbg.py
> +++ b/src/luajit_dbg.py
> @@ -1463,9 +1463,10 @@ def ctype_typeid(cts, ct):
> 
> 
> def cdata_getptr(p, size):
> - if LJ_64 and size == 4:
> + if (LJ_64 and size == 4) or not LJ_64:
> return dbg.cast('void *', dbg.cast('uint32_t *', p)[0])
> else:
> + assert size == 8, 'incorrect pointer size'
> return dbg.cast('void *', dbg.cast('uint64_t *', p)[0])
> 
> 
> ================================================================
> 
> >
> > >
> > > + return dbg.cast('void *', dbg.cast('uint64_t *', p)[0])
> > > +
> > > +
> 
> <snipped>
> 
> > > +def ctype_prepnum(ctypestr, info, size):
> > >
> >
> > Func proto differs with lj_ctype.c (static void ctype_prepnum(CTRepr
> *ctr, uint32_t n)).
> > It seems, you move some of ctype_repr() code here. Let’s comment it?
> 
> ===================================================================
> diff --git a/src/luajit_dbg.py b/src/luajit_dbg.py
> index de22d450..28cbe97d 100644
> --- a/src/luajit_dbg.py
> +++ b/src/luajit_dbg.py
> @@ -2479,6 +2479,8 @@ def ctype_preptype(cts, ctypestr, ctype, qual, tp):
> return ctypestr
> 
> 
> +# Partially moved the code from `ctype_repr()` here to make it
> +# more readable.
> def ctype_prepnum(ctypestr, info, size):
> if info & CTF_BOOL:
> ctypestr = ctype_preplit(ctypestr, 'bool')
> ===================================================================
> 

Thanks!

> 
> 
> <snipped>
> 
> > > +def dump_ctype(ct):
> > >
> >
> > Also, it seems, it will be easy to read to code, if it will be possible
> to distinguish between ported functions and extension itself ones. May be
> by use the ‘dbg_’ prefix for extension function names.
> 
> I suppose this refactoring can be done in the separate issue. Since it
> is related to all functions. Also, the `dbg` is already used for the
> instance of the corresponding class. `dump_` prefix looks common for all
> dumpers of our extension.
> 

Agreed. And I vote for this patch.
May be it will be several documented prefixes. It will be more verbosely, but I think it will be very helpful in a long perspective for supporting the extension to quick distinguish LuaJIT-ported routine e.g. `ctype_preplit` with extension routine e.g. `cdata_val_int64`.
Can you offer some prefix name good for you now? May be we can start naming with it at this point, what do you think?

> 
> 
> 
> <skipped>
> 
> > > +class TestLJCTypeBase(TestCaseBase):
> > > + location = 'lj_cf_ffi_new'
> > > + extension_cmds = (
> > > + # Load `ct`. Skip inlined functions for LLDB.
> > >
> >
> > The extension command set is common for GDB and LLDB. Does we skip for
> GDB also?
> 
> For GDB this function isn't inlined, but these n-s are harmless.
> Adjusted the comment.
> 

Thanks!

> 
> 
> ===================================================================
> diff --git a/test/tarantool-debugger-tests/debug-extension-tests.py
> b/test/tarantool-debugger-tests/debug-extension-tests.py
> index f17de27e..71b763d2 100644
> --- a/test/tarantool-debugger-tests/debug-extension-tests.py
> +++ b/test/tarantool-debugger-tests/debug-extension-tests.py
> @@ -1033,7 +1033,9 @@ class TestLJCTypeFunc(TestCaseBase):
> class TestLJCTypeBase(TestCaseBase):
> location = 'lj_cf_ffi_new'
> extension_cmds = (
> - # Load `ct`. Skip inlined functions for LLDB.
> + # Load `ct`. Skip inlined functions for LLDB. The skip is
> + # harmless for GDB since we are still in the body of the
> + # function.
> 'n\n'
> 'n\n'
> 'n\n'
> ===================================================================
> 
> 
> >
> > >
> > > + 'n\n'
> > > + 'n\n'
> > > + 'n\n'
> > > + 'n\n'
> > > + 'n\n'
> > > + 'n\n'
> > > + 'lj-ctype ct\n'
> > > + )
> 
> <snipped>
> 
> > > --
> > > 2.54.0
> > >
> 
> --
> Best regards,
> Sergey Kaplun
> 

--
Best regards,
Evgeniy Temirgaleev

[-- Attachment #2: Type: text/html, Size: 14187 bytes --]