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

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

Misha,

Thanks for the patch! Please consider my comments below.

On 23.07.22, Mikhail Elhimov via Tarantool-patches wrote:
> Changes:

Minor: You can omit "Changes:" here; just list them :)

> - 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
> - Framelink always refers to the slot right before frame's base

Side note: These three bullets above are simply great, thanks a lot!

> - Introduce constant LJ_FRAMELINK_NUM_SLOTS to make slot arithmetic
>   more obvious. Previous using of boolean LJ_RT2 in arithmetic looked
>   like a tricky way, both to read and maintain (however places where
>   LJ_RT2 approach had been originally replicated from the source code
>   are left intact, because it is expected that maintenance efforts for
>   them will be minimal)

Well, I'm also see no sense in introducing this variable.
1. It doesn't exist in LuaJIT sources.
2. It's simply LJ_FR2 + 1 (you can convert LJ_FR2 from the boolean
   context to the numeric one, if you want)
3. It is required only in a single place: <frames> function. Another
   place where it is used is <get_framelink_sentinel> but as Sergey
   already mentioned, LJ_FR2 fits much better there.

I suggest to stay as close as possible to LuaJIT defines and LuaJIT code
base, since this extension also violates DRY approach, that is already
ignored almost everywhere in LuaJIT codebase.

All in all, I see no reason to introduce a new variable for a single
case, and propose to continue using LJ_FR2 for such cases.

> ---
>  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
>  LJ_GC64 = None
>  LJ_FR2 = None
>  LJ_DUALNUM = None
> +LJ_FRAMELINK_NUM_SLOTS = None

Considering the comment right above, this can be dropped.

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

IMHO, you can move this into Frames section with the name kinda
<frame_sentinel>. And use LJ_FR2 here to make it a oneliner.

> +
> +# Generator that implements frame iterator
> +# every frame is represented as a tuple of framelink and frametop
> +def frames(L):
> +    frametop = L['top']
> +    framelink = L['base'] - 1
> +    framelink_sentinel = get_framelink_sentinel(L)
> +    while framelink is not None:
> +        yield framelink, frametop
> +        frametop = framelink - LJ_FRAMELINK_NUM_SLOTS
> +        framelink = frame_prev(framelink) if framelink > framelink_sentinel else None

Side note: I've seen the newest version.

> +
>  # 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

Minor: It's better to split this line into two (like it's done for
<frame_ftsz> or <frame_pc>). Feel free to ignore, since I was just
confused whether PADDING is added only to the else part or to the whole
if-else result.

> +
> +def dump_framelink_func(fr):
> +    return dump_lj_tfunc(fr - 1 if LJ_FR2 else fr)

Oh.. Just use fr - LJ_FR2 in the call below, please: there is no need to
introduce a function to be used once.

> +
>  def dump_framelink(L, fr):
> -    fr2 = fr + LJ_FR2
> -
> -    return '{fr}{padding} [    ] FRAME: [{pp}] delta={d}, {f}\n'.format(
> -        fr = fr,
> -        padding = ':{fr2}'.format(fr2 = fr2) if LJ_FR2 else PADDING,
> -        pp = 'PP' if frame_ispcall(fr2) else '{frname}{p}'.format(
> -            frname = frametypes(int(frame_type(fr2))),
> -            p = 'P' if frame_typep(fr2) & FRAME_P else ''
> +    if fr == get_framelink_sentinel(L):
> +        return '{addr} [S   ] FRAME: dummy L'.format(
> +            addr = dump_framelink_slot_address(fr),
> +        )
> +    return '{addr} [    ] FRAME: [{pp}] delta={d}, {f}'.format(
> +        addr = dump_framelink_slot_address(fr),
> +        pp = 'PP' if frame_ispcall(fr) else '{frname}{p}'.format(
> +            frname = frametypes(int(frame_type(fr))),
> +            p = 'P' if frame_typep(fr) & FRAME_P else ''
>          ),
> -        d = cast('TValue *', fr2) - cast('TValue *', frame_prev(fr2)),
> -        f = dump_lj_tfunc(fr),
> +        d = cast('TValue *', fr) - cast('TValue *', frame_prev(fr)),
> +        f = dump_framelink_func(fr),

Considering the comment right above, <dump_lj_tfunc> can be used here.

>      )
>  

<snipped>

> @@ -717,7 +713,7 @@ The command requires no args and dumps current GC stats:
>          ))
>  
>  def init(commands):
> -    global LJ_64, LJ_GC64, LJ_FR2, LJ_DUALNUM, LJ_TISNUM, PADDING
> +    global LJ_64, LJ_GC64, LJ_FR2, LJ_DUALNUM, LJ_FRAMELINK_NUM_SLOTS, LJ_TISNUM, PADDING

Considering the comment at the beginning, this part can be dropped.

>  
>      # XXX Fragile: though connecting the callback looks like a crap but it
>      # respects both Python 2 and Python 3 (see #4828).
> @@ -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
>      except:
>          gdb.write('luajit-gdb.py failed to load: '
>                    'no debugging symbols found for libluajit\n')
> -- 
> 2.34.1
> 

-- 
Best regards,
IM