[Tarantool-patches] [PATCH 0/2] gdb: adjust to support Python 2 + refactoring of stack dumping

[Tarantool-patches] [PATCH 0/2] gdb: adjust to support Python 2 + refactoring of stack dumping

[Mikhail Elhimov via Tarantool-patches]

https://github.com/tarantool/luajit/tree/elhimov/gdb_extension_support_python2

This patch fixes https://github.com/tarantool/tarantool/issues/7458 and also applies refactoring of stack dumping

Mikhail Elhimov (2):
  gdb: adjust to support python2 (centos 7)
  gdb: refactor iteration over frames while dumping stack

 src/luajit-gdb.py | 119 +++++++++++++++++++++++-----------------------
 1 file changed, 59 insertions(+), 60 deletions(-)

-- 
2.34.1

[Tarantool-patches] [PATCH 1/2] gdb: adjust to support python2 (centos 7)

[Mikhail Elhimov via Tarantool-patches]

Stop using unpacking arguments within list initialization as python2
only supports it in function call

fixes tarantool/luajit#7458
---
 src/luajit-gdb.py | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
index baf66f66..1e9a96fb 100644
--- a/src/luajit-gdb.py
+++ b/src/luajit-gdb.py
@@ -432,15 +432,16 @@ def dump_stack(L, base=None, top=None):
     maxstack = mref('TValue *', L['maxstack'])
     red = 5 + 2 * LJ_FR2
 
-    dump = '\n'.join([
-        '{padding} Red zone: {nredslots: >2} slots {padding}'.format(
-            padding = '-' * len(PADDING),
-            nredslots = red,
-        ),
-        *(
-            dump_stack_slot(L, maxstack + offset, base, top, '')
-                for offset in range(red, 0, -1)
-        ),
+    dump = []
+    dump.append('{padding} Red zone: {nredslots: >2} slots {padding}'.format(
+        padding = '-' * len(PADDING),
+        nredslots = red,
+    ))
+    dump.extend([
+        dump_stack_slot(L, maxstack + offset, base, top, '')
+            for offset in range(red, 0, -1)
+    ])
+    dump.extend([
         '{padding} Stack: {nstackslots: >5} slots {padding}'.format(
             padding = '-' * len(PADDING),
             nstackslots = int((tou64(maxstack) - tou64(stack)) >> 3),
@@ -451,7 +452,8 @@ def dump_stack(L, base=None, top=None):
             end = strx64(maxstack - 1),
             nfreeslots = int((tou64(maxstack) - tou64(top) - 8) >> 3),
         ),
-    ]) + '\n'
+    ])
+    dump = '\n'.join(dump) + '\n'
 
     slot = top
     framelink = base - (1 + LJ_FR2)
-- 
2.34.1

Re: [Tarantool-patches] [PATCH 1/2] gdb: adjust to support python2 (centos 7)

[Sergey Kaplun via Tarantool-patches]

Hi, Mikhail!

Thanks for the patch!
Sorry, for such long waiting.
It's LGTM, except a few stylish comments regarding the commit message.

On 23.07.22, Mikhail Elhimov via Tarantool-patches wrote:
> Stop using unpacking arguments within list initialization as python2

Typo: s/list initialization/the list initialization/
Also, you may mention that this problem affected Lua stack dump.

> only supports it in function call

Typo: s/function call/a function call/

Missed dot at the end of the sentence.

> 
> fixes tarantool/luajit#7458

Typo:
Fixes tarantool/tarantool#7458

> ---
>  src/luajit-gdb.py | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
> index baf66f66..1e9a96fb 100644
> --- a/src/luajit-gdb.py
> +++ b/src/luajit-gdb.py

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH 1/2] gdb: adjust to support python2 (centos 7)

[Mikhail Elhimov via Tarantool-patches]

Hi, Sergey!

Thanks for the review!

On 20.08.2022 09:26, Sergey Kaplun wrote:
> Hi, Mikhail!
>
> Thanks for the patch!
> Sorry, for such long waiting.
> It's LGTM, except a few stylish comments regarding the commit message.
>
> On 23.07.22, Mikhail Elhimov via Tarantool-patches wrote:
>> Stop using unpacking arguments within list initialization as python2
> Typo: s/list initialization/the list initialization/
> Also, you may mention that this problem affected Lua stack dump.
Fixed
>
>> only supports it in function call
> Typo: s/function call/a function call/
>
> Missed dot at the end of the sentence.
Fixed
>
>> fixes tarantool/luajit#7458
> Typo:
> Fixes tarantool/tarantool#7458
Fixed
>
>> ---
>>   src/luajit-gdb.py | 22 ++++++++++++----------
>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
>> index baf66f66..1e9a96fb 100644
>> --- a/src/luajit-gdb.py
>> +++ b/src/luajit-gdb.py
> <snipped>
>
>> -- 
>> 2.34.1
>>
The corrected commit message:
=================================================
gdb: adjust to support python2 (centos 7)

Stop using unpacking arguments within the list initialization as python2
only supports it in a function call (it affected Lua stack dump).

Fixes tarantool/tarantool#7458
=================================================

-- 
Best regards,
Mikhail Elhimov

Re: [Tarantool-patches] [PATCH 1/2] gdb: adjust to support python2 (centos 7)

[Mikhail Elhimov via Tarantool-patches]

Hi, Sergey!

Thanks for the review!

On 20.08.2022 09:26, Sergey Kaplun wrote:
> Hi, Mikhail!
>
> Thanks for the patch!
> Sorry, for such long waiting.
> It's LGTM, except a few stylish comments regarding the commit message.
>
> On 23.07.22, Mikhail Elhimov via Tarantool-patches wrote:
>> Stop using unpacking arguments within list initialization as python2
> Typo: s/list initialization/the list initialization/
> Also, you may mention that this problem affected Lua stack dump.
Fixed
>
>> only supports it in function call
> Typo: s/function call/a function call/
>
> Missed dot at the end of the sentence.
Fixed
>
>> fixes tarantool/luajit#7458
> Typo:
> Fixes tarantool/tarantool#7458
Fixed
>
>> ---
>>   src/luajit-gdb.py | 22 ++++++++++++----------
>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
>> index baf66f66..1e9a96fb 100644
>> --- a/src/luajit-gdb.py
>> +++ b/src/luajit-gdb.py
> <snipped>
>
>> -- 
>> 2.34.1
>>
The corrected commit message:
=================================================
gdb: adjust to support python2 (centos 7)

Stop using unpacking arguments within the list initialization as python2
only supports it in a function call (it affected Lua stack dump).

Fixes tarantool/tarantool#7458
=================================================

-- 
Best regards,
Mikhail Elhimov

Re: [Tarantool-patches] [PATCH 1/2] gdb: adjust to support python2 (centos 7)

[Igor Munkin via Tarantool-patches]

Misha,

Thanks for the patch! Almost LGTM after your fixes for Sergey comments:
please, consider the last nit, I found.

