

<HTML><BODY><div>Hi!</div><div>Thanks for the review!</div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16697361031131777581_BODY">Max,<br><br>Thanks for the patch! LGTM in general, but consider my nits below.<br><br>On 05.10.22, Maxim Kokryashkin via Tarantool-patches wrote:<br>> Following up the introduction of full-range 64-bit lightuserdata<br>> support in commit 2cacfa8 ("Add support for full-range 64 bit<br>> lightuserdata."), this patch modifies the corresponding dumper<br>> behavior for LJ_64 platforms in the luajit-gdb extension.<br>><br>> Resolves tarantool/tarantool#6481<br>> ---<br>> Branch: <a href="https://github.com/tarantool/luajit/tree/fckxorg/gh-6481-luajit-gdb-light-ud" target="_blank">https://github.com/tarantool/luajit/tree/fckxorg/gh-6481-luajit-gdb-light-ud</a><br>> Issue: <a href="https://github.com/tarantool/tarantool/issues/6481" target="_blank">https://github.com/tarantool/tarantool/issues/6481</a><br>> src/luajit-gdb.py | 18 +++++++++++++++++-<br>> 1 file changed, 17 insertions(+), 1 deletion(-)<br>><br>> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py<br>> index 6480d014..2993f2c1 100644<br>> --- a/src/luajit-gdb.py<br>> +++ b/src/luajit-gdb.py<br>> @@ -166,6 +166,9 @@ LJ_GCVMASK = ((1 << 47) - 1)<br>> LJ_TISNUM = None<br>> PADDING = None<br>><br>> +LJ_LIGHTUD_BITS_SEG = 8<br>> +LJ_LIGHTUD_BITS_LO = 47 - LJ_LIGHTUD_BITS_SEG<br><br>I have no strong opinion regarding this constant: on the one hand we're<br>trying to be as much close to LuaJIT sources here as we can, but Sergos'<br>comment looks valid either. I leave the final decision regarding this on<br>your mighty shoulders, since I'm OK with both introducing the constant<br>or leaving everything as is.</div></div></div></div></blockquote><div>Left those constants unchanged.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> +<br>> # }}}<br>><br>> def itype(o):<br>> @@ -315,6 +318,12 @@ def frames(L):<br>> break<br>> framelink = frame_prev(framelink)<br>><br>> +def lightudseg(u):<br>> + return (u >> LJ_LIGHTUD_BITS_LO) & ((1 << LJ_LIGHTUD_BITS_SEG) - 1)<br>> +<br>> +def lightudlo(u):<br>> + return u & ((1 << LJ_LIGHTUD_BITS_LO) - 1)<br><br>Regarding this function and your change in the final version, I have a<br>strong opinion, that we have to respect LuaJIT sources as close as we<br>can, so even if you decided to expand the macros (and hence do not use<br>the corresponding function in the Python extension), please do similar<br>work GCC does when expanding macros with -E parameter: add the comment<br>with the macro name that is expanded. Consider the following:<br>| def lightudV(tv):<br>| if LJ_64:<br>| u = int(tv['u64'])<br>| # lightudseg macro expanded.<br>| seg = (u >> LJ_LIGHTUD_BITS_LO) & LIGHTUD_SEG_MASK<br>| segmap = mref('uint32_t *', G(L(None))['gc']['lightudseg'])<br>| # lightudlo macro expanded.<br>| return (int(segmap[seg]) << 32) | (u & LIGHTUD_LO_MASK)<br>| else:<br>| return gcval(tv['gcr'])<br><br>Furthermore, why don't you consider hoisting the compile time condition<br>and create two functions to be specialized on the startup as lightudV<br>implementation? I doubt, this would change the performance (at least<br>this is not the key achievement), but the readability may be increased.<br>Anyway feel free to ignore the last comment.</div></div></div></div></blockquote><div>Added the comment you mentioned and hoisted the condition.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> +<br>> # Dumpers {{{<br>><br>> def dump_lj_tnil(tv):<br>> @@ -327,7 +336,14 @@ def dump_lj_ttrue(tv):<br>> return 'true'<br>><br>> def dump_lj_tlightud(tv):<br>> - return 'light userdata @ {}'.format(strx64(gcval(tv['gcr'])))<br>> + if LJ_64:<br>> + u = int(tv['u64'])<br>> + seg = lightudseg(u)<br>> + segmap = mref('uint32_t *', G(L(None))['gc']['lightudseg'])<br>> + addr = (int(segmap[seg]) << 32) | lightudlo(u)<br>> + else:<br>> + addr = gcval(tv['gcr'])<br>> + return 'light userdata @ {}'.format(strx64(addr))<br>><br>> def dump_lj_tstr(tv):<br>> return 'string {body} @ {address}'.format(<br>> --<br>> 2.36.1<br>><br><br>--<br>Best regards,<br>IM</div></div></div></div></blockquote><div><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div><div> </div></div></blockquote></BODY></HTML>