<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi, Sergey!</p>
<p>Thanks for the review!<br>
</p>
<div class="moz-cite-prefix">On 20.08.2022 09:28, Sergey Kaplun
wrote:<br>
</div>
<blockquote type="cite" cite="mid:YwB+8nY5SOKwVES6@root">
<pre class="moz-quote-pre" wrap="">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:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/of common/of the common/</pre>
</blockquote>
Fixed<br>
<blockquote type="cite" cite="mid:YwB+8nY5SOKwVES6@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">- Framelink always refers to the slot right before frame's base
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Don't get this change bullet description. Looks like this is just the
way you implement frame iterator, isn't it?</pre>
</blockquote>
<p>Fixed<br>
</p>
<p>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)"<br>
</p>
<blockquote type="cite" cite="mid:YwB+8nY5SOKwVES6@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">- Introduce constant LJ_FRAMELINK_NUM_SLOTS to make slot arithmetic
more obvious. Previous using of boolean LJ_RT2 in arithmetic looked
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/LJ_RT2/LJ_FR2</pre>
</blockquote>
Fixed
<blockquote type="cite" cite="mid:YwB+8nY5SOKwVES6@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> like a tricky way, both to read and maintain (however places where
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
<p>My point is: LJ_FR2 -- for boolean context,
LJ_FRAMELINK_NUM_SLOTS -- for formulas<br>
</p>
<p>Let me share my considerations.</p>
<p>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<br>
</p>
<p>An example:</p>
<p>red = 5 + 2 * LJ_FR2<br>
</p>
<p>which looks a bit magic, turns into</p>
<p>red = 3 + 2 * LJ_FRAMELINK_NUM_SLOTS</p>
<p>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)</p>
<p>These considerations may be changed when I dive deeper into
luajit, but for now they are )) <br>
</p>
<blockquote type="cite" cite="mid:YwB+8nY5SOKwVES6@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> LJ_RT2 approach had been originally replicated from the source code
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/LJ_RT2/LJ_FR2</pre>
</blockquote>
Fixed
<blockquote type="cite" cite="mid:YwB+8nY5SOKwVES6@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> are left intact, because it is expected that maintenance efforts for
them will be minimal)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
General: Missed dot at the end of the sentences.</pre>
</blockquote>
Fixed
<blockquote type="cite" cite="mid:YwB+8nY5SOKwVES6@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">---
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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">@@ -299,6 +300,21 @@ gclen = {
'mmudata': gcringlen,
}
+def get_framelink_sentinel(L):
+ stack = mref('TValue *', L['stack'])
+ return stack + LJ_FRAMELINK_NUM_SLOTS - 1
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
`LJ_FRAMELINK_NUM_SLOTS - 1 == LJ_FR2` so we can replace it like the
following:
| return stack + LJ_FR2
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+# Generator that implements frame iterator
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/Generator/The generator/
Typo: s/iterator/iterator./</pre>
</blockquote>
Fixed
<blockquote type="cite" cite="mid:YwB+8nY5SOKwVES6@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+# every frame is represented as a tuple of framelink and frametop
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/every/Every/
Typo: s/frametop/frametop./</pre>
</blockquote>
Fixed
<blockquote type="cite" cite="mid:YwB+8nY5SOKwVES6@root">
<pre class="moz-quote-pre" wrap="">Minor: I suggest to specify that this is yielded to the caller tuple.
Feel free to ignore.</pre>
</blockquote>
Sorry, I didn't catch the suggestion. Would you, please, explain?
<blockquote type="cite" cite="mid:YwB+8nY5SOKwVES6@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+def frames(L):
+ frametop = L['top']
+ framelink = L['base'] - 1
+ framelink_sentinel = get_framelink_sentinel(L)
+ while framelink is not None:
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Minor: `is not None` is excess, we can just omit it.</pre>
</blockquote>
<p>I'd prefer to follow the recommendation from <a
moz-do-not-send="true"
href="https://peps.python.org/pep-0008/#programming-recommendations">PEP8</a><br>
</p>
<p>=========</p>
<p>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!<br>
</p>
<p>=========</p>
<p>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:</p>
<p><code>while True:</code><code><br>
</code><code> yield framelink, frametop</code><code><br>
</code><code> frametop = framelink - LJ_FRAMELINK_NUM_SLOTS</code><code><br>
</code><code> if framelink <= framelink_sentinel:</code><code><br>
</code><code> break</code><code><br>
</code><code> framelink = frame_prev(framelink)</code><code><br>
</code><br>
</p>
<p>so finally `is not None` was removed, but for another reason ) <br>
</p>
<blockquote type="cite" cite="mid:YwB+8nY5SOKwVES6@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
We can replace it with the following:
| return dump_lj_tfunc(fr - LJ_FR2)</pre>
</blockquote>
Please, see my consideration above
<blockquote type="cite" cite="mid:YwB+8nY5SOKwVES6@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
def dump_framelink(L, fr):
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> )
-def dump_stack_slot(L, slot, base=None, top=None, eol='\n'):
+def dump_stack_slot(L, slot, base=None, top=None):
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
def dump_stack(L, base=None, top=None):
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ for framelink, frametop in frames(L):
+ # dump all data slots in the (framelink, top) interval
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/dump/Dump/
Typo: s/interval/interval./</pre>
</blockquote>
Fixed
<blockquote type="cite" cite="mid:YwB+8nY5SOKwVES6@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/dump/Dump/
Missed dot at the end of the sentence.</pre>
</blockquote>
Fixed
<blockquote type="cite" cite="mid:YwB+8nY5SOKwVES6@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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:
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">@@ -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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
As far as LJ_FR2 is defined before, we can initialize this variable like
the following:
| LJ_FRAMELINK_NUM_SLOTS = LJ_FR2 + 1</pre>
</blockquote>
Please, see my consideration above
<blockquote type="cite" cite="mid:YwB+8nY5SOKwVES6@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> except:
gdb.write('luajit-gdb.py failed to load: '
'no debugging symbols found for libluajit\n')
--
2.34.1
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">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)
===================================================
</pre>
<pre class="moz-signature" cols="72">--
Best regards,
Mikhail Elhimov</pre>
</body>
</html>