Re: [Tarantool-patches] [PATCH 2/2] gdb: refactor iteration over frames while dumping stack

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

Hi, Mikhail!

Thanks for the patch!

It's generally LGTM, except a bunch of nits and typos.
BTW, I like your iterator approach: the code looks so much better now!

On 23.07.22, Mikhail Elhimov via Tarantool-patches wrote:
> Changes:
> - Introduce generator to iterate over frames in a Python way
> - Dump framelink of the bottom frame in a common way and identify it as
>   a dummy framelink only inside of common dump function

Typo: s/of common/of the common/

> - Framelink always refers to the slot right before frame's base

Don't get this change bullet description. Looks like this is just the
way you implement frame iterator, isn't it?

> - Introduce constant LJ_FRAMELINK_NUM_SLOTS to make slot arithmetic
>   more obvious. Previous using of boolean LJ_RT2 in arithmetic looked

Typo: s/LJ_RT2/LJ_FR2

>   like a tricky way, both to read and maintain (however places where

Side note: As for me it looks like obvious and natural :). But maybe
this is the ill effect from LuaJIT sources. I leave comments in places
where I suggest to use LJ_FR2 instead LJ_FRAMELINK_NUM_SLOTS.

>   LJ_RT2 approach had been originally replicated from the source code

Typo: s/LJ_RT2/LJ_FR2

>   are left intact, because it is expected that maintenance efforts for
>   them will be minimal)

General: Missed dot at the end of the sentences.

> ---
>  src/luajit-gdb.py | 101 ++++++++++++++++++++++------------------------
>  1 file changed, 49 insertions(+), 52 deletions(-)
> 
> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
> index 1e9a96fb..d7d34c70 100644
> --- a/src/luajit-gdb.py
> +++ b/src/luajit-gdb.py
> @@ -158,6 +158,7 @@ LJ_64 = None

<snipped>

> @@ -299,6 +300,21 @@ gclen = {
>      'mmudata': gcringlen,
>  }
>  
> +def get_framelink_sentinel(L):
> +    stack = mref('TValue *', L['stack'])
> +    return stack + LJ_FRAMELINK_NUM_SLOTS - 1

`LJ_FRAMELINK_NUM_SLOTS - 1 == LJ_FR2` so we can replace it like the
following:
| return stack + LJ_FR2

> +
> +# Generator that implements frame iterator

Typo: s/Generator/The generator/
Typo: s/iterator/iterator./

> +# every frame is represented as a tuple of framelink and frametop

Typo: s/every/Every/
Typo: s/frametop/frametop./

Minor: I suggest to specify that this is yielded to the caller tuple.
Feel free to ignore.


> +def frames(L):
> +    frametop = L['top']
> +    framelink = L['base'] - 1
> +    framelink_sentinel = get_framelink_sentinel(L)
> +    while framelink is not None:

Minor: `is not None` is excess, we can just omit it.

> +        yield framelink, frametop
> +        frametop = framelink - LJ_FRAMELINK_NUM_SLOTS
> +        framelink = frame_prev(framelink) if framelink > framelink_sentinel else None
> +
>  # Dumpers {{{
>  
>  def dump_lj_tnil(tv):
> @@ -397,32 +413,38 @@ dumpers = {
>  def dump_tvalue(tvalue):
>      return dumpers.get(typenames(itypemap(tvalue)), dump_lj_invalid)(tvalue)
>  
> +def dump_framelink_slot_address(fr):
> +    return '{}:{}'.format(fr - 1, fr) if LJ_FR2 else '{}'.format(fr) + PADDING
> +
> +def dump_framelink_func(fr):
> +    return dump_lj_tfunc(fr - 1 if LJ_FR2 else fr)

We can replace it with the following:
| return dump_lj_tfunc(fr - LJ_FR2)


> +
>  def dump_framelink(L, fr):

<snipped>

>      )
>  
> -def dump_stack_slot(L, slot, base=None, top=None, eol='\n'):
> +def dump_stack_slot(L, slot, base=None, top=None):

<snipped>

>  
>  def dump_stack(L, base=None, top=None):

<snipped>

> +    for framelink, frametop in frames(L):
> +        # dump all data slots in the (framelink, top) interval

Typo: s/dump/Dump/
Typo: s/interval/interval./

> +        dump.extend([
> +            dump_stack_slot(L, framelink + offset, base, top)
> +                for offset in range(frametop - framelink, 0, -1)
> +        ])
> +        # dump frame slot (2 slots in case of GC64)

Typo: s/dump/Dump/
Missed dot at the end of the sentence.

> +        dump.append( dump_framelink(L, framelink) )
> +
> +    return '\n'.join(dump)
>  
>  def dump_gc(g):
>      gc = g['gc']
> @@ -717,7 +713,7 @@ The command requires no args and dumps current GC stats:

<snipped>

> @@ -759,6 +755,7 @@ def init(commands):
>          LJ_64 = str(gdb.parse_and_eval('IRT_PTR')) == 'IRT_P64'
>          LJ_FR2 = LJ_GC64 = str(gdb.parse_and_eval('IRT_PGC')) == 'IRT_P64'
>          LJ_DUALNUM = gdb.lookup_global_symbol('lj_lib_checknumber') is not None
> +        LJ_FRAMELINK_NUM_SLOTS = 2 if LJ_FR2 else 1

As far as LJ_FR2 is defined before, we can initialize this variable like
the following:
| LJ_FRAMELINK_NUM_SLOTS = LJ_FR2 + 1

>      except:
>          gdb.write('luajit-gdb.py failed to load: '
>                    'no debugging symbols found for libluajit\n')
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun