From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id E80AE6F153; Thu, 1 Sep 2022 02:20:29 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E80AE6F153 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1661988030; bh=05PMu8Zrfi7N8tOL6BBRTt0itjQjc7GU/kOQFg7SlOQ=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=d0FuVcVBulXJ94cGTcOW3JhcVh0UesAjude/4+TN60L0ohK8+W+zcL9RXMbRLOyn8 azJYXhbqrFCWHG2jFjYPFY2PtVolRnnZZT1Kf2PJmLZTAjsBAkwQUl+FW7Y0QVjhr/ pnFLRy4loWfnBCMH8h+eKGBNMEyNKqXMGdJEsxO4= Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 5D9346F153 for ; Thu, 1 Sep 2022 02:20:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 5D9346F153 Received: by smtp34.i.mail.ru with esmtpa (envelope-from ) id 1oTX0V-0005MK-Fx; Thu, 01 Sep 2022 02:20:28 +0300 Message-ID: Date: Thu, 1 Sep 2022 02:20:26 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Content-Language: en-US To: Igor Munkin , Mikhail Elhimov Cc: tarantool-patches@dev.tarantool.org References: <2cb4fea69537ffd196d535a7d3d47c0511b0ecac.1658531255.git.m.elhimov@vk.team> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Mailru-Src: smtp X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9BF9AC82A1D2D7237ECC9B9481A4CD7BD50746863994A0877182A05F538085040FFC081D2E3CF3A680210D0E137AE7EF16A566A451366C5A45445C078ACC22828 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE79145AB6E9E75F07EEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063773AA33509B65BC498638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D850B18A1C50DC21FA153843F28D6578466F9789CCF6C18C3F8528715B7D10C86878DA827A17800CE74A95F4E53E8DCE969FA2833FD35BB23D9E625A9149C048EE9ECD01F8117BC8BEA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18618001F51B5FD3F9D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE652FD71AFB96DC7D68E75A379BAF682BD8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3E6BDB36F057AC83CC0837EA9F3D19764C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F79006377563EB12C1DF93A4EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 9604B64F49C60606AD91A466A1DEF99B296C473AB1E142185AC9E3593CE4B31AB1881A6453793CE9274300E5CE05BD4401A9E91200F654B0D4C601CEA03E70EE523897933F9F83874D7B539BB5AA9E9A6626CA3424FB88359C2B6934AE262D3EE7EAB7254005DCED8DA55E71E02F9FC08E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D349379E7F8541B6C9A3DFD3EF7D4A33AA11CA2318CE0ADBEF4ABA794DB42F0BEC6A78695B5A8A92DFF1D7E09C32AA3244C4B8F96163C3A117F399AD300A3BB723CF26BFA4C8A6946B8FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojxzT94hBWyppzdU6Ui2MuIw== X-Mailru-Sender: 49D287FBCBBF3A5C9DDD0AAA0179FAA74B1F5F5AD2D2E48EEABD3F0BEF909DE60AA0D8A8FE37190D4CA08BFBBB45AAA72C22B24405C8F0CCB7331131FBF1F0346646DC4FAA7499610DD12ECE0827B3FF5685057D9971753F22B820C1B2086D890DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 2/2] gdb: refactor iteration over frames while dumping stack X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Mikhail Elhimov via Tarantool-patches Reply-To: Mikhail Elhimov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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: function. Another > place where it is used is 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 > . 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 > or ). 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, can be used here. Fixed >> ) >> > > >> @@ -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