Hi, Sergey!

Thanks for the review!

On 20.08.2022 09:28, Sergey Kaplun wrote:
```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/`
Fixed
```- 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?```

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)"

```- 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`
Fixed
```  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

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 ))

```  LJ_RT2 approach had been originally replicated from the source code
```
`Typo: s/LJ_RT2/LJ_FR2`
Fixed
```  are 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,
}

+    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./```
Fixed
```+# every frame is represented as a tuple of framelink and frametop
```
```Typo: s/every/Every/
Typo: s/frametop/frametop./```
Fixed
```Minor: I suggest to specify that this is yielded to the caller tuple.
Feel free to ignore.```
Sorry, I didn't catch the suggestion. Would you, please, explain?
```+def frames(L):
+    frametop = L['top']
+    framelink = L['base'] - 1
+    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 )

```+        yield framelink, frametop
+
# 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)

+    return '{}:{}'.format(fr - 1, fr) if LJ_FR2 else '{}'.format(fr) + PADDING
+
+    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)```
```+
```
```<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./```
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.```
Fixed
```+        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:
'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']
-    while framelink is not None:
+    while True:
+            break

# Dumpers {{{

@@ -477,12 +479,12 @@ def dump_stack(L, base=None, top=None):
])

-        # 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).
```--