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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.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