Hi, Sergey!
Thanks for the review!
FixedHi, 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 functionTypo: s/of common/of the common/
- Framelink always refers to the slot right before frame's baseDon't get this change bullet description. Looks like this is just the way you implement frame iterator, isn't it?
Fixed
Yes, this bullet described both "what" and "how" of the change. I
turned this bullet into "Iterate over frames in a Python way (with
iterator)"
Fixed- Introduce constant LJ_FRAMELINK_NUM_SLOTS to make slot arithmetic more obvious. Previous using of boolean LJ_RT2 in arithmetic lookedTypo: s/LJ_RT2/LJ_FR2
like a tricky way, both to read and maintain (however places whereSide 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.
My point is: LJ_FR2 -- for boolean context,
LJ_FRAMELINK_NUM_SLOTS -- for formulas
Let me share my considerations.
I see LJ_FR2 as a boolean entity that means either 1 or 2 slots
are used to store "framelink" and since it is boolean, the only
natural usage for it from my point of view is boolean context
(i.e. if ... else ...). I understand that using boolean in
formulas reduces branching and helps to improve performance, but
here it doesn't seem to be necessary. For me operating with
"number of slots" is easier than with "number of extra slots over
1" (which LJ_FR2 turns into in formulas). The latter is ok when
it's standalone, but when it appears in formula I feel that I need
to perform some additional calculation in my mind while reading
code, that's why I find it less-readable and introduced
LJ_FRAMELINK_NUM_SLOTS and that's why I would keep using
LJ_FRAMELINK_NUM_SLOTS in formulas, and wouldn't turn back LJ_FR2
even when `LJ_FRAMELINK_NUM_SLOTS - 1` occurs
An example:
red = 5 + 2 * LJ_FR2
which looks a bit magic, turns into
red = 3 + 2 * LJ_FRAMELINK_NUM_SLOTS
which produce the same results, but literally says that we reserve 3 data slots for 2 frames (I just realized that I forgot to ask the confirmation regarding my interpretation of the number of slots reserved and left the original formula intact, so if it's correct I'd adjust it)
These considerations may be changed when I dive deeper into
luajit, but for now they are ))
FixedLJ_RT2 approach had been originally replicated from the source codeTypo: s/LJ_RT2/LJ_FR2
Fixedare left intact, because it is expected that maintenance efforts for them will be minimal)General: Missed dot at the end of the sentences.
Fixed--- 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 iteratorTypo: s/Generator/The generator/ Typo: s/iterator/iterator./
Fixed+# every frame is represented as a tuple of framelink and frametopTypo: s/every/Every/ Typo: s/frametop/frametop./
Sorry, I didn't catch the suggestion. Would you, please, explain?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.
I'd prefer to follow the recommendation from PEP8
=========
Also, beware of writing if x when you really mean if x is not
None – e.g. when testing whether a variable or argument that
defaults to None was set to some other value. The other value
might have a type (such as a container) that could be false in a
boolean context!
=========
However while checking this code I found 2-step loop exit: first, inside the loop we check that sentinel is achieved, assign `None` and catch it immediately at checking loop condition. I adjusted loop as follows:
while True:
yield framelink, frametop
frametop = framelink - LJ_FRAMELINK_NUM_SLOTS
if framelink <= framelink_sentinel:
break
framelink = frame_prev(framelink)
so finally `is not None` was removed, but for another reason )
Please, see my consideration above+ 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)
Fixed+ 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) intervalTypo: s/dump/Dump/ Typo: s/interval/interval./
Fixed+ 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.
Please, see my consideration above+ 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 1As 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
The corrected commit message: =================================================== gdb: refactor iteration over frames while dumping stack Changes: - Iterate over frames in a Python way (with iterator) - Dump framelink of the bottom frame in a common way and identify it as a dummy framelink only inside of the common dump function - Framelink always refers to the slot right before frame's base - Introduce constant LJ_FRAMELINK_NUM_SLOTS to make slot arithmetic more obvious. Previous using of boolean LJ_FR2 in arithmetic looked like a tricky way, both to read and maintain (however places where LJ_FR2 approach had been originally replicated from the source code are left intact, because it is expected that maintenance efforts for them will be minimal). =================================================== diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py index d7d34c70..2c11057b 100644 --- a/src/luajit-gdb.py +++ b/src/luajit-gdb.py @@ -304,16 +304,18 @@ def get_framelink_sentinel(L): stack = mref('TValue *', L['stack']) return stack + LJ_FRAMELINK_NUM_SLOTS - 1 -# Generator that implements frame iterator -# every frame is represented as a tuple of framelink and frametop +# The 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: + while True: yield framelink, frametop frametop = framelink - LJ_FRAMELINK_NUM_SLOTS - framelink = frame_prev(framelink) if framelink > framelink_sentinel else None + if framelink <= framelink_sentinel: + break + framelink = frame_prev(framelink) # Dumpers {{{ @@ -477,12 +479,12 @@ def dump_stack(L, base=None, top=None): ]) for framelink, frametop in frames(L): - # dump all data slots in the (framelink, top) interval + # Dump all data slots in the (framelink, top) 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) + # Dump frame slot (2 slots in case of GC64). dump.append( dump_framelink(L, framelink) ) return '\n'.join(dump) ===================================================
-- Best regards, Mikhail Elhimov