Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] luajit-gdb: support dualnum mode
@ 2021-07-31 13:36 Maxim Kokryashkin via Tarantool-patches
  2021-08-11  8:27 ` Sergey Kaplun via Tarantool-patches
  2021-08-17  9:23 ` [Tarantool-patches] [PATCH luajit] " Igor Munkin via Tarantool-patches
  0 siblings, 2 replies; 13+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2021-07-31 13:36 UTC (permalink / raw)
  To: tarantool-patches, imun, skaplun

For x86/x64 LJ_DUALNUM mode is disabled. But for some other arches
(arm or arm64, for example) it is enabled by default. luajit-gdb.py
displays integers in LJ_DUALNUM mode as nan-s.

As it turned out, luajit-gdb detects those integers as integers, but
there was a problem with the dumper function itself. The dumper
function produces output thinking of any input value as of a double.
However, in DUALNUM mode, integers and floats 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.

But in any mode, lua numbers are stored as doubles on the C side, so it
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.

Closes tarantool/tarantool#6224
---
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']):
+        integer = cast('int32_t', cast('uint64_t', cast('void*', tv['n'])) & 0xFFFFFFFF)
+        return 'number {}'.format(integer)
+    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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] luajit-gdb: support dualnum mode
  2021-07-31 13:36 [Tarantool-patches] [PATCH luajit] luajit-gdb: support dualnum mode Maxim Kokryashkin via Tarantool-patches
@ 2021-08-11  8:27 ` Sergey Kaplun via Tarantool-patches
  2021-08-13 14:29   ` Максим Корякшин via Tarantool-patches
  2021-08-17  9:23 ` [Tarantool-patches] [PATCH luajit] " Igor Munkin via Tarantool-patches
  1 sibling, 1 reply; 13+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-08-11  8:27 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: tarantool-patches

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches]  [PATCH luajit] luajit-gdb: support dualnum mode
  2021-08-11  8:27 ` Sergey Kaplun via Tarantool-patches
@ 2021-08-13 14:29   ` Максим Корякшин via Tarantool-patches
  2021-08-14 11:38     ` [Tarantool-patches] [PATCH luajit v2] " Maxim Kokryashkin via Tarantool-patches
  0 siblings, 1 reply; 13+ messages in thread
From: Максим Корякшин via Tarantool-patches @ 2021-08-13 14:29 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: Maxim Kokryashkin, tarantool-patches

[-- 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 --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Tarantool-patches] [PATCH luajit v2] luajit-gdb: support dualnum mode
  2021-08-13 14:29   ` Максим Корякшин via Tarantool-patches
@ 2021-08-14 11:38     ` Maxim Kokryashkin via Tarantool-patches
  2021-08-14 13:36       ` Максим Корякшин via Tarantool-patches
                         ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2021-08-14 11:38 UTC (permalink / raw)
  To: tarantool-patches, imun, skaplun

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
---
Changes in v2:
  - Fixed comments as per review by Sergey

 src/luajit-gdb.py | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
index c50405ad..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,7 +346,10 @@ 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 tvisint(tv):
+        return 'number {}'.format(cast('int32_t', tv['i']))
+    else:
+        return 'number {}'.format(cast('double', tv['n']))
 
 def dump_lj_invalid(tv):
     return 'not valid type @ {}'.format(strx64(gcval(tv['gcr'])))
@@ -683,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).
@@ -724,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')
@@ -733,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')
 
-- 
2.32.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches]  [PATCH luajit v2] luajit-gdb: support dualnum mode
  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-16  8:49       ` Igor Munkin via Tarantool-patches
  2 siblings, 0 replies; 13+ messages in thread
From: Максим Корякшин via Tarantool-patches @ 2021-08-14 13:36 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: tarantool-patches

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


While I was working on another luajit-gdb enhancement,
I noticed that I forgot to add LJ_DUALNUM and LJ_TISNUM
declarations. So here is the diff that fixes it:
===========================================
diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
index d4882dd7..b656a808 100644
--- a/src/luajit-gdb.py
+++ b/src/luajit-gdb.py
@@ -157,9 +157,10 @@ def frame_prev(framelink):
 LJ_64 = None
 LJ_GC64 = None
 LJ_FR2 = None
+LJ_DUALNUM = None
 LJ_GCVMASK = ((1 << 47) - 1)
-
+LJ_TISNUM = None
 PADDING = None
 # }}}
===========================================
  
>Суббота, 14 августа 2021, 14:39 +03:00 от Maxim Kokryashkin <max.kokryashkin@gmail.com>:
> 
>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
>---
>Changes in v2:
>  - Fixed comments as per review by Sergey
>
> src/luajit-gdb.py | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
>diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
>index c50405ad..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,7 +346,10 @@ 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 tvisint(tv):
>+ return 'number {}'.format(cast('int32_t', tv['i']))
>+ else:
>+ return 'number {}'.format(cast('double', tv['n']))
> 
> def dump_lj_invalid(tv):
>     return 'not valid type @ {}'.format(strx64(gcval(tv['gcr'])))
>@@ -683,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).
>@@ -724,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')
>@@ -733,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')
> 
>--
>2.32.0
 

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit v2] luajit-gdb: support dualnum mode
  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
  2 siblings, 2 replies; 13+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-08-14 13:47 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: tarantool-patches

Hi, Maxim!

Thanks for the fixes!

LGTM, except two nitpicks below.

On 14.08.21, Maxim Kokryashkin wrote:
> 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
> ---
> Changes in v2:
>   - Fixed comments as per review by Sergey
> 
>  src/luajit-gdb.py | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
> index c50405ad..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

Strictly saying, it is `<`, because `==` is staying for integer
representation.

>  
>  def tvislightud(o):
>      if LJ_64 and not LJ_GC64:
> @@ -343,7 +346,10 @@ 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 tvisint(tv):
> +        return 'number {}'.format(cast('int32_t', tv['i']))

Nit: Lets change it to 'integer {}', as far as it is an integer value in
the internal LuaJIT representation.

> +    else:
> +        return 'number {}'.format(cast('double', tv['n']))
>  
>  def dump_lj_invalid(tv):
>      return 'not valid type @ {}'.format(strx64(gcval(tv['gcr'])))
> @@ -683,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).
> @@ -724,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')
> @@ -733,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')
>  
> -- 
> 2.32.0
> 

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches]  [PATCH luajit v2] luajit-gdb: support dualnum mode
  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
  1 sibling, 0 replies; 13+ messages in thread
From: Максим Корякшин via Tarantool-patches @ 2021-08-14 17:26 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: Maxim Kokryashkin, tarantool-patches

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


Hi, Sergey!
Thanks for the comments!
 
Here is the fix:
=========================================
diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
index b656a808..9ccca66a 100644
--- a/src/luajit-gdb.py
+++ b/src/luajit-gdb.py
@@ -243,7 +243,7 @@ def tvisint(o):
     return LJ_DUALNUM and itype(o) == LJ_TISNUM
 def tvisnumber(o):
-    return itype(o) <= LJ_TISNUM
+    return itype(o) < LJ_TISNUM
 def tvislightud(o):
     if LJ_64 and not LJ_GC64:
@@ -348,7 +348,7 @@ def dump_lj_tudata(tv):
 def dump_lj_tnumx(tv):
     if tvisint(tv):
-        return 'number {}'.format(cast('int32_t', tv['i']))
+        return 'integer {}'.format(cast('int32_t', tv['i']))
     else:
         return 'number {}'.format(cast('double', tv['n']))
=========================================
  
