From: Mikhail Elhimov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Igor Munkin <imun@tarantool.org>, Mikhail Elhimov <elhimov@gmail.com> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH 2/2] gdb: refactor iteration over frames while dumping stack Date: Thu, 1 Sep 2022 02:20:26 +0300 [thread overview] Message-ID: <a1a47308-8ef0-befa-07c3-141b51ebc19e@vk.team> (raw) In-Reply-To: <Yw90k1oDJMmzH0Df@tarantool.org> Hi, Igor! Thanks for the review! On 31.08.2022 17:47, Igor Munkin wrote: > Misha, > > Thanks for the patch! Please consider my comments below. > > On 23.07.22, Mikhail Elhimov via Tarantool-patches wrote: >> Changes: > Minor: You can omit "Changes:" here; just list them :) Fixed >> - Introduce generator to iterate over frames in a Python way >> - Dump framelink of the bottom frame in a common way and identify it as >> a dummy framelink only inside of common dump function >> - Framelink always refers to the slot right before frame's base > Side note: These three bullets above are simply great, thanks a lot! > >> - Introduce constant LJ_FRAMELINK_NUM_SLOTS to make slot arithmetic >> more obvious. Previous using of boolean LJ_RT2 in arithmetic looked >> like a tricky way, both to read and maintain (however places where >> LJ_RT2 approach had been originally replicated from the source code >> are left intact, because it is expected that maintenance efforts for >> them will be minimal) > Well, I'm also see no sense in introducing this variable. > 1. It doesn't exist in LuaJIT sources. > 2. It's simply LJ_FR2 + 1 (you can convert LJ_FR2 from the boolean > context to the numeric one, if you want) > 3. It is required only in a single place: <frames> function. Another > place where it is used is <get_framelink_sentinel> but as Sergey > already mentioned, LJ_FR2 fits much better there. > > I suggest to stay as close as possible to LuaJIT defines and LuaJIT code > base, since this extension also violates DRY approach, that is already > ignored almost everywhere in LuaJIT codebase. > > All in all, I see no reason to introduce a new variable for a single > case, and propose to continue using LJ_FR2 for such cases. Fixed >> --- >> src/luajit-gdb.py | 101 ++++++++++++++++++++++------------------------ >> 1 file changed, 49 insertions(+), 52 deletions(-) >> >> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py >> index 1e9a96fb..d7d34c70 100644 >> --- a/src/luajit-gdb.py >> +++ b/src/luajit-gdb.py >> @@ -158,6 +158,7 @@ LJ_64 = None >> LJ_GC64 = None >> LJ_FR2 = None >> LJ_DUALNUM = None >> +LJ_FRAMELINK_NUM_SLOTS = None > Considering the comment right above, this can be dropped. Fixed >> >> LJ_GCVMASK = ((1 << 47) - 1) >> LJ_TISNUM = None >> @@ -299,6 +300,21 @@ gclen = { >> 'mmudata': gcringlen, >> } >> >> +def get_framelink_sentinel(L): >> + stack = mref('TValue *', L['stack']) >> + return stack + LJ_FRAMELINK_NUM_SLOTS - 1 > IMHO, you can move this into Frames section with the name kinda > <frame_sentinel>. And use LJ_FR2 here to make it a oneliner. Fixed >> + >> +# Generator that implements frame iterator >> +# every frame is represented as a tuple of framelink and frametop >> +def frames(L): >> + frametop = L['top'] >> + framelink = L['base'] - 1 >> + framelink_sentinel = get_framelink_sentinel(L) >> + while framelink is not None: >> + yield framelink, frametop >> + frametop = framelink - LJ_FRAMELINK_NUM_SLOTS >> + framelink = frame_prev(framelink) if framelink > framelink_sentinel else None > Side note: I've seen the newest version. > >> + >> # Dumpers {{{ >> >> def dump_lj_tnil(tv): >> @@ -397,32 +413,38 @@ dumpers = { >> def dump_tvalue(tvalue): >> return dumpers.get(typenames(itypemap(tvalue)), dump_lj_invalid)(tvalue) >> >> +def dump_framelink_slot_address(fr): >> + return '{}:{}'.format(fr - 1, fr) if LJ_FR2 else '{}'.format(fr) + PADDING > Minor: It's better to split this line into two (like it's done for > <frame_ftsz> or <frame_pc>). Feel free to ignore, since I was just > confused whether PADDING is added only to the else part or to the whole > if-else result. Fixed >> + >> +def dump_framelink_func(fr): >> + return dump_lj_tfunc(fr - 1 if LJ_FR2 else fr) > Oh.. Just use fr - LJ_FR2 in the call below, please: there is no need to > introduce a function to be used once. Fixed >> + >> def dump_framelink(L, fr): >> - fr2 = fr + LJ_FR2 >> - >> - return '{fr}{padding} [ ] FRAME: [{pp}] delta={d}, {f}\n'.format( >> - fr = fr, >> - padding = ':{fr2}'.format(fr2 = fr2) if LJ_FR2 else PADDING, >> - pp = 'PP' if frame_ispcall(fr2) else '{frname}{p}'.format( >> - frname = frametypes(int(frame_type(fr2))), >> - p = 'P' if frame_typep(fr2) & FRAME_P else '' >> + if fr == get_framelink_sentinel(L): >> + return '{addr} [S ] FRAME: dummy L'.format( >> + addr = dump_framelink_slot_address(fr), >> + ) >> + return '{addr} [ ] FRAME: [{pp}] delta={d}, {f}'.format( >> + addr = dump_framelink_slot_address(fr), >> + pp = 'PP' if frame_ispcall(fr) else '{frname}{p}'.format( >> + frname = frametypes(int(frame_type(fr))), >> + p = 'P' if frame_typep(fr) & FRAME_P else '' >> ), >> - d = cast('TValue *', fr2) - cast('TValue *', frame_prev(fr2)), >> - f = dump_lj_tfunc(fr), >> + d = cast('TValue *', fr) - cast('TValue *', frame_prev(fr)), >> + f = dump_framelink_func(fr), > Considering the comment right above, <dump_lj_tfunc> can be used here. Fixed >> ) >> > <snipped> > >> @@ -717,7 +713,7 @@ The command requires no args and dumps current GC stats: >> )) >> >> def init(commands): >> - global LJ_64, LJ_GC64, LJ_FR2, LJ_DUALNUM, LJ_TISNUM, PADDING >> + global LJ_64, LJ_GC64, LJ_FR2, LJ_DUALNUM, LJ_FRAMELINK_NUM_SLOTS, LJ_TISNUM, PADDING > Considering the comment at the beginning, this part can be dropped. Fixed (other occurrences of LJ_FRAMELINK_NUM_SLOTS are dropped as well) >> >> # XXX Fragile: though connecting the callback looks like a crap but it >> # respects both Python 2 and Python 3 (see #4828). >> @@ -759,6 +755,7 @@ def init(commands): >> LJ_64 = str(gdb.parse_and_eval('IRT_PTR')) == 'IRT_P64' >> LJ_FR2 = LJ_GC64 = str(gdb.parse_and_eval('IRT_PGC')) == 'IRT_P64' >> LJ_DUALNUM = gdb.lookup_global_symbol('lj_lib_checknumber') is not None >> + LJ_FRAMELINK_NUM_SLOTS = 2 if LJ_FR2 else 1 >> except: >> gdb.write('luajit-gdb.py failed to load: ' >> 'no debugging symbols found for libluajit\n') >> -- >> 2.34.1 >> =================================================== diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py index 9846f3a2..85dffa7b 100644 --- a/src/luajit-gdb.py +++ b/src/luajit-gdb.py @@ -150,6 +150,9 @@ def frame_prev(framelink): return frame_prevl(framelink) if frame_islua(framelink) \ else frame_prevd(framelink) +def frame_sentinel(L): + return mref('TValue *', L['stack']) + LJ_FR2 + # }}} # Const {{{ @@ -299,6 +302,19 @@ gclen = { 'mmudata': gcringlen, } +# The generator that implements frame iterator. +# Every frame is represented as a tuple of framelink and frametop. +def frames(L): + frametop = L['top'] + framelink = L['base'] - 1 + framelink_sentinel = frame_sentinel(L) + while True: + yield framelink, frametop + frametop = framelink - (1 + LJ_FR2) + if framelink <= framelink_sentinel: + break + framelink = frame_prev(framelink) + # Dumpers {{{ def dump_lj_tnil(tv): @@ -397,32 +413,36 @@ dumpers = { def dump_tvalue(tvalue): return dumpers.get(typenames(itypemap(tvalue)), dump_lj_invalid)(tvalue) +def dump_framelink_slot_address(fr): + return '{}:{}'.format(fr - 1, fr) if LJ_FR2 \ + else '{}'.format(fr) + PADDING + def dump_framelink(L, fr): - fr2 = fr + LJ_FR2 - - return '{fr}{padding} [ ] FRAME: [{pp}] delta={d}, {f}\n'.format( - fr = fr, - padding = ':{fr2}'.format(fr2 = fr2) if LJ_FR2 else PADDING, - pp = 'PP' if frame_ispcall(fr2) else '{frname}{p}'.format( - frname = frametypes(int(frame_type(fr2))), - p = 'P' if frame_typep(fr2) & FRAME_P else '' + if fr == frame_sentinel(L): + return '{addr} [S ] FRAME: dummy L'.format( + addr = dump_framelink_slot_address(fr), + ) + return '{addr} [ ] FRAME: [{pp}] delta={d}, {f}'.format( + addr = dump_framelink_slot_address(fr), + pp = 'PP' if frame_ispcall(fr) else '{frname}{p}'.format( + frname = frametypes(int(frame_type(fr))), + p = 'P' if frame_typep(fr) & FRAME_P else '' ), - d = cast('TValue *', fr2) - cast('TValue *', frame_prev(fr2)), - f = dump_lj_tfunc(fr), + d = cast('TValue *', fr) - cast('TValue *', frame_prev(fr)), + f = dump_lj_tfunc(fr - LJ_FR2), ) -def dump_stack_slot(L, slot, base=None, top=None, eol='\n'): +def dump_stack_slot(L, slot, base=None, top=None): base = base or L['base'] top = top or L['top'] - return '{addr}{padding} [ {B}{T}{M}] VALUE: {value}{eol}'.format( + return '{addr}{padding} [ {B}{T}{M}] VALUE: {value}'.format( addr = strx64(slot), padding = PADDING, B = 'B' if slot == base else ' ', T = 'T' if slot == top else ' ', M = 'M' if slot == mref('TValue *', L['maxstack']) else ' ', value = dump_tvalue(slot), - eol = eol, ) def dump_stack(L, base=None, top=None): @@ -439,7 +459,7 @@ def dump_stack(L, base=None, top=None): ), ] dump.extend([ - dump_stack_slot(L, maxstack + offset, base, top, '') + dump_stack_slot(L, maxstack + offset, base, top) for offset in range(red, 0, -1) ]) dump.extend([ @@ -447,50 +467,24 @@ def dump_stack(L, base=None, top=None): padding = '-' * len(PADDING), nstackslots = int((tou64(maxstack) - tou64(stack)) >> 3), ), - dump_stack_slot(L, maxstack, base, top, ''), + dump_stack_slot(L, maxstack, base, top), '{start}:{end} [ ] {nfreeslots} slots: Free stack slots'.format( start = strx64(top + 1), end = strx64(maxstack - 1), nfreeslots = int((tou64(maxstack) - tou64(top) - 8) >> 3), ), ]) - dump = '\n'.join(dump) + '\n' - - slot = top - framelink = base - (1 + LJ_FR2) - - # XXX: Lua stack unwinding algorithm consists of the following steps: - # 1. dump all data slots in the (framelink, top) interval - # 2. check whether there are remaining frames - # 3. if there are no slots further, stop the unwinding loop - # 4. otherwise, resolve the next framelink and top and go to (1) - # - # Postcondition (i.e. do-while) loops is the most fitting idiom for such - # case, but Python doesn't provide such lexical construction. Hence step (1) - # is unrolled for the topmost stack frame. - while slot > framelink + LJ_FR2: - dump += dump_stack_slot(L, slot, base, top) - slot -= 1 - - while framelink > stack: - assert slot == framelink + LJ_FR2, "Invalid slot during frame unwind" - dump += dump_framelink(L, framelink) - framelink = frame_prev(framelink + LJ_FR2) - LJ_FR2 - slot -= 1 + LJ_FR2 - while slot > framelink + LJ_FR2: - dump += dump_stack_slot(L, slot, base, top) - slot -= 1 - - assert slot == framelink + LJ_FR2, "Invalid slot after frame unwind" - # Skip a nil slot for the last frame for 2-slot frames. - slot -= LJ_FR2 - - dump += '{fr}{padding} [S ] FRAME: dummy L'.format( - fr = slot, - padding = ':{nilslot}'.format(nilslot = slot + 1) if LJ_FR2 else PADDING - ) - return dump + for framelink, frametop in frames(L): + # Dump all data slots in the (framelink, top) interval. + dump.extend([ + dump_stack_slot(L, framelink + offset, base, top) + for offset in range(frametop - framelink, 0, -1) + ]) + # Dump frame slot (2 slots in case of GC64). + dump.append( dump_framelink(L, framelink) ) + + return '\n'.join(dump) def dump_gc(g): gc = g['gc'] -- Best regards, Mikhail Elhimov
next prev parent reply other threads:[~2022-08-31 23:20 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-23 0:11 [Tarantool-patches] [PATCH 0/2] gdb: adjust to support Python 2 + refactoring of stack dumping Mikhail Elhimov via Tarantool-patches 2022-07-23 0:11 ` [Tarantool-patches] [PATCH 1/2] gdb: adjust to support python2 (centos 7) Mikhail Elhimov via Tarantool-patches 2022-08-20 6:26 ` Sergey Kaplun via Tarantool-patches 2022-08-22 13:00 ` Mikhail Elhimov via Tarantool-patches 2022-08-22 17:46 ` Mikhail Elhimov via Tarantool-patches 2022-08-31 14:47 ` Igor Munkin via Tarantool-patches 2022-08-31 23:17 ` Mikhail Elhimov via Tarantool-patches 2022-07-23 0:11 ` [Tarantool-patches] [PATCH 2/2] gdb: refactor iteration over frames while dumping stack Mikhail Elhimov via Tarantool-patches 2022-08-20 6:28 ` Sergey Kaplun via Tarantool-patches 2022-08-23 9:13 ` Mikhail Elhimov via Tarantool-patches 2022-08-31 14:47 ` Igor Munkin via Tarantool-patches 2022-08-31 23:20 ` Mikhail Elhimov via Tarantool-patches [this message] 2022-09-14 21:28 ` Igor Munkin via Tarantool-patches 2022-11-11 8:53 ` [Tarantool-patches] [PATCH 0/2] gdb: adjust to support Python 2 + refactoring of stack dumping Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=a1a47308-8ef0-befa-07c3-141b51ebc19e@vk.team \ --to=tarantool-patches@dev.tarantool.org \ --cc=elhimov@gmail.com \ --cc=imun@tarantool.org \ --cc=m.elhimov@vk.team \ --subject='Re: [Tarantool-patches] [PATCH 2/2] gdb: refactor iteration over frames while dumping stack' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox