Re: [Tarantool-patches] [PATCH luajit] luajit-gdb: support dualnum mode

["Максим Корякшин via Tarantool-patches" <tarantool-patches@dev.tarantool.org>]

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


Hi, Sergey!
Thanks for the comments, here is the new commit message:
=======================================================
luajit-gdb: support dualnum mode
 
luajit-gdb.py displays integers in LJ_DUALNUM mode as nan-s. The
dumper function produces output thinking of any input value as of a
double. However, in DUALNUM mode, integers and doubles are stored
differently, so the `itype` of a float number must be less than
`LJ_TISNUM`, and the `itype` of an integer must be `LJ_TISNUM`. With
this fact in mind, we can easily differentiate one from another.
 
Closes tarantool/tarantool#6224
=======================================================
 
And here is the diff:
=======================================================
diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
index 5f79c277..d4882dd7 100644
--- a/src/luajit-gdb.py
+++ b/src/luajit-gdb.py
@@ -238,8 +238,11 @@ def jit_state(g):
         0x15: 'ERR',
     }.get(int(J(g)['state']), 'INVALID')
+def tvisint(o):
+    return LJ_DUALNUM and itype(o) == LJ_TISNUM
+
 def tvisnumber(o):
-    return itype(o) <= (0xfffeffff if LJ_64 and not LJ_GC64 else LJ_T['NUMX'])
+    return itype(o) <= LJ_TISNUM
 def tvislightud(o):
     if LJ_64 and not LJ_GC64:
@@ -343,9 +346,8 @@ def dump_lj_tudata(tv):
     return 'userdata @ {}'.format(strx64(gcval(tv['gcr'])))
 def dump_lj_tnumx(tv):
-    if itype(tv) == (0xfffeffff if LJ_64 and not LJ_GC64 else LJ_T['NUMX']):
-        integer = cast('int32_t', cast('uint64_t', cast('void*', tv['n'])) & 0xFFFFFFFF)
-        return 'number {}'.format(integer)
+    if tvisint(tv):
+        return 'number {}'.format(cast('int32_t', tv['i']))
     else:
         return 'number {}'.format(cast('double', tv['n']))
@@ -687,7 +689,7 @@ The command requires no args and dumps current GC stats:
         ))
 def init(commands):
-    global LJ_64, LJ_GC64, LJ_FR2, PADDING
+    global LJ_64, LJ_GC64, LJ_DUALNUM, LJ_TISNUM, LJ_FR2, PADDING
     # XXX Fragile: though connecting the callback looks like a crap but it
     # respects both Python 2 and Python 3 (see #4828).
@@ -728,6 +730,7 @@ def init(commands):
     try:
         LJ_64 = str(gdb.parse_and_eval('IRT_PTR')) == 'IRT_P64'
         LJ_FR2 = LJ_GC64 = str(gdb.parse_and_eval('IRT_PGC')) == 'IRT_P64'
+        LJ_DUALNUM = lookup('lj_lib_checknumber') is not None
     except:
         gdb.write('luajit-gdb.py failed to load: '
                   'no debugging symbols found for libluajit\n')
@@ -737,6 +740,7 @@ def init(commands):
         command(name)
     PADDING = ' ' * len(':' + hex((1 << (47 if LJ_GC64 else 32)) - 1))
+    LJ_TISNUM = 0xfffeffff if LJ_64 and not LJ_GC64 else LJ_T['NUMX']
     gdb.write('luajit-gdb.py is successfully loaded\n')
=======================================================
  