>Суббота, 14 августа 2021, 16:48 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
> 
>Hi, Maxim!
>
>Thanks for the fixes!
>
>LGTM, except two nitpicks below.
>
>On 14.08.21, Maxim Kokryashkin wrote:
>> 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
>> ---
>> Changes in v2:
>> - Fixed comments as per review by Sergey
>>
>> src/luajit-gdb.py | 14 +++++++++++---
>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
>> index c50405ad..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
>
>Strictly saying, it is `<`, because `==` is staying for integer
>representation.
>>
>> def tvislightud(o):
>> if LJ_64 and not LJ_GC64:
>> @@ -343,7 +346,10 @@ 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 tvisint(tv):
>> + return 'number {}'.format(cast('int32_t', tv['i']))
>
>Nit: Lets change it to 'integer {}', as far as it is an integer value in
>the internal LuaJIT representation.
>
>> + else:
>> + return 'number {}'.format(cast('double', tv['n']))
>>
>> def dump_lj_invalid(tv):
>> return 'not valid type @ {}'.format(strx64(gcval(tv['gcr'])))
>> @@ -683,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).
>> @@ -724,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')
>> @@ -733,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')
>>
>> --
>> 2.32.0
>>
>
>--
>Best regards,
>Sergey Kaplun
 

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit v2] luajit-gdb: support dualnum mode
  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-16  8:49       ` Igor Munkin via Tarantool-patches
  2021-08-16 10:01         ` Максим Корякшин via Tarantool-patches
  2 siblings, 1 reply; 13+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-08-16  8:49 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: tarantool-patches

Max,

Thanks for the patch! Please consider the comments below.

At first, please use 'gdb:' instead of 'luajit-gdb:' like for the others
patches. I would also use 'LJ_DUALNUM' instead of the plain 'dualnum'.

On 14.08.21, Maxim Kokryashkin wrote:
> 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

Minor: s/thinking of/considering/, but feel free to ignore.

> double. However, in DUALNUM mode, integers and doubles are stored

Please, be consistent in mode name: choose one from LJ_DUALNUM, DUALNUM
or dualnum and use it patch-wide.

> differently, so the `itype` of a float number must be less than

Minor: It's either double (for the storage type) or floating point.

> `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
> ---
> Changes in v2:
>   - Fixed comments as per review by Sergey
> 
>  src/luajit-gdb.py | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
> index c50405ad..d4882dd7 100644
> --- a/src/luajit-gdb.py
> +++ b/src/luajit-gdb.py

<snipped>

>  def tvislightud(o):
>      if LJ_64 and not LJ_GC64:
> @@ -343,7 +346,10 @@ 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 tvisint(tv):
> +        return 'number {}'.format(cast('int32_t', tv['i']))

Side note: Agree with Sergey here. It is more convenient to understand
that TValue structure for integer slot differs from double.

> +    else:
> +        return 'number {}'.format(cast('double', tv['n']))
>  
>  def dump_lj_invalid(tv):
>      return 'not valid type @ {}'.format(strx64(gcval(tv['gcr'])))
> @@ -683,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

Minor: Why did you split LJ_GC64 and LJ_FR2. I understand you want to
group all LJ_* entries together, so you didn't just append the new ones,
but please move the new LJ_* entries after the old ones.

>  
>      # XXX Fragile: though connecting the callback looks like a crap but it
>      # respects both Python 2 and Python 3 (see #4828).
> @@ -724,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

Ouch... Not such elegant as for other defines, but I see we have no
choice here... Anyway, this doesn't work for the following scenario:
| # gdb --args ./luajit -e 'print(1)'
| <snipped>
| (gdb) source luajit-gdb.py
| luajit-gdb.py failed to load: no debugging symbols found for libluajit

However, the next case is fine:
| # gdb --args ./luajit -e 'print(1)'
| <snipped>
| (gdb) b lj_cf_print
| Breakpoint 1 at 0x28350: file /luajit/src/lib_base.c, line 486.
| (gdb) r
| Starting program: /luajit/src/luajit -e print\(1\)
|
| Breakpoint 1, lj_cf_print (L=0x7fb7d57378) at /luajit/src/lib_base.c:486
| 486       ptrdiff_t i, nargs = L->top - L->base;
| (gdb) source luajit-gdb.py
| lj-arch command initialized
| lj-tv command initialized
| lj-str command initialized
| lj-tab command initialized
| lj-stack command initialized
| lj-state command initialized
| lj-gc command initialized
| luajit-gdb.py is successfully loaded

The problem is that you use <gdb.lookup_symbol> that requires a frame,
but there is none, until the program is run. The function you need for
your case is <gdb.lookup_global_symbol>. Consider the diff below:
| diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
| index 9ccca66a..4dc8c844 100644
| --- a/src/luajit-gdb.py
| +++ b/src/luajit-gdb.py
| @@ -731,7 +731,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
| +        LJ_DUALNUM = gdb.lookup_global_symbol('lj_lib_checknumber') is not None
|      except:
|          gdb.write('luajit-gdb.py failed to load: '
|                    'no debugging symbols found for libluajit\n')

As a result, the extension loads fine even before the program is started:
| # gdb --args ./luajit -e 'print(1)'
| <snipped>
| (gdb) source luajit-gdb.py
| lj-arch command initialized
| lj-tv command initialized
| lj-str command initialized
| lj-tab command initialized
| lj-stack command initialized
| lj-state command initialized
| lj-gc command initialized
| luajit-gdb.py is successfully loaded

BTW, it's worth to extend <lj-arch> output with LJ_DUALNUM value.

>      except:
>          gdb.write('luajit-gdb.py failed to load: '
>                    'no debugging symbols found for libluajit\n')

<snipped>

> -- 
> 2.32.0
> 

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches]  [PATCH luajit v2] luajit-gdb: support dualnum mode
  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
  0 siblings, 2 replies; 13+ messages in thread
From: Максим Корякшин via Tarantool-patches @ 2021-08-16 10:01 UTC (permalink / raw)
  To: Igor Munkin; +Cc: tarantool-patches

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


Hello, Igor!
Thanks for your comments!
 
Here is the new commit message:
=========================================================
gdb: support LJ_DUALNUM mode
 
luajit-gdb.py displays integers in LJ_DUALNUM mode as nan-s. The
dumper function produces output considering any input value as a
double. However, in LJ_DUALNUM mode, integers and doubles are stored
differently, so the `itype` of a double 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
=========================================================
 
Here is the diff:
=========================================================
diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
index 9ccca66a..25428745 100644
--- a/src/luajit-gdb.py
+++ b/src/luajit-gdb.py
@@ -507,10 +507,14 @@ pointers respectively.
     '''
     def invoke(self, arg, from_tty):
-        gdb.write('LJ_64: {LJ_64}, LJ_GC64: {LJ_GC64}\n'.format(
-            LJ_64 = LJ_64,
-            LJ_GC64 = LJ_GC64
-        ))
+        gdb.write(
+            'LJ_64: {LJ_64}, LJ_GC64: {LJ_GC64}, LJ_DUALNUM : {LJ_DUALNUM}\n'
+            .format(
+                LJ_64 = LJ_64,
+                LJ_GC64 = LJ_GC64,
+                LJ_DUALNUM = LJ_DUALNUM
+            )
+        )
 class LJDumpTValue(LJBase):
     '''
@@ -690,7 +694,7 @@ The command requires no args and dumps current GC stats:
         ))
 def init(commands):
-    global LJ_64, LJ_GC64, LJ_DUALNUM, LJ_TISNUM, LJ_FR2, PADDING
+    global LJ_64, LJ_GC64, LJ_FR2, LJ_DUALNUM, LJ_TISNUM, PADDING
     # XXX Fragile: though connecting the callback looks like a crap but it
     # respects both Python 2 and Python 3 (see #4828).
@@ -731,7 +735,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
+        LJ_DUALNUM = gdb.lookup_global_symbol('lj_lib_checknumber') is not None
     except:
         gdb.write('luajit-gdb.py failed to load: '
                   'no debugging symbols found for libluajit\n')
=========================================================
 
> 
>>Max,
>>
>>Thanks for the patch! Please consider the comments below.
><snipped>
>>> def tvislightud(o):
>>> if LJ_64 and not LJ_GC64:
>>> @@ -343,7 +346,10 @@ 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 tvisint(tv):
>>> + return 'number {}'.format(cast('int32_t', tv['i']))
>>
>>Side note: Agree with Sergey here. It is more convenient to understand
>>that TValue structure for integer slot differs from double.
>I have already fixed the output format here with the fix for v2 of the patch.
><snipped>
> 
>Best regards,
>Maxim Kokryashkin
> 

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches]  [PATCH luajit v2] luajit-gdb: support dualnum mode
  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
  1 sibling, 0 replies; 13+ messages in thread
From: Vitaliia Ioffe via Tarantool-patches @ 2021-08-16 16:23 UTC (permalink / raw)
  To: Максим
	Корякшин
  Cc: tarantool-patches

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


QA LGTM
 
 
--
Vitaliia Ioffe
 
  
>Понедельник, 16 августа 2021, 13:02 +03:00 от Максим Корякшин via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
> 
>Hello, Igor!
>Thanks for your comments!
> 
>Here is the new commit message:
>=========================================================
>gdb: support LJ_DUALNUM mode
> 
>luajit-gdb.py displays integers in LJ_DUALNUM mode as nan-s. The
>dumper function produces output considering any input value as a
>double. However, in LJ_DUALNUM mode, integers and doubles are stored
>differently, so the `itype` of a double 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
>=========================================================
> 
>Here is the diff:
>=========================================================
>diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
>index 9ccca66a..25428745 100644
>--- a/src/luajit-gdb.py
>+++ b/src/luajit-gdb.py
>@@ -507,10 +507,14 @@ pointers respectively.
>     '''
>     def invoke(self, arg, from_tty):
>-        gdb.write('LJ_64: {LJ_64}, LJ_GC64: {LJ_GC64}\n'.format(
>-            LJ_64 = LJ_64,
>-            LJ_GC64 = LJ_GC64
>-        ))
>+        gdb.write(
>+            'LJ_64: {LJ_64}, LJ_GC64: {LJ_GC64}, LJ_DUALNUM : {LJ_DUALNUM}\n'
>+            .format(
>+                LJ_64 = LJ_64,
>+                LJ_GC64 = LJ_GC64,
>+                LJ_DUALNUM = LJ_DUALNUM
>+            )
>+        )
> class LJDumpTValue(LJBase):
>     '''
>@@ -690,7 +694,7 @@ The command requires no args and dumps current GC stats:
>         ))
> def init(commands):
>-    global LJ_64, LJ_GC64, LJ_DUALNUM, LJ_TISNUM, LJ_FR2, PADDING
>+    global LJ_64, LJ_GC64, LJ_FR2, LJ_DUALNUM, LJ_TISNUM, PADDING
>     # XXX Fragile: though connecting the callback looks like a crap but it
>     # respects both Python 2 and Python 3 (see #4828).
>@@ -731,7 +735,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
>+        LJ_DUALNUM = gdb.lookup_global_symbol('lj_lib_checknumber') is not None
>     except:
>         gdb.write('luajit-gdb.py failed to load: '
>                   'no debugging symbols found for libluajit\n')
>=========================================================
> 
>> 
>>>Max,
>>>
>>>Thanks for the patch! Please consider the comments below.
>><snipped>
>>>> def tvislightud(o):
>>>> if LJ_64 and not LJ_GC64:
>>>> @@ -343,7 +346,10 @@ 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 tvisint(tv):
>>>> + return 'number {}'.format(cast('int32_t', tv['i']))
>>>
>>>Side note: Agree with Sergey here. It is more convenient to understand
>>>that TValue structure for integer slot differs from double.
>>I have already fixed the output format here with the fix for v2 of the patch.
>><snipped>
>> 
>>Best regards,
>>Maxim Kokryashkin
>> 
 

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit v2] luajit-gdb: support dualnum mode
  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
  1 sibling, 0 replies; 13+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-08-17  7:47 UTC (permalink / raw)
  To: Максим
	Корякшин
  Cc: tarantool-patches

Max,

Thanks for the fixes! LGTM now. BTW, I've reverted one fix proposed by
Sergey (see the corresponding thread) and fixed a typo below.

On 16.08.21, Максим Корякшин wrote:
> 
> Hello, Igor!
> Thanks for your comments!
>  
> Here is the new commit message:
> =========================================================
> gdb: support LJ_DUALNUM mode
>  
> luajit-gdb.py displays integers in LJ_DUALNUM mode as nan-s. The
> dumper function produces output considering any input value as a
> double. However, in LJ_DUALNUM mode, integers and doubles are stored
> differently, so the `itype` of a double 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
> =========================================================
>  
> Here is the diff:
> =========================================================
> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
> index 9ccca66a..25428745 100644
> --- a/src/luajit-gdb.py
> +++ b/src/luajit-gdb.py
> @@ -507,10 +507,14 @@ pointers respectively.
>      '''
>      def invoke(self, arg, from_tty):
> -        gdb.write('LJ_64: {LJ_64}, LJ_GC64: {LJ_GC64}\n'.format(
> -            LJ_64 = LJ_64,
> -            LJ_GC64 = LJ_GC64
> -        ))
> +        gdb.write(
> +            'LJ_64: {LJ_64}, LJ_GC64: {LJ_GC64}, LJ_DUALNUM : {LJ_DUALNUM}\n'

Side note: Excess whitespace. I've fixed this before merging the patch
into the trunk.

> +            .format(
> +                LJ_64 = LJ_64,
> +                LJ_GC64 = LJ_GC64,
> +                LJ_DUALNUM = LJ_DUALNUM
> +            )
> +        )
>  class LJDumpTValue(LJBase):
>      '''

<snipped>

> =========================================================
>  
> > 

<snipped>

> >Best regards,
> >Maxim Kokryashkin
> > 

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit v2] luajit-gdb: support dualnum mode
  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
  1 sibling, 0 replies; 13+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-08-17  7:56 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Sergey,

On 14.08.21, Sergey Kaplun wrote:
> Hi, Maxim!
> 
> Thanks for the fixes!
> 
> LGTM, except two nitpicks below.
> 
> On 14.08.21, Maxim Kokryashkin wrote:

<snipped>

> > diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
> > index c50405ad..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
> 
> Strictly saying, it is `<`, because `==` is staying for integer
> representation.

At first, I believed you're right, but unfortunately, you're wrong.
Consider the following:
| $ cmake . -DLUAJIT_NUMMODE=2 -DCMAKE_BUILD_TYPE=Debug
| <snipped> # Manually enable LJ_DUALNUM for x64
| $ make -j
| <snipped>
| $ cd src
| $ gdb --args ./luajit -e 'print(1)'
| <snipped>
| Reading symbols from ./luajit...
| lj-arch command initialized
| lj-tv command initialized
| lj-str command initialized
| lj-tab command initialized
| lj-stack command initialized
| lj-state command initialized
| lj-gc command initialized
| luajit-gdb.py is successfully loaded
| (gdb) b lj_cf_print
| Breakpoint 1 at 0x2960a: file /luajit/src/lib_base.c, line 485.
| (gdb) r
| Starting program: /luajit/src/luajit -e print\(1\)
| 
| Breakpoint 1, lj_cf_print (L=0x0) at /luajit/src/lib_base.c:485
| 485     {
| (gdb) n
| 486       ptrdiff_t i, nargs = L->top - L->base;
| (gdb) lj-stack L
| 0x40001a50:0x40001a70 [    ] 5 slots: Red zone
| 0x40001a48            [   M]
| 0x40001958:0x40001a40 [    ] 30 slots: Free stack slots
| 0x40001950            [  T ]
| 0x40001948            [ B  ] VALUE: not valid type @ 0x1
| 0x40001940            [    ] FRAME: [L] delta=1, fast function #29
| 0x40001938            [    ] FRAME: [V] delta=1, Lua function @ 0x40008428, 0 upvalues, "=(command line)":0
| 0x40001930            [    ] FRAME: [CP] delta=3, Lua function @ 0x40008428, 0 upvalues, "=(command line)":0
| 0x40001928            [    ] VALUE: C function @ 0x55555555c945
| 0x40001920            [    ] VALUE: light userdata @ 0x0
| 0x40001918            [    ] FRAME: [CP] delta=1, C function @ 0x55555555def8
| 0x40001910            [S   ] FRAME: dummy L

There is a "not valid type" stub, since < is used instead of <=. I
reverted this change and here is the result:
| $ git diff
| diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
| index f7347cf3..4488775c 100644
| --- a/src/luajit-gdb.py
| +++ b/src/luajit-gdb.py
| @@ -243,7 +243,7 @@ def tvisint(o):
|      return LJ_DUALNUM and itype(o) == LJ_TISNUM
|  
|  def tvisnumber(o):
| -    return itype(o) < LJ_TISNUM
| +    return itype(o) <= LJ_TISNUM
|  
|  def tvislightud(o):
|      if LJ_64 and not LJ_GC64:
| $ cmake . -DLUAJIT_NUMMODE=2 -DCMAKE_BUILD_TYPE=Debug
| <snipped> # Manually enable LJ_DUALNUM for x64
| $ make -j
| <snipped>
| $ cd src
| $ gdb --args ./luajit -e 'print(1)'
| <snipped>
| Reading symbols from ./luajit...
| lj-arch command initialized
| lj-tv command initialized
| lj-str command initialized
| lj-tab command initialized
| lj-stack command initialized
| lj-state command initialized
| lj-gc command initialized
| luajit-gdb.py is successfully loaded
| (gdb) b lj_cf_print
| Breakpoint 1 at 0x2960a: file /luajit/src/lib_base.c, line 485.
| (gdb) r
| Starting program: /luajit/src/luajit -e print\(1\)
| 
| Breakpoint 1, lj_cf_print (L=0x0) at /luajit/src/lib_base.c:485
| 485     {
| (gdb) n
| 486       ptrdiff_t i, nargs = L->top - L->base;
| (gdb) lj-stack L
| 0x40001a50:0x40001a70 [    ] 5 slots: Red zone
| 0x40001a48            [   M]
| 0x40001958:0x40001a40 [    ] 30 slots: Free stack slots
| 0x40001950            [  T ]
| 0x40001948            [ B  ] VALUE: integer 1
| 0x40001940            [    ] FRAME: [L] delta=1, fast function #29
| 0x40001938            [    ] FRAME: [V] delta=1, Lua function @ 0x40008428, 0 upvalues, "=(command line)":0
| 0x40001930            [    ] FRAME: [CP] delta=3, Lua function @ 0x40008428, 0 upvalues, "=(command line)":0
| 0x40001928            [    ] VALUE: C function @ 0x55555555c945
| 0x40001920            [    ] VALUE: light userdata @ 0x0
| 0x40001918            [    ] FRAME: [CP] delta=1, C function @ 0x55555555def8
| 0x40001910            [S   ] FRAME: dummy L

Hence, I dropped this change from the resulting patch.

> 

<snipped>

> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] luajit-gdb: support dualnum mode
  2021-07-31 13:36 [Tarantool-patches] [PATCH luajit] luajit-gdb: support dualnum mode Maxim Kokryashkin via Tarantool-patches
  2021-08-11  8:27 ` Sergey Kaplun via Tarantool-patches
