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

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

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/

> with various version of luajit because different version might use

Typo: s/version/versions/g

> different set of enum members (newer version might introduce additional

Typo: s/set/sets/
Typo: s/newer/a newer/

> BC/IR/etc.)
> 
> Prior to this patch string was used as a debugger-agnostic way to

Typo: s/this/this, the/

> specify type, but this way might not work in case of enum because

Typo: s/case of enum/the case of an enum/

> debugging information might be optimized out if no variable of such enum

Typo: s/enum/an enum/

> type is declared and its members are used only as a predefined

Typo: s/a //

> constants. The type of enum is needed to be able to get human readable

Typo: s/human readable/human-readable/

> 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/

> 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/

> 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.

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.

> ---
> 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.

>  
>      @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.

>          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).

> +    @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.

> -        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()`.

>          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()`.

>  
>          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))

>  
>      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.

> +        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?

> +            return val
> +
> +        # LLDB uses enum name in expression above but debugging information

Typo: s/enum/the enum/
Typo: s/expression/the expression/

> +        # about enum name migth be optimized out if no variable of the given

Typo: s/enum/the enum/

> +        # enum type is declared and its members are only used as the predefined
> +        # constants (like IRFieldID).

Nit: Please, use 66 comment line width.

> +
> +        # 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/

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.

> +            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)`

> +        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.

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?

> +                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.

> +                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.

> +                        }.get(x, cut_prefix(x, 'IRT_')[:3].ljust(3).lower()))

<snipped>

> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun