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

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

[Maxim Kokryashkin via Tarantool-patches]

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
+
 # }}}
 
 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)
+
 # 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

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

[sergos via Tarantool-patches]

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
> 

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

[Igor Munkin via Tarantool-patches]

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

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

[Maxim Kokryashkin via Tarantool-patches]

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


Hi!
Thanks for the review!
 
> 
>>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.
>Left those constants unchanged.
>>
>>> +
>>> # }}}
>>>
>>> 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.
>Added the comment you mentioned and hoisted the condition.
>>
>>> +
>>> # 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
>--
>Best regards,
>Maxim Kokryashkin
> 

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