[Tarantool-patches] [PATCH 2/2] gdb: refactor iteration over frames while dumping stack
Sergey Kaplun
skaplun at tarantool.org
Sat Aug 20 09:28:02 MSK 2022
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
More information about the Tarantool-patches
mailing list