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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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