On 23.07.22, Mikhail Elhimov via Tarantool-patches wrote:
> Stop using unpacking arguments within list initialization as python2
> only supports it in function call
> 
> fixes tarantool/luajit#7458
> ---
>  src/luajit-gdb.py | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
> index baf66f66..1e9a96fb 100644
> --- a/src/luajit-gdb.py
> +++ b/src/luajit-gdb.py
> @@ -432,15 +432,16 @@ def dump_stack(L, base=None, top=None):
>      maxstack = mref('TValue *', L['maxstack'])
>      red = 5 + 2 * LJ_FR2
>  
> -    dump = '\n'.join([
> -        '{padding} Red zone: {nredslots: >2} slots {padding}'.format(
> -            padding = '-' * len(PADDING),
> -            nredslots = red,
> -        ),
> -        *(
> -            dump_stack_slot(L, maxstack + offset, base, top, '')
> -                for offset in range(red, 0, -1)
> -        ),
> +    dump = []

Please move this line to the "definition" section above to save the
original structure.

> +    dump.append('{padding} Red zone: {nredslots: >2} slots {padding}'.format(
> +        padding = '-' * len(PADDING),
> +        nredslots = red,
> +    ))
> +    dump.extend([
> +        dump_stack_slot(L, maxstack + offset, base, top, '')
> +            for offset in range(red, 0, -1)
> +    ])
> +    dump.extend([
>          '{padding} Stack: {nstackslots: >5} slots {padding}'.format(
>              padding = '-' * len(PADDING),
>              nstackslots = int((tou64(maxstack) - tou64(stack)) >> 3),
> @@ -451,7 +452,8 @@ def dump_stack(L, base=None, top=None):
>              end = strx64(maxstack - 1),
>              nfreeslots = int((tou64(maxstack) - tou64(top) - 8) >> 3),
>          ),
> -    ]) + '\n'
> +    ])
> +    dump = '\n'.join(dump) + '\n'
>  
>      slot = top
>      framelink = base - (1 + LJ_FR2)
> -- 
> 2.34.1
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 1/2] gdb: adjust to support python2 (centos 7)

[Mikhail Elhimov via Tarantool-patches]

Hi, Igor!

Thanks for the review!

On 31.08.2022 17:47, Igor Munkin wrote:
> Misha,
>
> Thanks for the patch! Almost LGTM after your fixes for Sergey comments:
> please, consider the last nit, I found.
>
> On 23.07.22, Mikhail Elhimov via Tarantool-patches wrote:
>> Stop using unpacking arguments within list initialization as python2
>> only supports it in function call
>>
>> fixes tarantool/luajit#7458
>> ---
>>   src/luajit-gdb.py | 22 ++++++++++++----------
>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
>> index baf66f66..1e9a96fb 100644
>> --- a/src/luajit-gdb.py
>> +++ b/src/luajit-gdb.py
>> @@ -432,15 +432,16 @@ def dump_stack(L, base=None, top=None):
>>       maxstack = mref('TValue *', L['maxstack'])
>>       red = 5 + 2 * LJ_FR2
>>   
>> -    dump = '\n'.join([
>> -        '{padding} Red zone: {nredslots: >2} slots {padding}'.format(
>> -            padding = '-' * len(PADDING),
>> -            nredslots = red,
>> -        ),
>> -        *(
>> -            dump_stack_slot(L, maxstack + offset, base, top, '')
>> -                for offset in range(red, 0, -1)
>> -        ),
>> +    dump = []
> Please move this line to the "definition" section above to save the
> original structure.

Fixed.

I personally prefer to do similar operations (populating of `dump` list) 
in a similar way (call appropriate "populating" method). For me 
populating-over-initialization is not the "similar" way here, and I'd 
use it here only if I could initialize whole list at once (as it was 
before). Anyway, if you find that it saves the original structure then 
let it be )

>> +    dump.append('{padding} Red zone: {nredslots: >2} slots {padding}'.format(
>> +        padding = '-' * len(PADDING),
>> +        nredslots = red,
>> +    ))
>> +    dump.extend([
>> +        dump_stack_slot(L, maxstack + offset, base, top, '')
>> +            for offset in range(red, 0, -1)
>> +    ])
>> +    dump.extend([
>>           '{padding} Stack: {nstackslots: >5} slots {padding}'.format(
>>               padding = '-' * len(PADDING),
>>               nstackslots = int((tou64(maxstack) - tou64(stack)) >> 3),
>> @@ -451,7 +452,8 @@ def dump_stack(L, base=None, top=None):
>>               end = strx64(maxstack - 1),
>>               nfreeslots = int((tou64(maxstack) - tou64(top) - 8) >> 3),
>>           ),
>> -    ]) + '\n'
>> +    ])
>> +    dump = '\n'.join(dump) + '\n'
>>   
>>       slot = top
>>       framelink = base - (1 + LJ_FR2)
>> -- 
>> 2.34.1
>>
===================================================

diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
index baf66f66..9846f3a2 100644
--- a/src/luajit-gdb.py
+++ b/src/luajit-gdb.py
@@ -432,15 +432,17 @@ def dump_stack(L, base=None, top=None):
      maxstack = mref('TValue *', L['maxstack'])
      red = 5 + 2 * LJ_FR2

-    dump = '\n'.join([
+    dump = [
          '{padding} Red zone: {nredslots: >2} slots {padding}'.format(
              padding = '-' * len(PADDING),
              nredslots = red,
          ),
-        *(
-            dump_stack_slot(L, maxstack + offset, base, top, '')
-                for offset in range(red, 0, -1)
-        ),
+    ]
+    dump.extend([
+        dump_stack_slot(L, maxstack + offset, base, top, '')
+            for offset in range(red, 0, -1)
+    ])
+    dump.extend([
          '{padding} Stack: {nstackslots: >5} slots {padding}'.format(
              padding = '-' * len(PADDING),
              nstackslots = int((tou64(maxstack) - tou64(stack)) >> 3),
@@ -451,7 +453,8 @@ def dump_stack(L, base=None, top=None):
              end = strx64(maxstack - 1),
              nfreeslots = int((tou64(maxstack) - tou64(top) - 8) >> 3),
          ),
-    ]) + '\n'
+    ])
+    dump = '\n'.join(dump) + '\n'

      slot = top
      framelink = base - (1 + LJ_FR2)

===================================================

-- 
Best regards,
Mikhail Elhimov

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

[Mikhail Elhimov via Tarantool-patches]

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
- 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_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)
---
 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
 
 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
+
+# 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
+
 # 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)
+
 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),
     )
 
-def dump_stack_slot(L, slot, base=None, top=None, eol='\n'):
+def dump_stack_slot(L, slot, base=None, top=None):
     base = base or L['base']
     top = top or L['top']
 
-    return '{addr}{padding} [ {B}{T}{M}] VALUE: {value}{eol}'.format(
+    return '{addr}{padding} [ {B}{T}{M}] VALUE: {value}'.format(
         addr = strx64(slot),
         padding = PADDING,
         B = 'B' if slot == base else ' ',
         T = 'T' if slot == top else ' ',
         M = 'M' if slot == mref('TValue *', L['maxstack']) else ' ',
         value = dump_tvalue(slot),
-        eol = eol,
     )
 
 def dump_stack(L, base=None, top=None):
@@ -438,7 +460,7 @@ def dump_stack(L, base=None, top=None):
         nredslots = red,
     ))
     dump.extend([
-        dump_stack_slot(L, maxstack + offset, base, top, '')
+        dump_stack_slot(L, maxstack + offset, base, top)
             for offset in range(red, 0, -1)
     ])
     dump.extend([
@@ -446,50 +468,24 @@ def dump_stack(L, base=None, top=None):
             padding = '-' * len(PADDING),
             nstackslots = int((tou64(maxstack) - tou64(stack)) >> 3),
         ),
-        dump_stack_slot(L, maxstack, base, top, ''),
+        dump_stack_slot(L, maxstack, base, top),
         '{start}:{end} [    ] {nfreeslots} slots: Free stack slots'.format(
             start = strx64(top + 1),
             end = strx64(maxstack - 1),
             nfreeslots = int((tou64(maxstack) - tou64(top) - 8) >> 3),
         ),
     ])
