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 1DA836EC40; Mon, 5 Jul 2021 12:42:26 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1DA836EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1625478146; bh=OBDJQzHUwJZ9982aWb6hxTYiT0NJiWNeQiw5LE+G83s=; 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=gAl5ubN4XyyUU/l7DU5kvP740MTut8KGJh9pwbBWXI+35HIRpSGNcD32V7JJBLwui /Le2oby6WgvvQx/9uHiMV3eHxMvClA6uUq9tCjUH2LJK5iYWgy7KlK42LH+E3c9C/d CasjlP6DDXUOz4bSU7bHv70D6laVLYGWc8lU5q0g= Received: from smtpng2.i.mail.ru (smtpng2.i.mail.ru [94.100.179.3]) (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 4D5F76EC40 for ; Mon, 5 Jul 2021 12:42:24 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4D5F76EC40 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1m0L7P-0008LZ-Aa; Mon, 05 Jul 2021 12:42:23 +0300 Date: Mon, 5 Jul 2021 12:18:53 +0300 To: Mikhail Shishatskiy Message-ID: <20210705091852.GB6106@tarantool.org> References: <20210630142020.326411-1-m.shishatskiy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210630142020.326411-1-m.shishatskiy@tarantool.org> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD954DFF1DC42D673FB64496B12B6C6FB871CAFB561BC326F0E182A05F53808504088155C133B81E5E2D17078E7AC2F788F78D04AB004614BBA8FAC54AE3578D2EE X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE73870E1FF9A049D91EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637AA11E7A85791869B8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D81CAF5588B6F94448606AEAFEF0666BE0117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BAA867293B0326636D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B642416645EBD45DC2089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2AD77751E876CB595E8F7B195E1C97831D80BC7CDA836C3320BD5C9FA4661C8D9 X-C1DE0DAB: 0D63561A33F958A5B0CFEFC4561D2DA4A1B68A9D23CAD12FAA8FF86FE6D3F53AD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75F95D039699DA79E9410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D2AC226BFD455724167A80E1C62C97105353621FDCDA298231CF566445FD548E6C658F87BA1256121D7E09C32AA3244CD4F23181C6138A15AB79D24C8BB6F3F1E3D93501275E802F927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj5fH2RN9TpJlbUiosEFHX3g== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5DD2011DFEE33F79D1B8EFD945B1D53B24A7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH] luajit-gdb: fix misalignment in lj-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 your patch! In addition to the comments left by Sergey, I also drop more verbose ones below. Please consider them. Let's start from the subject: * Consider the comment by Sergey, where new commit subject is proposed. If you look for the other patches related to luajit-gdb.py, you will find that they have 'gdb' prefix, not 'luajit-gdb'. Furthermore, since the issue relates only for LJ_GC64 mode, it's worth to mention this within the commit subject. * To differ the patches for tarantool/tarantool repo from the patches for tarantool/luajit repo, please use 'luajit' in subject prefix when formatting the patch (consider this patch[1] as an example). On 30.06.21, Mikhail Shishatskiy wrote: > When LJ_GC64 was enabled, the alignment of stack slots printed by > lj-stack command failed. It looked like the following: > > | 0x7ffff7fb0d90:0x7ffff7fb0dc0 [ ] 7 slots: Red zone > | 0x7ffff7fb0d88 [ M] > | 0x7ffff7fb0cc0:0x7ffff7fb0d80 [ ] 25 slots: Free stack slots > | 0x7ffff7fb0cb8 [ T ] We always try (at least) to describe the reason of such misbehaviour. You can consider this patch[2] as an example, but yours one don't need such deep investigation: the issue has been introduced from the very beginning[3]. PADDING variable is initialized to 11 even when LJ_GC64 mode is detected and the root cause is that this variable is set before the LuaJIT build mode is defined within function. Considering everything above, I propose to reword the part above to the following: | Since commit 58790750b9c4bd4c21d883f109ab552a2e202a15 ('gdb: introduce | luajit-gdb extension') when luajit-gdb.py is introduced, Lua stack | slots printed by lj-stack command are misaligned, when LJ_GC64 is | enabled. The issue is that PADDING variable used for aligning single | slots in the output is initialized before LuaJIT build mode is | defined in function, so the result was the following: | | | 0x7ffff7fb0d90:0x7ffff7fb0dc0 [ ] 7 slots: Red zone | | 0x7ffff7fb0d88 [ M] | | 0x7ffff7fb0cc0:0x7ffff7fb0d80 [ ] 25 slots: Free stack slots | | 0x7ffff7fb0cb8 [ T ] | | ... Mind ellipsis at the end, since the remaining slots are stripped. If you are OK with the part above, feel free to paste it as is into the next version of the patch. > > This patch fixes the output to look as expected: > > | 0x7ffff7fb0d90:0x7ffff7fb0dc0 [ ] 7 slots: Red zone > | 0x7ffff7fb0d88 [ M] > | 0x7ffff7fb0cc0:0x7ffff7fb0d80 [ ] 25 slots: Free stack slots > | 0x7ffff7fb0cb8 [ T ] > As for the change description, we also always try (at least) to describe the way the problem is fixed (look for this in the example[2] too). Considering your fix, I propose the rewording below: | In scope of this patch, PADDING initialization is also moved to | function right after all commands are loaded. As a result, the | output looks as expected: | | | 0x7ffff7fb0d90:0x7ffff7fb0dc0 [ ] 7 slots: Red zone | | 0x7ffff7fb0d88 [ M] | | 0x7ffff7fb0cc0:0x7ffff7fb0d80 [ ] 25 slots: Free stack slots | | 0x7ffff7fb0cb8 [ T ] | | ... Mind ellipsis at the end, since the remaining slots are stripped. If you are also OK with the part above, feel free to paste it as is into the next version too. > To test changes do the following: Tests are vital, but unfortunately we have no tests for the extension. As Sergey mentioned in his review, the patch "body" is splitted into two parts by three dashes ('---'). * The part above is the commit message and it should contain only the polished description of the patch (in other words, everything you have written before the part with the test). * The part below is your blackboard/canvas/etc: you can put there any supplementary or auxiliary information, such as branch name, link to the issue (that is missing), test results (also missing). The test is neat, so feel free to put it to the bottom part. > > | $ gdb -q --args ./luajit -e 'print("QQ")' > | Reading symbols from ./luajit... > | lj-arch command initialized > | lj-tv command initialized > | lj-str command initialized > | lj-tab command initialized > | lj-stack command initialized > | lj-state command initialized > | lj-gc command initialized > | luajit-gdb.py is successfully loaded > | (gdb) lj-arch > | LJ_64: True, LJ_GC64: True > | (gdb) b lj_cf_print > | Breakpoint 1 at 0x298b0: file /mnt/WorkDisk/share/dev/__tarantool/lj-tarantool/src/lib_base.c, line 485. > | (gdb) r > | Starting program: /mnt/WorkDisk/share/dev/__tarantool/lj-tarantool/src/luajit -e print\(\"QQ\"\) > | > | Breakpoint 1, lj_cf_print (L=0x0) > | at /mnt/WorkDisk/share/dev/__tarantool/lj-tarantool/src/lib_base.c:485 > | 485 { > | (gdb) s > | 486 ptrdiff_t i, nargs = L->top - L->base; > | (gdb) lj-stack L > | 0x7ffff7fa8d90:0x7ffff7fa8dc0 [ ] 7 slots: Red zone > | 0x7ffff7fa8d88 [ M] > | 0x7ffff7fa8cc0:0x7ffff7fa8d80 [ ] 25 slots: Free stack slots > | 0x7ffff7fa8cb8 [ T ] > | 0x7ffff7fa8cb0 [ B ] VALUE: string "QQ" @ 0x7ffff7fb1178 > | 0x7ffff7fa8ca0:0x7ffff7fa8ca8 [ ] FRAME: [L] delta=2, fast function #29 > | 0x7ffff7fa8c90:0x7ffff7fa8c98 [ ] FRAME: [V] delta=2, Lua function @ 0x7ffff7fb1240, 0 upvalues, "=(command line)":0 > | 0x7ffff7fa8c80:0x7ffff7fa8c88 [ ] FRAME: [CP] delta=4, Lua function @ 0x7ffff7fb1240, 0 upvalues, "=(command line)":0 > | 0x7ffff7fa8c78 [ ] VALUE: C function @ 0x55555555c962 > | 0x7ffff7fa8c70 [ ] VALUE: light userdata @ 0x0 > | 0x7ffff7fa8c60:0x7ffff7fa8c68 [ ] FRAME: [CP] delta=2, C function @ 0x55555555df1c > | 0x7ffff7fa8c58:0x7ffff7fa8c60 [S ] FRAME: dummy L > > Fixes: tarantool/tarantool#5876 Consider Sergey comment regarding this. This is awkward, that there is a colon used in the template[4], but there is none in the real-world examples. Typo happens... > > Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5876 This is also the great candidate to be moved below '---'. > > --- > src/luajit-gdb.py | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > The patch itself looks fine and clear, so no comments regarding it. > diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py > index f1fd6230..720d989a 100644 > --- a/src/luajit-gdb.py > +++ b/src/luajit-gdb.py > @@ -160,7 +160,7 @@ LJ_FR2 = None > > LJ_GCVMASK = ((1 << 47) - 1) > > -PADDING = ' ' * len(':' + hex((1 << (47 if LJ_GC64 else 32)) - 1)) > +PADDING = None > > # }}} > > @@ -665,7 +665,7 @@ The command requires no args and dumps current GC stats: > )) > > def init(commands): > - global LJ_64, LJ_GC64, LJ_FR2 > + global LJ_64, LJ_GC64, LJ_FR2, PADDING > > # XXX Fragile: though connecting the callback looks like a crap but it > # respects both Python 2 and Python 3 (see #4828). > @@ -714,6 +714,8 @@ def init(commands): > for name, command in commands.items(): > command(name) > > + PADDING = ' ' * len(':' + hex((1 << (47 if LJ_GC64 else 32)) - 1)) > + > gdb.write('luajit-gdb.py is successfully loaded\n') > > def load(event=None): > -- > 2.32.0 > [1]: https://lists.tarantool.org/tarantool-patches/YOKuzcc%2FTy32JxVW@root/T/#t [2]: https://github.com/tarantool/luajit/commit/ad1d444 [3]: https://github.com/tarantool/luajit/commit/5879075 [4]: https://www.tarantool.io/en/doc/latest/dev_guide/developer_guidelines/#how-to-write-a-commit-message -- Best regards, IM