* [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; 4+ 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] 4+ 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; 4+ 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] 4+ 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 2022-12-01 5:14 ` Maxim Kokryashkin via Tarantool-patches 1 sibling, 1 reply; 4+ 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] 4+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] luajit-gdb: support full-range 64-bit lightud 2022-11-29 15:20 ` Igor Munkin via Tarantool-patches @ 2022-12-01 5:14 ` Maxim Kokryashkin via Tarantool-patches 0 siblings, 0 replies; 4+ messages in thread From: Maxim Kokryashkin via Tarantool-patches @ 2022-12-01 5:14 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches, Maxim Kokryashkin [-- 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 --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-12-01 5:14 UTC | newest] Thread overview: 4+ 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 2022-12-01 5:14 ` Maxim Kokryashkin 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