Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit v1] tools: fix luajit-gdb stack dump
Date: Tue, 20 Jul 2021 15:29:52 +0300
Message-ID: <20210720122952.GH11494@tarantool.org> (raw)
In-Reply-To: <20210618062234.871-1-skaplun@tarantool.org>

Sergey,

Thanks for the patch! Nice catch, actually! I wonder, how much impact
this bug made while digging the crash reports. Anyway, LGTM, but
consider several nits below.

On 18.06.21, Sergey Kaplun wrote:
> When there is only one frame (so-called dummy frame) with a dummy thread
> on stack bottom at L->base - 1 - LJ_FR2, stack dump does not show stack
> slots from bottom to top, because the frame link is already pointed to
> stack bottom (dummy thread mentioned above). Its output looks like the

Too much information in a single sentence. I consider the following info
vital to understand the patch.
* Dummy frame is the particular and kinda "initial"  coroutine state,
  when the framelink slot (i.e. L->base - (1 + LJ_FR2)) is the most
  bottom slot of the guest stack (i.e. L->stack).
* Since coroutine stack unwinding is implemented via precondition loop,
  no slots are dumped from L->top to L->base in case when only dummy
  frame is on the stack.
* Python is a crap language, and seems like postcondition loop violates
  The Zen (this is too hard for users, isn't it?), so Guido decided not
  to introduce such construction in the language at all.

  #ifdef offtop
    I dare to guess, it was just too hard to distinguish precondition
    from postcondition in "colon&spaces" paradigm; glad Larry Wall
    decided to use braces for blocks, and Roberto just left Pascalish
    syntax for this. I'm upset with the GDB guys choice every time we're
    back to developing this extension.
  #endif

comment.yield()

> following:
> 
> | 0x7fb512ac40:0x7fb512ac70 [    ] 7 slots: Red zone
> | 0x7fb512ac38            [   M]
> | 0x7fb512ab28:0x7fb512ac30 [    ] 34 slots: Free stack slots
> | 0x7fb512ab20            [  T ]
> | 0x7fb512ab08:0x7fb512ab10 [S   ] FRAME: dummy L

Minor: I guess, you can align the output. At least I see no reason not
to do this.

> 

comment.resume()

Hence, there is no other option except unroll the first iteration and
reorder stack slots dump with moving framelinks downwards to the
coroutine stack bottom.

> This patch unrolled first loop iteration. Stack dump first inspect all
> fields for the top frame and then continue if there are other frames.
> After patch the output looks like the following:
> 
> | 0x7fb512ac40:0x7fb512ac70 [    ] 7 slots: Red zone
> | 0x7fb512ac38            [   M]
> | 0x7fb512ab28:0x7fb512ac30 [    ] 34 slots: Free stack slots
> | 0x7fb512ab20            [  T ]
> | 0x7fb512ab18            [    ] VALUE: string 0 "/tmp/net_box.lua:6: err in ser" @ 0x7fb512ade8
> | 0x7fb512ab10            [ B  ] VALUE: table @ 0x7fb512ac80 (asize: 0, hmask: 0x0)
> | 0x7fb512ab00:0x7fb512ab08 [S   ] FRAME: dummy L

Feel free to align the output here too.

> ---
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/fix-luajit-gdb
> 
> Steps to test behaviour:
> 0) Clone branch
> 1) Build tarantool with GC64 or not (I've tested both cases)
> 2) Run the following script </tmp/net_box.lua> by Tarantool via gdb.
> 
> | $ cat /tmp/netbox.lua
> | box.cfg{log_level = 50, listen = "127.0.0.1:3802"}
> | box.schema.user.grant('guest','read,write,execute','universe', nil, {if_not_exists=true})
> | local net_box = require"net.box"
> | local cn = net_box:connect("127.0.0.1:3802")
> | local function f()
> |   return setmetatable({}, {__serialize = function() error[[err in ser]] end})
> | end
> | _G.f = f
> | local r, st = cn:call("f")
> | print(r,st)
> 
> | $ gdb --args ./tarantool /tmp/netbox.lua
> | ...
> | (gdb) source ~/builds_workspace/luajit/fix-luajit-gdb/src/luajit-gdb.py
> | ...
> 
> 3) Set breakpoint to ./src/lua/msgpack.c:234, and run
> 
> | (gdb) b ./src/lua/msgpack.c:234
> 
> I'm not sure about your sources version, so you can just set breakpoin to
> mp_encode(), run and continue (need two entries). We are interested in
> the line #234:
> 
> | 233|.......if (luaL_tofield(L, cfg, opts, lua_gettop(L), &field) < 0)
> | 234|.......|.......return luaT_error(L);
> 
> And dump the stack:
> 
> | (gdb) lj-stack L
> | 0x7fb512ac40:0x7fb512ac70 [    ] 7 slots: Red zone
> | 0x7fb512ac38            [   M]
> | 0x7fb512ab28:0x7fb512ac30 [    ] 34 slots: Free stack slots
> | 0x7fb512ab20            [  T ]
> | 0x7fb512ab18            [    ] VALUE: string 0 "/tmp/net_box.lua:6: err in ser" @ 0x7fb512ade8
> | 0x7fb512ab10            [ B  ] VALUE: table @ 0x7fb512ac80 (asize: 0, hmask: 0x0)
> | 0x7fb512ab00:0x7fb512ab08 [S   ] FRAME: dummy L
> 
>  src/luajit-gdb.py | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
> index f1fd6230..a3dad585 100644
> --- a/src/luajit-gdb.py
> +++ b/src/luajit-gdb.py
> @@ -424,16 +424,26 @@ def dump_stack(L, base=None, top=None):
>          )
>      ]) + '\n'
>  
> +    # Set framelink for the top frame. Dump frame slots even for dummy frame.

It would be also great to drop a few words why the iteration is unrolled
(the lack of postcondition loops in Python) via XXX comment.

>      slot = top - 1
>      framelink = base - (1 + LJ_FR2)

I suggest to split this part into two blocks via blank line:
* variable initialization
* unrolled iteration

> +    while slot > framelink + LJ_FR2:
> +        dump += dump_stack_slot(L, slot, base, top)
> +        slot -= 1
>  
> +    # If there are other frames -- continue.

Again, this is not the particular reason. As for me the main reason you
can't implement stack unwinding via do-while construction.

>      while framelink > mref('TValue *', L['stack']):
> -        while slot > framelink + LJ_FR2:
> -            dump += dump_stack_slot(L, slot, base, top)
> -            slot -= 1
> +        assert slot == framelink + LJ_FR2, "Invalid slot during frame unwind"

Please, use parentheses for assert call.

>          dump += dump_framelink(L, framelink)
>          framelink = frame_prev(framelink + LJ_FR2) - LJ_FR2
>          slot -= 1 + LJ_FR2
> +        while slot > framelink + LJ_FR2:
> +            dump += dump_stack_slot(L, slot, base, top)
> +            slot -= 1
> +
> +    assert slot == framelink + LJ_FR2, "Invalid slot after frame unwind"

Ditto.

> +    # Skip nil slot for the last frame for 2-slot frames.
> +    slot -= LJ_FR2
>  
>      dump += '{fr}{padding} [S   ] FRAME: dummy L'.format(
>          fr = slot,
> -- 
> 2.31.0
> 

-- 
Best regards,
IM

  parent reply	other threads:[~2021-07-20 12:53 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-18  6:22 Sergey Kaplun via Tarantool-patches
2021-07-05  7:03 ` Sergey Kaplun via Tarantool-patches
2021-07-20 12:29 ` Igor Munkin via Tarantool-patches [this message]
2021-07-21  8:49   ` Sergey Kaplun via Tarantool-patches
2021-07-21  8:54     ` Igor Munkin via Tarantool-patches
2021-07-22 10:32       ` Sergey Ostanevich via Tarantool-patches
2021-07-22 11:48         ` Igor Munkin via Tarantool-patches
2021-07-22 12:51           ` Sergey Ostanevich via Tarantool-patches
2021-07-21  9:02     ` Sergey Kaplun via Tarantool-patches
2021-07-22 13:34 ` Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210720122952.GH11494@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=skaplun@tarantool.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git