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

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

[-- 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 --]