@ 2021-08-17  9:23 ` Igor Munkin via Tarantool-patches
  1 sibling, 0 replies; 13+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-08-17  9:23 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: tarantool-patches

Max,

I've checked the patch into all long-term branches in tarantool/luajit
and bumped a new version in 1.10, 2.7, 2.8 and master.

On 31.07.21, Maxim Kokryashkin wrote:
> For x86/x64 LJ_DUALNUM mode is disabled. But for some other arches
> (arm or arm64, for example) it is enabled by default. luajit-gdb.py
> displays integers in LJ_DUALNUM mode as nan-s.
> 
> As it turned out, luajit-gdb detects those integers as integers, but
> there was a problem with the dumper function itself. The dumper
> function produces output thinking of any input value as of a double.
> However, in DUALNUM mode, integers and floats 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.
> 
> But in any mode, lua numbers are stored as doubles on the C side, so it
> 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.
> 
> Closes tarantool/tarantool#6224
> ---
> 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(-)
> 

<snipped>

> -- 
> 2.32.0
> 

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-08-17  9:48 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-31 13:36 [Tarantool-patches] [PATCH luajit] luajit-gdb: support dualnum mode Maxim Kokryashkin via Tarantool-patches
2021-08-11  8:27 ` Sergey Kaplun via Tarantool-patches
2021-08-13 14:29   ` Максим Корякшин via Tarantool-patches
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

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