-    dump = '\n'.join(dump) + '\n'
-
-    slot = top
-    framelink = base - (1 + LJ_FR2)
-
-    # XXX: Lua stack unwinding algorithm consists of the following steps:
-    # 1. dump all data slots in the (framelink, top) interval
-    # 2. check whether there are remaining frames
-    # 3. if there are no slots further, stop the unwinding loop
-    # 4. otherwise, resolve the next framelink and top and go to (1)
-    #
-    # Postcondition (i.e. do-while) loops is the most fitting idiom for such
-    # case, but Python doesn't provide such lexical construction. Hence step (1)
-    # is unrolled for the topmost stack frame.
-    while slot > framelink + LJ_FR2:
-        dump += dump_stack_slot(L, slot, base, top)
-        slot -= 1
-
-    while framelink > stack:
-        assert slot == framelink + LJ_FR2, "Invalid slot during frame unwind"
-        dump += dump_framelink(L, framelink)
-        framelink = frame_prev(framelink + LJ_FR2) - LJ_FR2
-        slot -= 1 + LJ_FR2
-        while slot > framelink + LJ_FR2:
-            dump += dump_stack_slot(L, slot, base, top)
-            slot -= 1
-
-    assert slot == framelink + LJ_FR2, "Invalid slot after frame unwind"
-    # Skip a nil slot for the last frame for 2-slot frames.
-    slot -= LJ_FR2
-
-    dump += '{fr}{padding} [S   ] FRAME: dummy L'.format(
-        fr = slot,
-        padding = ':{nilslot}'.format(nilslot = slot + 1) if LJ_FR2 else PADDING
-    )
 
-    return dump
+    for framelink, frametop in frames(L):
+        # 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.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:
         ))
 
 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
 
     # 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

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

[Sergey Kaplun via Tarantool-patches]

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

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

[Mikhail Elhimov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 10240 bytes --]

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

>>    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,
>>   }
>>   
>> +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./
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
>> +    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 
<https://peps.python.org/pep-0008/#programming-recommendations>

=========

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
>> +        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)
Please, see my consideration above
>> +
>>   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./
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
Please, see my consideration above
>>       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

[-- Attachment #2: Type: text/html, Size: 15127 bytes --]

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

[Igor Munkin via Tarantool-patches]

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

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

[Mikhail Elhimov via Tarantool-patches]

Hi, Igor!

Thanks for the review!

On 31.08.2022 17:47, Igor Munkin wrote:
> 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 :)
Fixed
>> - 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.
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
>>   LJ_GC64 = None
>>   LJ_FR2 = None
>>   LJ_DUALNUM = None
>> +LJ_FRAMELINK_NUM_SLOTS = None
> Considering the comment right above, this can be dropped.
Fixed
>>   
>>   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.
Fixed
>> +
>> +# 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.
Fixed
>> +
>> +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.
Fixed
>> +
>>   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.
Fixed
>>       )
>>   
> <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.
Fixed (other occurrences of LJ_FRAMELINK_NUM_SLOTS are dropped as well)
>>   
>>       # 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
>>
===================================================
diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
index 9846f3a2..85dffa7b 100644
--- a/src/luajit-gdb.py
+++ b/src/luajit-gdb.py
@@ -150,6 +150,9 @@ def frame_prev(framelink):
      return frame_prevl(framelink) if frame_islua(framelink) \
          else frame_prevd(framelink)
  
+def frame_sentinel(L):
+    return mref('TValue *', L['stack']) + LJ_FR2
+
  # }}}
  
  # Const {{{
@@ -299,6 +302,19 @@ gclen = {
      'mmudata': gcringlen,
  }
  
+# 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 = frame_sentinel(L)
+    while True:
+        yield framelink, frametop
+        frametop = framelink - (1 + LJ_FR2)
+        if framelink <= framelink_sentinel:
+            break
+        framelink = frame_prev(framelink)
+
  # Dumpers {{{
  
  def dump_lj_tnil(tv):
@@ -397,32 +413,36 @@ 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(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 == frame_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_lj_tfunc(fr - LJ_FR2),
      )
  
-def dump_stack_slot(L, slot, base=None, top=None, eol='\n'):
+def dump_stack_slot(L, slot, base=None, top=None):
      base = base or L['base']
      top = top or L['top']
  
-    return '{addr}{padding} [ {B}{T}{M}] VALUE: {value}{eol}'.format(
+    return '{addr}{padding} [ {B}{T}{M}] VALUE: {value}'.format(
          addr = strx64(slot),
          padding = PADDING,
          B = 'B' if slot == base else ' ',
          T = 'T' if slot == top else ' ',
          M = 'M' if slot == mref('TValue *', L['maxstack']) else ' ',
          value = dump_tvalue(slot),
-        eol = eol,
      )
  
  def dump_stack(L, base=None, top=None):
@@ -439,7 +459,7 @@ def dump_stack(L, base=None, top=None):
          ),
      ]
      dump.extend([
-        dump_stack_slot(L, maxstack + offset, base, top, '')
+        dump_stack_slot(L, maxstack + offset, base, top)
              for offset in range(red, 0, -1)
      ])
      dump.extend([
@@ -447,50 +467,24 @@ def dump_stack(L, base=None, top=None):
              padding = '-' * len(PADDING),
              nstackslots = int((tou64(maxstack) - tou64(stack)) >> 3),
          ),
-        dump_stack_slot(L, maxstack, base, top, ''),
+        dump_stack_slot(L, maxstack, base, top),
          '{start}:{end} [    ] {nfreeslots} slots: Free stack slots'.format(
              start = strx64(top + 1),
              end = strx64(maxstack - 1),
              nfreeslots = int((tou64(maxstack) - tou64(top) - 8) >> 3),
          ),
      ])
-    dump = '\n'.join(dump) + '\n'
-
-    slot = top
-    framelink = base - (1 + LJ_FR2)
-
-    # XXX: Lua stack unwinding algorithm consists of the following steps:
-    # 1. dump all data slots in the (framelink, top) interval
-    # 2. check whether there are remaining frames
-    # 3. if there are no slots further, stop the unwinding loop
-    # 4. otherwise, resolve the next framelink and top and go to (1)
-    #
-    # Postcondition (i.e. do-while) loops is the most fitting idiom for such
-    # case, but Python doesn't provide such lexical construction. Hence step (1)
-    # is unrolled for the topmost stack frame.
-    while slot > framelink + LJ_FR2:
-        dump += dump_stack_slot(L, slot, base, top)
-        slot -= 1
-
-    while framelink > stack:
-        assert slot == framelink + LJ_FR2, "Invalid slot during frame unwind"
-        dump += dump_framelink(L, framelink)
-        framelink = frame_prev(framelink + LJ_FR2) - LJ_FR2
-        slot -= 1 + LJ_FR2
-        while slot > framelink + LJ_FR2:
-            dump += dump_stack_slot(L, slot, base, top)
-            slot -= 1
-
-    assert slot == framelink + LJ_FR2, "Invalid slot after frame unwind"
-    # Skip a nil slot for the last frame for 2-slot frames.
-    slot -= LJ_FR2
-
-    dump += '{fr}{padding} [S   ] FRAME: dummy L'.format(
-        fr = slot,
-        padding = ':{nilslot}'.format(nilslot = slot + 1) if LJ_FR2 else PADDING
-    )
  
-    return dump
+    for framelink, frametop in frames(L):
+        # 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.append( dump_framelink(L, framelink) )
+
+    return '\n'.join(dump)
  
  def dump_gc(g):
      gc = g['gc']

-- 
Best regards,
Mikhail Elhimov

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

[Igor Munkin via Tarantool-patches]

Misha,

Thanks for the fixes! LGTM now.

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 0/2] gdb: adjust to support Python 2 + refactoring of stack dumping

[Igor Munkin via Tarantool-patches]

Misha,

I've checked the patch into all long-term branches in tarantool/luajit
and bumped a new version in master, 2.10 and 1.10.

On 23.07.22, Mikhail Elhimov via Tarantool-patches wrote:
> https://github.com/tarantool/luajit/tree/elhimov/gdb_extension_support_python2
> 
> This patch fixes https://github.com/tarantool/tarantool/issues/7458 and also applies refactoring of stack dumping
> 
> Mikhail Elhimov (2):
>   gdb: adjust to support python2 (centos 7)
>   gdb: refactor iteration over frames while dumping stack
> 
>  src/luajit-gdb.py | 119 +++++++++++++++++++++++-----------------------
>  1 file changed, 59 insertions(+), 60 deletions(-)
> 
> -- 
> 2.34.1
> 

-- 
Best regards,
IM