Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] luajit-gdb: support full-range 64-bit lightud
@ 2022-10-05 14:43 Maxim Kokryashkin via Tarantool-patches
  2022-10-06  9:02 ` sergos via Tarantool-patches
  2022-11-29 15:20 ` Igor Munkin via Tarantool-patches
  0 siblings, 2 replies; 3+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2022-10-05 14:43 UTC (permalink / raw)
  To: tarantool-patches, sergos, skaplun

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] luajit-gdb: support full-range 64-bit lightud
  2022-10-05 14:43 [Tarantool-patches] [PATCH luajit] luajit-gdb: support full-range 64-bit lightud Maxim Kokryashkin via Tarantool-patches
@ 2022-10-06  9:02 ` sergos via Tarantool-patches
  2022-11-29 15:20 ` Igor Munkin via Tarantool-patches
  1 sibling, 0 replies; 3+ messages in thread
From: sergos via Tarantool-patches @ 2022-10-06  9:02 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: 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
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] luajit-gdb: support full-range 64-bit lightud
  2022-10-05 14:43 [Tarantool-patches] [PATCH luajit] luajit-gdb: support full-range 64-bit lightud Maxim Kokryashkin via Tarantool-patches
  2022-10-06  9:02 ` sergos via Tarantool-patches
@ 2022-11-29 15:20 ` Igor Munkin via Tarantool-patches
  1 sibling, 0 replies; 3+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2022-11-29 15:20 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: 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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-11-29 15:34 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-05 14:43 [Tarantool-patches] [PATCH luajit] luajit-gdb: support full-range 64-bit lightud Maxim Kokryashkin via Tarantool-patches
2022-10-06  9:02 ` sergos via Tarantool-patches
2022-11-29 15:20 ` Igor Munkin via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox