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 E43E56E444; Sat, 20 Aug 2022 09:30:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E43E56E444 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1660977037; bh=twVDg3DlfsiGsj6NmHXfB61JaklpKEqRUto17tCflFo=; 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=ZGbDlb5VKzoZk3Nyd/XS5GqXqvEriTA7j0yZ7U+odYkmOp83Gae6ulhSKeOtd8AFs KrDkme/9+9CJUQ1ugjf4DoTOhrpwgIOKu+eOoG9CIPmZIU0bgfVDnFbf/7CHk7eRgG tsscUXJanPqNaM2mTN9sm9sBVeAa18e9XYpeacio= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (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 2787A6E444 for ; Sat, 20 Aug 2022 09:30:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2787A6E444 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1oPI0B-0005AB-F3; Sat, 20 Aug 2022 09:30:35 +0300 Date: Sat, 20 Aug 2022 09:28:02 +0300 To: Mikhail Elhimov Message-ID: References: <2cb4fea69537ffd196d535a7d3d47c0511b0ecac.1658531255.git.m.elhimov@vk.team> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2cb4fea69537ffd196d535a7d3d47c0511b0ecac.1658531255.git.m.elhimov@vk.team> X-Mailru-Src: smtp X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9E6910A3ADAF35E02D55AE39D95B190EA5D3686AA287B8E2C182A05F5380850404C228DA9ACA6FE2704EEE22AA2B98F80B217961CF177520B3DC17A96CBEADEEEE63F4FB5F886482F X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE73B44982FA5E78411EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006379A70878BADDF00B98638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8FD05208E6046D9FF1B28301793FBA4EE117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC5FC25ED3FCEC3375A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18E5D25F19253116ADD2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B67ECBC18655D52CDF089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 9604B64F49C60606AD91A466A1DEF99B296C473AB1E142185AC9E3593CE4B31AB1881A6453793CE9274300E5CE05BD4401A9E91200F654B08B38D1BE91256DA79FD1061F65C6B0B0F742D97473548D2A964E2F42045214479C2B6934AE262D3EE7EAB7254005DCED8DA55E71E02F9FC08E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D349A401E2B4D763A4AA457DAE079636A2F4200993E0B1AEECA633D370601405B8F1C455FAE54DDC7901D7E09C32AA3244C0CB2EDDA4092968767BF5B394F417EE36C24832127668422FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojsjCIxnYhpZM0sbiZdhhSxw== X-DA7885C5: AA18AC9F4655CBD7C91E6870482FA9EAD850759EE44F00F8497DA569B69B6311262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F7393CC2E0F076E87284E0CD9F28E008B94BFC78A53DD3E29E7930FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 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: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Mikhail! Thanks for the patch! It's generally LGTM, except a bunch of nits and typos. BTW, I like your iterator approach: the code looks so much better now! On 23.07.22, Mikhail Elhimov via Tarantool-patches wrote: > Changes: > - 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 Typo: s/of common/of the common/ > - Framelink always refers to the slot right before frame's base Don't get this change bullet description. Looks like this is just the way you implement frame iterator, isn't it? > - Introduce constant LJ_FRAMELINK_NUM_SLOTS to make slot arithmetic > more obvious. Previous using of boolean LJ_RT2 in arithmetic looked Typo: s/LJ_RT2/LJ_FR2 > like a tricky way, both to read and maintain (however places where Side note: As for me it looks like obvious and natural :). But maybe this is the ill effect from LuaJIT sources. I leave comments in places where I suggest to use LJ_FR2 instead LJ_FRAMELINK_NUM_SLOTS. > LJ_RT2 approach had been originally replicated from the source code Typo: s/LJ_RT2/LJ_FR2 > are left intact, because it is expected that maintenance efforts for > them will be minimal) General: Missed dot at the end of the sentences. > --- > 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 > @@ -299,6 +300,21 @@ gclen = { > 'mmudata': gcringlen, > } > > +def get_framelink_sentinel(L): > + stack = mref('TValue *', L['stack']) > + return stack + LJ_FRAMELINK_NUM_SLOTS - 1 `LJ_FRAMELINK_NUM_SLOTS - 1 == LJ_FR2` so we can replace it like the following: | return stack + LJ_FR2 > + > +# Generator that implements frame iterator Typo: s/Generator/The generator/ Typo: s/iterator/iterator./ > +# every frame is represented as a tuple of framelink and frametop Typo: s/every/Every/ Typo: s/frametop/frametop./ Minor: I suggest to specify that this is yielded to the caller tuple. Feel free to ignore. > +def frames(L): > + frametop = L['top'] > + framelink = L['base'] - 1 > + framelink_sentinel = get_framelink_sentinel(L) > + while framelink is not None: Minor: `is not None` is excess, we can just omit it. > + yield framelink, frametop > + frametop = framelink - LJ_FRAMELINK_NUM_SLOTS > + framelink = frame_prev(framelink) if framelink > framelink_sentinel else None > + > # 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 > + > +def dump_framelink_func(fr): > + return dump_lj_tfunc(fr - 1 if LJ_FR2 else fr) We can replace it with the following: | return dump_lj_tfunc(fr - LJ_FR2) > + > def dump_framelink(L, fr): > ) > > -def dump_stack_slot(L, slot, base=None, top=None, eol='\n'): > +def dump_stack_slot(L, slot, base=None, top=None): > > def dump_stack(L, base=None, top=None): > + for framelink, frametop in frames(L): > + # dump all data slots in the (framelink, top) interval Typo: s/dump/Dump/ Typo: s/interval/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) Typo: s/dump/Dump/ Missed dot at the end of the sentence. > + dump.append( dump_framelink(L, framelink) ) > + > + return '\n'.join(dump) > > def dump_gc(g): > gc = g['gc'] > @@ -717,7 +713,7 @@ The command requires no args and dumps current GC stats: > @@ -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 As far as LJ_FR2 is defined before, we can initialize this variable like the following: | LJ_FRAMELINK_NUM_SLOTS = LJ_FR2 + 1 > except: > gdb.write('luajit-gdb.py failed to load: ' > 'no debugging symbols found for libluajit\n') > -- > 2.34.1 > -- Best regards, Sergey Kaplun