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 >