Re: [Tarantool-patches] [PATCH] luajit-gdb: fix misalignment in lj-stack

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, again!

Sorry, one more nit to the commit message.

On 05.07.21, Sergey Kaplun via Tarantool-patches wrote:
> Hi!
> 
> Thanks for the patch!
> LGTM, except several nits to the commit message.
> 
> On 30.06.21, Mikhail Shishatskiy wrote:
> 
> | luajit-gdb: fix misalignment in lj-stack
> 
> Please use just
> | gdb: fix misalignment in lj-stack for LJ_GC64
> "tag" (you can check all tags types in the git log).
> 
> > When LJ_GC64 was enabled, the alignment of stack slots printed by
> > lj-stack command failed. It looked like the following:
> 
> Nit: I suggest reformulate it like the following:
> 
> | When LJ_GC64 is enabled, stack slots printed by lj-stack command
> | are misaligned.
> 
> > 
> > | 0x7ffff7fb0d90:0x7ffff7fb0dc0 [    ] 7 slots: Red zone
> > | 0x7ffff7fb0d88            [   M]
> > | 0x7ffff7fb0cc0:0x7ffff7fb0d80 [    ] 25 slots: Free stack slots
> > | 0x7ffff7fb0cb8            [  T ]
> 
> Nit: please describe the reason of this issue
> 
> > 
> > This patch fixes the output to look as expected:
> 
> Nit: by how?
> 
> > 
> > | 0x7ffff7fb0d90:0x7ffff7fb0dc0 [    ] 7 slots: Red zone
> > | 0x7ffff7fb0d88                [   M]
> > | 0x7ffff7fb0cc0:0x7ffff7fb0d80 [    ] 25 slots: Free stack slots
> > | 0x7ffff7fb0cb8                [  T ]
> > 
> > To test changes do the following:
> > 
> > | $ 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
> 
> Thanks for the test case! Please delete it from commit message (it's
> redundant there, I suppose). You can mention it in the mail itself after
> "---" before the diff entry [*] (see below).
> 
> > 
> > Fixes: tarantool/tarantool#5876

Nit: Don't use ":" in the ticket-mentioning, just:

| Fixes tarantool/tarantool#5876

> > 
> > Branch: https://github.com/tarantool/luajit/tree/shishqa/gh-5876
> 
> This too [*] (especially it is not a part of the commit message).
> 
> > 
> > ---
> 
> <<< [*] Exactly here.
> 
> >  src/luajit-gdb.py | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > 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
> 
> <snipped>
> 
> > -- 
> > 2.32.0
> > 
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
Sergey Kaplun