Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Ostanevich <sergos@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit v1] tools: fix luajit-gdb stack dump
Date: Thu, 22 Jul 2021 14:48:21 +0300	[thread overview]
Message-ID: <20210722114821.GM11494@tarantool.org> (raw)
In-Reply-To: <3E1848F9-799D-4A38-9E7C-CD218ADFDF54@tarantool.org>


Thanks for your review! TL;DR: I would rather leave the patch with no
changes, but please consider my comments regarding your alternative
approach for iterating through the guest stack.

On 22.07.21, Sergey Ostanevich wrote:
> Hi!
> Thanks for the patch!
> I would rather remove all mentions of ‘bottom loop’ as we named it at
> Intel. I'm not a big pro in python, still the ‘for’ loop works fine 
> for me:
> If we can unwind to the next frame - dump it (and further)
> >>> s=[1]
> >>> for i in s:
> ...   if (i==1):
> ...     s.append(2)
> ...     continue
> ...   print(i)
> ... 
> 2
> If it is the only one (dummy):
> >>> s=[3]
> >>> for i in s:
> ...   if (i==1):
> ...     s.append(2)
> ...     continue
> ...   print(i)
> ... 
> 3
> Does it make sense in this case?

Let's see what would we get for our case with guest stack unwinding. The
algorithm is described by Sergey in the comment near the loop. But
imagine, we are iterating (backwards) through [L->stack, L-top] segment
via for loop (as you suggest above). There is one condition to check
whether we have reached the bottom of the frame (dump framelink slot and
continue the same way you showed above). However, we need one more
condition to skip the second slot for the framelink in LJ_FR2 mode.  As
a result we have a loop that iterates through the stack with at least
two conditions to skip some slots or dump them in other way.
Furthermore, your proposal leads to much more complex change in the
extension, comparing to those made by Sergey.

As I've already said, postcondition loop is the classic way for this
case. Moreover, I googled "how to emulate a do-while loop in Python?"
and the second option is unrolling the first iteration[1]. If one
wonders what is the first option -- use "while-true" loop with the
conditional break at the end. I equally hate both, so I'm OK with the
one chosen by Sergey. If you prefer "while-true", I believe Sergey can
rewrite this part, but considering your LGTM below, I guess you're OK
also with the current solution. So, I'll push the patch in a jiffy.

> Anyways, it’s too much of nitpicking - LGTM in its current version.
> Regards,
> Sergos



[1]: https://stackoverflow.com/a/743186

Best regards,

  reply	other threads:[~2021-07-22 12:11 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
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 [this message]
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:

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

  git send-email \
    --in-reply-to=20210722114821.GM11494@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=sergos@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit v1] tools: fix luajit-gdb stack dump' \


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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox