Re: [Tarantool-patches] [PATCH luajit] dbg: avoid hardcoded enums (get them from target)

[Mikhail Elhimov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!

Thanks for the review! See my comments below

On 21.09.2026 21:47, Sergey Kaplun wrote:
> Hi, Mikhail!
> Thanks for the patch!
> I really like this! It helps to avoid copy-pasting and makes the code
> much more robust. Brilliant idea to use type of enums!
> I left some stillistic comments and asked for some patch clean-up
> below.
>
> On 09.09.26, Mikhail Elhimov wrote:
>> Besides reducing lines of code this way the extension become compatible
> Typo: s/become/becomes/
Fixed
>> with various version of luajit because different version might use
> Typo: s/version/versions/g
Fixed
>> different set of enum members (newer version might introduce additional
> Typo: s/set/sets/
> Typo: s/newer/a newer/
Fixed
>> BC/IR/etc.)
>>
>> Prior to this patch string was used as a debugger-agnostic way to
> Typo: s/this/this, the/
Fixed
>> specify type, but this way might not work in case of enum because
> Typo: s/case of enum/the case of an enum/
Fixed
>> debugging information might be optimized out if no variable of such enum
> Typo: s/enum/an enum/
Fixed
>> type is declared and its members are used only as a predefined
> Typo: s/a //
Fixed
>> constants. The type of enum is needed to be able to get human readable
> Typo: s/human readable/human-readable/
Fixed
>> value (member name instead of number). The alternative way to get enum
>> type is get it from the value, i.e. somehow obtain the value that is of
> Typo: s/is get/is to get/
Fixed
>> enum type and then get its type object.
>>
>> To do that, separate method to create enum value was introduced in
> Typo: s/separate/a separate/
Fixed
>> Debugger API, LLDB value class was monkey-patched to get value type in
>> the same way as GDB value class does and 'cast' method was adjusted to
>> accept also type object, not only string.
> I would prefer to keep the `cast()` API to be used only for the string
> as the first parameter, since it is very helpful for reading of the
> code.
>
> May I suggest the little bit different approach here?
>
> Introduce the `dbg.cast_typeof()` API to have the following semantics:
>
> | dbg.cast_typeof(obj_with_type, value)
>
> The idea is the same, but it doesn't allow getting and working with
> internal types somehow in the debugger extension.
Done
> I'll mention this in the cast usages below.
>
>> Other adjustments:
>> - [lldb] 'eval' adjusted to return object of the same type as 'cast'
>>    method (this improves consistency)
>> - [gdb] in 'eval' method dropped check of the value returned by
>>    gdb.parse_and_eval() as it would fail also for expression like '0'
>>    (looks like a kind of legacy code that is not needed now).
>> - [gdb/lldb] renamed eval argument to reflect its meaning (it is
>>    an expression rather than a command).
>>
>> Closes tarantool/tarantool#13094
> Nit: s/Closes/Resolves/
> Since technically speaking it will be closed after LuaJIT's bump in
> Tarantool.
Adjusted
>> ---
>> This patch is to be applied after the 'fix mapping of FPMATHOP' patch.
>>
>> Branch: https://github.com/tarantool/luajit/tree/elhimov/gh-13094-dbg-use-enums-from-inferior
>> Related issue: https://github.com/tarantool/tarantool/issues/13094
>>
>>   src/luajit_dbg.py | 628 +++++++++++-----------------------------------
>>   1 file changed, 141 insertions(+), 487 deletions(-)
>>
>> diff --git a/src/luajit_dbg.py b/src/luajit_dbg.py
>> index 76001b7d..1ac1d275 100644
>> --- a/src/luajit_dbg.py
>> +++ b/src/luajit_dbg.py
>> @@ -110,9 +110,21 @@ class Debugger(object):
>>                   self.write('{} command initialized\n'.format(name))
>>               self.write('LuaJIT debug extension is successfully loaded\n')
>>   
>> +    def cast(self, tp, val):
>> +        '''Cast the value to the given type (it is either C type string
>> +        or the debugger-specific type object).'''
>> +        if isinstance(tp, str):
>> +            tp = self._dbgtype(tp)
> I would prefer to avoid casting to the given debugger-specific type.
> It is better to introduce `cast_typeof()` method to be used with another
> object given as the first argument (it should be read approximately as
> `cast(typeof(x), y))`.
>
> | def cast_typeof(self, typed_object, val):
> |     '''Cast the value to the type of the given object.
> |     It is used mostly for casting when the string name of the type isn't
> |     accessible (for example, anonymous enums).'''
>
>> +        return self._cast(tp, val)
>> +
>> +    @abc.abstractmethod
>> +    def _cast(self, tp, val):
>> +        '''Cast the value to the debugger-specific type.'''
>> +        pass
>> +
>>       @abc.abstractmethod
>> -    def cast(self, typestr, val):
>> -        '''Cast the value to the required C type.'''
>> +    def _dbgtype(self, typestr):
>> +        '''Convert C type string into debugger-specific type object.'''
>>           pass
> I suppose since it is internal for each debugger, this should not be
> declared as the required type in the parent class.
Removed after switching to the `cast_typeof` approach
>>   
>>       @abc.abstractmethod
>> @@ -146,8 +158,9 @@ class Debugger(object):
>>           pass
>>   
>>       @abc.abstractmethod
>> -    def eval(self, command):
>> -        '''Parse and evaluate the given debugger command.'''
>> +    def eval(self, expr):
>> +        '''Parse and evaluate the given debugger expression.
>> +        Return debugger-specific value.'''
> Nit: This clarification looks unrelated to the patch itself.
> It is better to move it to the separate commit if you really want it.
Removed
>>           pass
>>   
>>       @abc.abstractmethod
>> @@ -187,6 +200,13 @@ class Debugger(object):
>>           '''Register the command with the corresponding name.'''
>>           pass
>>   
> I suggest adding some comments with motivations about such a specific
> method (mostly described in the first paragraph in the commit message).
Done
>> +    @abc.abstractmethod
>> +    def create_enum_value(self, enum_name, enum_member_name):
>> +        '''Return debugger-specific value object that represents
>> +        the given enum member.
>> +        '''
>> +        pass
>> +
>>       @abc.abstractproperty
>>       def LJBase(self):
>>           '''Base command class.
>> @@ -211,8 +231,9 @@ class _GDBDebugger(Debugger):
>>           super(_GDBDebugger, self).__init__()
>>           self.CONNECTED = False
>>   
>> -    def cast(self, typestr, val):
>> -        return gdb.Value(val).cast(self._dbgtype(typestr))
>> +    def _cast(self, tp, val):
>> +        assert isinstance(tp, gdb.Type)
>> +        return gdb.Value(val).cast(tp)
>>   
>>       def sizeof(self, typestr):
>>           return self._dbgtype(typestr).sizeof
>> @@ -268,14 +289,11 @@ class _GDBDebugger(Debugger):
>>           else:
>>               return None
>>   
>> -    def eval(self, command):
>> -        if not command:
>> +    def eval(self, expr):
>> +        if not expr:
>>               return None
>>   
>> -        ret = gdb.parse_and_eval(command)
>> -        if not ret:
>> -            raise gdb.GdbError('table argument empty')
> Hmm, indeed, I can't find an example when this returns something not
> evaluated as `True`...
> Probably it may be removed. But it should be done in the separate clean
> up patch since it is unrelated to the refactoring of the enums.

It has to be removed in this patch because it does affect all the enums 
as '0' is treated as `False`. I've adjusted commit description to 
clarify why this change is applied.

>> -        return ret
>> +        return gdb.parse_and_eval(expr)
>>   
>>       def detect_arch(self):
>>           if hasattr(self, 'arch'):
>> @@ -340,6 +358,9 @@ class _GDBDebugger(Debugger):
>>       def register_command(self, command, name):
>>           command(name)
>>   
>> +    def create_enum_value(self, enum_name, enum_member_name):
>> +        return self.eval(enum_member_name)
>> +
>>       class LJBase(gdb and gdb.Command or object):
>>           def __init__(ljbase, name):
>>               # XXX Fragile: Though the command initialization looks
>> @@ -378,6 +399,10 @@ class _LLDBDebugger(Debugger):
>>               lldb.eBasicTypeInt128
>>           ]
>>   
>> +    def _lldb_tp_isenum(self, tp):
>> +        return tp.GetCanonicalType().GetTypeClass() == \
>> +            lldb.eTypeClassEnumeration
>> +
>>       def _lldb_value_from_raw(self, raw_value, size, tp):
>>           isfp = self._lldb_tp_isfp(tp)
>>           if isfp:
>> @@ -491,8 +516,7 @@ class _LLDBDebugger(Debugger):
>>               # Instead of default GetSummary.
>>               if not lldbval.sbvalue.TypeIsPointerType():
>>                   tp = lldbval.sbvalue.GetType()
>> -                is_float = self._lldb_tp_isfp(tp)
>> -                if is_float:
>> +                if self._lldb_tp_isfp(tp) or self._lldb_tp_isenum(tp):
>>                       return lldbval.sbvalue.GetValue()
>>                   else:
>>                       return str(int(lldbval))
>> @@ -530,6 +554,9 @@ class _LLDBDebugger(Debugger):
>>               else:
>>                   return int(lldbval) - int(other)
>>   
>> +        def lldb_gettype(lldbval):
>> +            return lldbval.sbvalue.type
>> +
> I prefer to drop this method in favor of `cast_typeof()`.
Removed after switching to the `cast_typeof` approach
>>           super(_LLDBDebugger, self).__init__()
>>           self.target = lldb.debugger.GetSelectedTarget()
>>           # Monkey-patch the lldb.value class.
>> @@ -545,6 +572,7 @@ class _LLDBDebugger(Debugger):
>>           lldb.value.__ror__ = lldb__or__  # Same semantics.
>>           lldb.value.__str__ = lldb__str__
>>           lldb.value.__sub__ = lldb__sub__
>> +        lldb.value.type = property(lldb_gettype)
> I prefer to drop this field in favor of `cast_typeof()`.
Removed after switching to the `cast_typeof` approach
>>   
>>           def lldb_major_version():
>>               version_string = lldb.SBDebugger.GetVersionString()
>> @@ -568,11 +596,11 @@ class _LLDBDebugger(Debugger):
>>           self.dbgtype_cache[typestr] = dbgtype
>>           return dbgtype
>>   
>> -    def cast(self, typestr, val):
>> +    def _cast(self, tp, val):
>> +        assert isinstance(tp, lldb.SBType)
>>           if isinstance(val, lldb.value):
>>               val = val.sbvalue
>>           elif type(val) is int:
>> -            tp = self._dbgtype(typestr)
>>               return self._lldb_value_from_raw(val, tp.GetByteSize(), tp)
>>           elif not isinstance(val, lldb.SBValue):
>>               raise Exception(
>> @@ -582,7 +610,6 @@ class _LLDBDebugger(Debugger):
>>           # XXX: Simply SBValue.Cast() works incorrectly since it
>>           # may take the 8 bytes of memory instead of 4, before the
>>           # cast. Construct the value on the fly.
>> -        tp = self._dbgtype(typestr)
>>           if self._lldb_tp_isfp(tp):
>>               rawval = float(val.GetValue())
>>           elif self._lldb_tp_issigned(tp):
>> @@ -670,15 +697,15 @@ class _LLDBDebugger(Debugger):
>>           else:
>>               return None
>>   
>> -    def eval(self, command):
>> -        if not command:
>> +    def eval(self, expr):
>> +        if not expr:
>>               return None
>>   
>>           process = self.target.GetProcess()
>>           thread = process.GetSelectedThread()
>>           frame = thread.GetSelectedFrame()
>> -        ret = frame.EvaluateExpression(command)
>> -        return ret
>> +        ret = frame.EvaluateExpression(expr)
>> +        return lldb.value(ret)
> Nit: Lets drop unneeded variable `ret`:
> | return lldb.value(frame.EvaluateExpression(expr))
Done
>>   
>>       def detect_arch(self):
>>           if hasattr(self, 'arch'):
>> @@ -724,6 +751,37 @@ class _LLDBDebugger(Debugger):
>>               )
>>           )
>>   
>> +    def create_enum_value(self, enum_name, enum_member_name):
>> +        val = self.eval(enum_name + "::" + enum_member_name)
> Please add the comment about this eval construction.
Done
>> +        if val.sbvalue.IsValid() and val.sbvalue.error.Success():
> Why do we need to check `val.sbvalue.error.Success()` value?
> Why is `val.sbvalue.IsValid()` not enough?
Added comment
>> +            return val
>> +
>> +        # LLDB uses enum name in expression above but debugging information
> Typo: s/enum/the enum/
> Typo: s/expression/the expression/
Fixed
>> +        # about enum name migth be optimized out if no variable of the given
> Typo: s/enum/the enum/
Fixed
>> +        # enum type is declared and its members are only used as the predefined
>> +        # constants (like IRFieldID).
> Nit: Please, use 66 comment line width.
Done
>> +
>> +        # In this case the above method doesn't work so trying to discover
>> +        # enum type by the given enum member.
> Typo: s/enum/the enum/
Fixed
> Nit: Please, use 66 comment line width.
>
>> +
>> +        def find_enum_type_member(enum_type, enum_member_name):
>> +            # SBTypeEnumMemberList supports members iteration and [] access
>> +            # (both by index and by member name) only starting from lldb-12
>> +            # so this implementation is used to handle earlier versions.
> Nit: Please, use 66 comment line width.
Done
>> +            members = enum_type.GetEnumMembers()
>> +            for i in range(members.GetSize()):
>> +                item = members.GetTypeEnumMemberAtIndex(i)
>> +                if item.name == enum_member_name:
>> +                    return item
>> +            return None
>> +
>> +        for m in self.target.modules:
>> +            for et in m.GetTypes(lldb.eTypeClassEnumeration):
>> +                et_member = find_enum_type_member(et, enum_member_name)
>> +                if et_member is not None:
>> +                    return self.cast(et, et_member.unsigned)
> `self.cast_typeof(et, et_member.unsigned)`
Done
>> +        return None
>> +
>>       class LJBase(object):
>>           # Ignore given parameters by LLDB.
>>           def __init__(ljbase, debugger, unused):
>> @@ -800,6 +858,45 @@ def strx64(val):
>>       return re.sub('L?$', '', hex(int(tou64(val))))
>>   
>>   
>> +class EnumBasedList(object):
>> +    def __init__(self, enum_name, max_enum_member, map_func=None,
>> +                 *map_func_extra_args):
>> +        self.__enum_name = enum_name
>> +        self.__max_enum_member = max_enum_member
>> +        self.__map_func = map_func
>> +        self.__map_func_extra_args = map_func_extra_args
>> +        # Lazy initialization (see __get_items method) as the required
> Nit: Please, use 66 comment line width.
Done
> Side note: I like this approach, it helps to avoid performance issues.
>
>> +        # information might be unavailable at this moment.
>> +        self.__items = None
>> +
>> +    def __iter__(self):
>> +        return iter(self.__get_items())
>> +
>> +    def __getitem__(self, key):
>> +        return self.__get_items()[key]
>> +
>> +    def __len__(self):
>> +        return len(self.__get_items())
>> +
>> +    def __get_items(self):
>> +        if self.__items is None:
>> +            max_enum_value = dbg.create_enum_value(
>> +                self.__enum_name, self.__max_enum_member
>> +            )
>> +            items = []
>> +            for i in range(dbg.cast('int', max_enum_value)):
> Can it be just:
> | for i in range(int(max_enum_value))
> instead?

Looks like forgot to remove some experimental code. It can be even simpler

for i in range(max_enum_value)

>> +                item = str(dbg.cast(max_enum_value.type, dbg.eval(str(i))))
> | item = str(cast_typeof(max_enum_value, dbg.eval(str(i))))
>
> Be aware, that debugger-specific type usage should be part of the
> `cast_typeof()`. Hence, it allows dropping `.type` setting for LLDB.

Dropped `.type` after switching to the `cast_typeof` approach
>> +                if self.__map_func:
>> +                    item = self.__map_func(item, *self.__map_func_extra_args)
>> +                items.append(item)
>> +            self.__items = items
>> +        return self.__items
> <snipped>
>
>> +IRTYPES = EnumBasedList('IRType', 'IRT__MAX', lambda x: {
>> +                            'IRT_LIGHTUD': 'lud',
>> +                            'IRT_CDATA': 'cdt',
>> +                            'IRT_UDATA': 'udt',
>> +                            'IRT_FLOAT': 'flt',
>> +                            'IRT_SOFTFP': 'sfp',
> Minor: since this is not the full spectrum of values, lets sort them
> alphabetically.
Done
>> +                        }.get(x, cut_prefix(x, 'IRT_')[:3].ljust(3).lower()))
> <snipped>
>
>> -- 
>> 2.43.0
>>
-- 
Best regards,
Mikhail Elhimov