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 3187E6F154; Wed, 31 Aug 2022 17:58:07 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3187E6F154 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1661957887; bh=76igAFpTv+cEWkZ7rLry6otWmycPFVE1piebdcE8pno=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=UKl/xjyDACpwwaNMnVILzh2YXIL5/5ty2P3NhC0Hf6df7gyfjzy+udT/oD4qIR6UC //4MOHf+Q16F2sk6gcomcuYg7tzJC/Rw/q/pkjlyGMt1xDWPYyV/+z72I1vZlhko12 dsfjKKJkf7q5Tp4r7MjfTy6rtKLuxmZYne37XxKw= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (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 BEF516F154 for ; Wed, 31 Aug 2022 17:58:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BEF516F154 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1oTPAK-0005iV-PC; Wed, 31 Aug 2022 17:58:05 +0300 Date: Wed, 31 Aug 2022 17:47:47 +0300 To: Mikhail Elhimov Message-ID: References: <2cb4fea69537ffd196d535a7d3d47c0511b0ecac.1658531255.git.m.elhimov@vk.team> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <2cb4fea69537ffd196d535a7d3d47c0511b0ecac.1658531255.git.m.elhimov@vk.team> X-Clacks-Overhead: GNU Terry Pratchett X-Mailru-Src: smtp X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD9BF9AC82A1D2D723796C00AC894E796FC85C14BEDEA5216A0182A05F538085040BBF942317DDA1E0DA55DF3A9FE32382871AB3299E747DAE12DC9B7AEE4B42D10 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE78E8764B5BC580342EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637BD890EF519C7ACB58638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D838B031ED246D0599ED2C3FFDC58361A3117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B974A882099E279BDA471835C12D1D977C4224003CC8364762BB6847A3DEAEFB0F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7CA1D607EB49F9BFEF43847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 9604B64F49C60606AD91A466A1DEF99B296C473AB1E142185AC9E3593CE4B31AB1881A6453793CE9274300E5CE05BD4401A9E91200F654B0F56C66D0505D9834523897933F9F8387FE3E2D1544F0556C55E79E1B5F72F2F29C2B6934AE262D3EE7EAB7254005DCED8DA55E71E02F9FC08E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D349320586B19AD2C725B89BBEA4FF5A103843CFCDBB142C9A4A35187D571DD47690E509CB636ABB18F1D7E09C32AA3244C37081541D434677D5331FCEA11DB27B295A9E0DC41E9A4CF927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojxzT94hBWypp4CLSrSvZmMg== X-DA7885C5: B8A327C75E3849090083DFCCC5F5FD47E1991A65585C0C6BFBA7BAEE129AFEB2262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F7393CC2E0F076E87284E61F914EE481FB50595D17C1A3037AE78A7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E3365FEEDEB644C299C0ED14614B50AE0675 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: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 :) > - 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. > --- > 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. > > 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. > + > +# 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. > + > +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. > + > 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. > ) > > @@ -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. > > # 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 > -- Best regards, IM