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

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

Max,

Thanks for the patch! LGTM in general, but consider my nits below.

On 05.10.22, Maxim Kokryashkin via Tarantool-patches 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

I have no strong opinion regarding this constant: on the one hand we're
trying to be as much close to LuaJIT sources here as we can, but Sergos'
comment looks valid either. I leave the final decision regarding this on
your mighty shoulders, since I'm OK with both introducing the constant
or leaving everything as is.

> +
>  # }}}
>  
>  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)
> +
> +def lightudlo(u):
> +  return u & ((1 << LJ_LIGHTUD_BITS_LO) - 1)

Regarding this function and your change in the final version, I have a
strong opinion, that we have to respect LuaJIT sources as close as we
can, so even if you decided to expand the macros (and hence do not use
the corresponding function in the Python extension), please do similar
work GCC does when expanding macros with -E parameter: add the comment
with the macro name that is expanded. Consider the following:
| def lightudV(tv):
|     if LJ_64:
|         u = int(tv['u64'])
|         # lightudseg macro expanded.
|         seg = (u >> LJ_LIGHTUD_BITS_LO) & LIGHTUD_SEG_MASK
|         segmap = mref('uint32_t *', G(L(None))['gc']['lightudseg'])
|         # lightudlo macro expanded.
|         return (int(segmap[seg]) << 32) | (u & LIGHTUD_LO_MASK)
|     else:
|         return gcval(tv['gcr'])

Furthermore, why don't you consider hoisting the compile time condition
and create two functions to be specialized on the startup as lightudV
implementation? I doubt, this would change the performance (at least
this is not the key achievement), but the readability may be increased.
Anyway feel free to ignore the last comment.

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

-- 
Best regards,
IM