Re: [Tarantool-patches] [PATCH luajit] luajit-gdb: support full-range 64-bit lightud

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

Hi, Max!

Thanks for the patch!

0. Please, add Mikhail<m.elhimov@vk.team> to review.


> On 5 Oct 2022, at 17:43, Maxim Kokryashkin <max.kokryashkin@gmail.com> wrote:
> 
> Following up the introduction of full-range 64-bit lightuserdata
> support in commit 2cacfa8 ("Add support for full-range 64 bit
> lightuserdata."), this patch modifies the corresponding dumper
> behavior for LJ_64 platforms in the luajit-gdb extension.
> 
> Resolves tarantool/tarantool#6481
> ---
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-6481-luajit-gdb-light-ud
> Issue: https://github.com/tarantool/tarantool/issues/6481
> src/luajit-gdb.py | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
> index 6480d014..2993f2c1 100644
> --- a/src/luajit-gdb.py
> +++ b/src/luajit-gdb.py
> @@ -166,6 +166,9 @@ LJ_GCVMASK = ((1 << 47) - 1)
> LJ_TISNUM = None
> PADDING = None
> 
> +LJ_LIGHTUD_BITS_SEG = 8
> +LJ_LIGHTUD_BITS_LO = 47 - LJ_LIGHTUD_BITS_SEG

1. Is this ’47’ literal has the same meaning as the one above in the ‘LJ_GCVMASK’ 
   definition and the one below in the ‘itype()’ function? 

   Shall we introduce a constant to avoid running across the code to fix all ’47’s 
   later on?

> +
> # }}}
> 
> def itype(o):
> @@ -315,6 +318,12 @@ def frames(L):
>             break
>         framelink = frame_prev(framelink)
> 
> +def lightudseg(u):
> +  return (u >> LJ_LIGHTUD_BITS_LO) & ((1 << LJ_LIGHTUD_BITS_SEG) - 1)
> +
> +deflightudlo(u):
> +  return u & ((1 << LJ_LIGHTUD_BITS_LO) - 1)
> +

2a. Did you expect to reuse these two above somewhere else later?
    Also, the '((1 << LJ_LIGHTUD_BITS_LO) - 1)’ is a constant, can be named
    ‘LJ_LIGHTUD_BITS_MASK’. Then the whole ‘lightudlo’ is just an ‘&’ with
    this constant. The same can be done for _SEG_MASK and we can proceed to 2b.

> # Dumpers {{{
> 
> def dump_lj_tnil(tv):
> @@ -327,7 +336,14 @@ def dump_lj_ttrue(tv):
>     return 'true'
> 
> def dump_lj_tlightud(tv):
> -    return 'light userdata @ {}'.format(strx64(gcval(tv['gcr'])))
> +    if LJ_64:
> +        u = int(tv['u64'])
> +        seg = lightudseg(u)
> +        segmap = mref('uint32_t *', G(L(None))['gc']['lightudseg'])
> +        addr = (int(segmap[seg]) << 32) | lightudlo(u)

2b. Otherwise why not to put the whole iff clause above into a separate function?

> +    else:
> +        addr = gcval(tv['gcr'])
> +    return 'light userdata @ {}'.format(strx64(addr))
> 
> def dump_lj_tstr(tv):
>     return 'string {body} @ {address}'.format(
> -- 
> 2.36.1
>