>Среда, 11 августа 2021, 11:28 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
> 
>Hi, Maxim!
>
>Thanks for the patch!
>
>Please consider my comments below.
>
>Side note: First of all, I'm very disappointed, that there this patch
>[1] isn't merged (in any form) into gdb. Those work with the expanding
>of macros is very helpful...
>
>On 31.07.21, Maxim Kokryashkin wrote:
>> For x86/x64 LJ_DUALNUM mode is disabled. But for some other arches
>
>Nit: It can be enabled by corresponding configuration options for
>x86/x64, too, IINM. So I suggest to drop the first and the second
>sentence.
>
>> (arm or arm64, for example) it is enabled by default. luajit-gdb.py
>> displays integers in LJ_DUALNUM mode as nan-s.
>>
>
>Nit: This paragraph may be joined to the next after suggesting changes.
>
>> As it turned out, luajit-gdb detects those integers as integers, but
>> there was a problem with the dumper function itself.
>
>Nit: The next sentence is about the problem with the dumping function, so
>I suggest to drop this opening sentence:)
>Feel free to ignore.
>
>> The dumper
>> function produces output thinking of any input value as of a double.
>> However, in DUALNUM mode, integers and floats are stored differently,
>
>Typo: s/floats/doubles/
>Here and below.
>
>> so the `itype` of a float number must be less than `LJ_TISNUM`, and the
>> `itype` of an integer must be `LJ_TISNUM`. With this fact in mind, we
>> can easily differentiate one from another.
>>
>> But in any mode, lua numbers are stored as doubles on the C side, so it
>
>Typo: s/lua/Lua/
>
>Do you mean LuaJIT here? Because this is not true for LuaJIT. See
><lj_obj.h> for details.
>
>> takes an ugly cast chain on the Python side to perform the some sort of
>> the `reinterpret_cast` because the gdb module doesn't have any
>> mechanism to get the address of a symbol.
>
>This sentence isn't clear to me. What symbol do you mean?
>
>>
>> Closes tarantool/tarantool#6224
>
>Side note: You can say that it really closes the issue, because we use
>luajit-gdb from this fork. OTOH, maybe one uses it as a part of
>third_party from Tarantool repo.
>"Closes" is good to me, but I'm not sure what is idiomatically correct.
>
>> ---
>> Github branch:  https://github.com/tarantool/luajit/tree/fckxorg/gh-6224-support-dulanum
>> Issue:  https://github.com/tarantool/tarantool/issues/6224
>> For more info, see line 273 in lj_obj.h
>>
>> src/luajit-gdb.py | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
>> index c50405ad..5f79c277 100644
>> --- a/src/luajit-gdb.py
>> +++ b/src/luajit-gdb.py
>> @@ -343,7 +343,11 @@ def dump_lj_tudata(tv):
>> return 'userdata @ {}'.format(strx64(gcval(tv['gcr'])))
>>
>> def dump_lj_tnumx(tv):
>> - return 'number {}'.format(cast('double', tv['n']))
>> + if itype(tv) == (0xfffeffff if LJ_64 and not LJ_GC64 else LJ_T['NUMX']):
>
>This is true only in LJ_DUALNUM mode. So I suggest we can add another
>one global constant for this: LJ_DUALNUM. We can set it via hack with
>lookuping symbol `lj_lib_checknumber` -- it is compiled only for
>LJ_DUALNUM mode for now. So, if a result of lookup() isn't None we are
>in LJ_DUALNUM mode.
>
>Also, I suggest to create another global constant for LJ_TISNUM, like it
>is done for PADDING constant.
>
>I suppose we want to do something similar to tvisint() macro for this check:
>| LJ_DUALNUM && itype(tv) == LJ_TISNUM
>
>> + integer = cast('int32_t', cast('uint64_t', cast('void*', tv['n'])) & 0xFFFFFFFF)
>
>I don't get this cast to uint64_t and mask. Why can't we just take
>`(int32_t)(o)->i` value, like it is done for `intV()` macro?
>
>> + return 'number {}'.format(integer)
>
>Nit: I suppose, it is better to highlight the fact that TValue stores
>integer here with corresponding return. It may be helpful for debugging
>some issues, related to storing type, I suppose.
>
>But I'm not so sure at this point. Wait for Igor's opinion.
>
>> + else:
>> + return 'number {}'.format(cast('double', tv['n']))
>>
>> def dump_lj_invalid(tv):
>> return 'not valid type @ {}'.format(strx64(gcval(tv['gcr'])))
>> --
>> 2.32.0
>>
>
>[1]:  https://sourceware.org/legacy-ml/gdb-patches/2011-08/msg00441.html
>
>--
>Best regards,
>Sergey Kaplun
 

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