Tarantool development patches archive
 help / color / mirror / Atom feed
From: "Максим Корякшин via Tarantool-patches" <tarantool-patches@dev.tarantool.org>
To: "Sergey Kaplun" <skaplun@tarantool.org>
Cc: "Maxim Kokryashkin" <max.kokryashkin@gmail.com>,
Subject: Re: [Tarantool-patches]  [PATCH luajit] luajit-gdb: support dualnum mode
Date: Fri, 13 Aug 2021 17:29:27 +0300	[thread overview]
Message-ID: <1628864967.181226130@f723.i.mail.ru> (raw)
In-Reply-To: <YROJ6jRfGaiOZzD3@root>

[-- 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']))
         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
     # 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):
         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
         gdb.write('luajit-gdb.py failed to load: '
                   'no debugging symbols found for libluajit\n')
@@ -737,6 +740,7 @@ def init(commands):
     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
>> (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 --]

  reply	other threads:[~2021-08-13 14:29 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-31 13:36 Maxim Kokryashkin via Tarantool-patches
2021-08-11  8:27 ` Sergey Kaplun via Tarantool-patches
2021-08-13 14:29   ` Максим Корякшин via Tarantool-patches [this message]
2021-08-14 11:38     ` [Tarantool-patches] [PATCH luajit v2] " Maxim Kokryashkin via Tarantool-patches
2021-08-14 13:36       ` Максим Корякшин via Tarantool-patches
2021-08-14 13:47       ` Sergey Kaplun via Tarantool-patches
2021-08-14 17:26         ` Максим Корякшин via Tarantool-patches
2021-08-17  7:56         ` Igor Munkin via Tarantool-patches
2021-08-16  8:49       ` Igor Munkin via Tarantool-patches
2021-08-16 10:01         ` Максим Корякшин via Tarantool-patches
2021-08-16 16:23           ` Vitaliia Ioffe via Tarantool-patches
2021-08-17  7:47           ` Igor Munkin via Tarantool-patches
2021-08-17  9:23 ` [Tarantool-patches] [PATCH luajit] " Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1628864967.181226130@f723.i.mail.ru \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=m.kokryashkin@tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches]  [PATCH luajit] luajit-gdb: support dualnum mode' \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox