* Re: [Tarantool-patches] [PATCH] luajit-gdb: fix misalignment in lj-stack
2021-06-30 14:20 [Tarantool-patches] [PATCH] luajit-gdb: fix misalignment in lj-stack Mikhail Shishatskiy via Tarantool-patches
@ 2021-07-05 7:28 ` Sergey Kaplun via Tarantool-patches
2021-07-05 7:50 ` Sergey Kaplun via Tarantool-patches
2021-07-05 9:18 ` Igor Munkin via Tarantool-patches
1 sibling, 1 reply; 4+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-07-05 7:28 UTC (permalink / raw)
To: Mikhail Shishatskiy; +Cc: tarantool-patches
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
>
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Tarantool-patches] [PATCH] luajit-gdb: fix misalignment in lj-stack
2021-06-30 14:20 [Tarantool-patches] [PATCH] luajit-gdb: fix misalignment in lj-stack Mikhail Shishatskiy via Tarantool-patches
2021-07-05 7:28 ` Sergey Kaplun via Tarantool-patches
@ 2021-07-05 9:18 ` Igor Munkin via Tarantool-patches
1 sibling, 0 replies; 4+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-07-05 9:18 UTC (permalink / raw)
To: Mikhail Shishatskiy; +Cc: 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 <init> 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 <init> 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
| <init> 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
^ permalink raw reply [flat|nested] 4+ messages in thread