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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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