Re: [Tarantool-patches] [PATCH luajit 2/2] gdb: introduce lj-bc, lj-func and lj-proto dumpers

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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


Hi, Sergey!
Thanks for the patch!
 
Please consider my comments below.
 
>
>diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
>index 779a25f8..339b57ed 100644
>--- a/src/luajit-gdb.py
>+++ b/src/luajit-gdb.py
<snipped> 
>+___ = 0
>+BC_NAME = 0
>+BC_A = 1
>+BC_B = 2
>+BC_CD = 3
>+BC_MM = 4
>+ 
>+BYTECODES = [
>+ # Comparison ops. ORDER OPR.
>+ ['ISLT', 'var', ___, 'var', 'lt'],
>+ ['ISGE', 'var', ___, 'var', 'lt'],
>+ ['ISLE', 'var', ___, 'var', 'le'],
>+ ['ISGT', 'var', ___, 'var', 'le'],
>+
>+ ['ISEQV', 'var', ___, 'var', 'eq'],
>+ ['ISNEV', 'var', ___, 'var', 'eq'],
>+ ['ISEQS', 'var', ___, 'str', 'eq'],
>+ ['ISNES', 'var', ___, 'str', 'eq'],
>+ ['ISEQN', 'var', ___, 'num', 'eq'],
>+ ['ISNEN', 'var', ___, 'num', 'eq'],
>+ ['ISEQP', 'var', ___, 'pri', 'eq'],
>+ ['ISNEP', 'var', ___, 'pri', 'eq'],
>+
>+ # Unary test and copy ops.
>+ ['ISTC', 'dst', ___, 'var', ___],
>+ ['ISFC', 'dst', ___, 'var', ___],
>+ ['IST', ___, ___, 'var', ___],
>+ ['ISF', ___, ___, 'var', ___],
>+ ['ISTYPE', 'var', ___, 'lit', ___],
>+ ['ISNUM', 'var', ___, 'lit', ___],
>+ ['MOV', 'dst', ___, 'var', ___],
>+ ['NOT', 'dst', ___, 'var', ___],
>+ ['UNM', 'dst', ___, 'var', 'unm'],
>+ ['LEN', 'dst', ___, 'var', 'len'],
>+ ['ADDVN', 'dst', 'var', 'num', 'add'],
>+ ['SUBVN', 'dst', 'var', 'num', 'sub'],
>+ ['MULVN', 'dst', 'var', 'num', 'mul'],
>+ ['DIVVN', 'dst', 'var', 'num', 'div'],
>+ ['MODVN', 'dst', 'var', 'num', 'mod'],
>+
>+ # Binary ops. ORDER OPR.
>+ ['ADDNV', 'dst', 'var', 'num', 'add'],
>+ ['SUBNV', 'dst', 'var', 'num', 'sub'],
>+ ['MULNV', 'dst', 'var', 'num', 'mul'],
>+ ['DIVNV', 'dst', 'var', 'num', 'div'],
>+ ['MODNV', 'dst', 'var', 'num', 'mod'],
>+
>+ ['ADDVV', 'dst', 'var', 'var', 'add'],
>+ ['SUBVV', 'dst', 'var', 'var', 'sub'],
>+ ['MULVV', 'dst', 'var', 'var', 'mul'],
>+ ['DIVVV', 'dst', 'var', 'var', 'div'],
>+ ['MODVV', 'dst', 'var', 'var', 'mod'],
>+
>+ ['POW', 'dst', 'var', 'var', 'pow'],
>+ ['CAT', 'dst', 'rbase', 'rbase', 'concat'],
>+
>+ # Constant ops.
>+ ['KSTR', 'dst', ___, 'str', ___],
>+ ['KCDATA', 'dst', ___, 'cdata', ___],
>+ ['KSHORT', 'dst', ___, 'lits', ___],
>+ ['KNUM', 'dst', ___, 'num', ___],
>+ ['KPRI', 'dst', ___, 'pri', ___],
>+ ['KNIL', 'base', ___, 'base', ___],
>+
>+ # Upvalue and function ops.
>+ ['UGET', 'dst', ___, 'uv', ___],
>+ ['USETV', 'uv', ___, 'var', ___],
>+ ['USETS', 'uv', ___, 'str', ___],
>+ ['USETN', 'uv', ___, 'num', ___],
>+ ['USETP', 'uv', ___, 'pri', ___],
>+ ['UCLO', 'rbase', ___, 'jump', ___],
>+ ['FNEW', 'dst', ___, 'func', ___],
>+
>+ # Table ops.
>+ ['TNEW', 'dst', ___, 'lit', ___],
>+ ['TDUP', 'dst', ___, 'tab', ___],
>+ ['GGET', 'dst', ___, 'str', 'index'],
>+ ['GSET', 'var', ___, 'str', 'newindex'],
>+ ['TGETV', 'dst', 'var', 'var', 'index'],
>+ ['TGETS', 'dst', 'var', 'str', 'index'],
>+ ['TGETB', 'dst', 'var', 'lit', 'index'],
>+ ['TGETR', 'dst', 'var', 'var', 'index'],
>+ ['TSETV', 'var', 'var', 'var', 'newindex'],
>+ ['TSETS', 'var', 'var', 'str', 'newindex'],
>+ ['TSETB', 'var', 'var', 'lit', 'newindex'],
>+ ['TSETM', 'base', ___, 'num', 'newindex'],
>+ ['TSETR', 'var', 'var', 'var', 'newindex'],
>+
>+ # Calls and vararg handling. T = tail call.
>+ ['CALLM', 'base', 'lit', 'lit', 'call'],
>+ ['CALL', 'base', 'lit', 'lit', 'call'],
>+ ['CALLMT', 'base', ___, 'lit', 'call'],
>+ ['CALLT', 'base', ___, 'lit', 'call'],
>+ ['ITERC', 'base', 'lit', 'lit', 'call'],
>+ ['ITERN', 'base', 'lit', 'lit', 'call'],
>+ ['VARG', 'base', 'lit', 'lit', ___],
>+ ['ISNEXT', 'base', ___, 'jump', ___],
>+
>+ # Returns.
>+ ['RETM', 'base', ___, 'lit', ___],
>+ ['RET', 'rbase', ___, 'lit', ___],
>+ ['RET0', 'rbase', ___, 'lit', ___],
>+ ['RET1', 'rbase', ___, 'lit', ___],
>+
>+ # Loops and branches. I/J = interp/JIT, I/C/L = init/call/loop.
>+ ['FORI', 'base', ___, 'jump', ___],
>+ ['JFORI', 'base', ___, 'jump', ___],
>+
>+ ['FORL', 'base', ___, 'jump', ___],
>+ ['IFORL', 'base', ___, 'jump', ___],
>+ ['JFORL', 'base', ___, 'lit', ___],
>+
>+ ['ITERL', 'base', ___, 'jump', ___],
>+ ['IITERL', 'base', ___, 'jump', ___],
>+ ['JITERL', 'base', ___, 'lit', ___],
>+
>+ ['LOOP', 'rbase', ___, 'jump', ___],
>+ ['ILOOP', 'rbase', ___, 'jump', ___],
>+ ['JLOOP', 'rbase', ___, 'lit', ___],
>+
>+ ['JMP', 'rbase', ___, 'jump', ___],
>+
>+ # Function headers. I/J = interp/JIT, F/V/C = fixarg/vararg/C func.
>+ ['FUNCF', 'rbase', ___, ___, ___],
>+ ['IFUNCF', 'rbase', ___, ___, ___],
>+ ['JFUNCF', 'rbase', ___, 'lit', ___],
>+ ['FUNCV', 'rbase', ___, ___, ___],
>+ ['IFUNCV', 'rbase', ___, ___, ___],
>+ ['JFUNCV', 'rbase', ___, 'lit', ___],
>+ ['FUNCC', 'rbase', ___, ___, ___],
>+ ['FUNCCW', 'rbase', ___, ___, ___],
>+]
I suggest rewriting that chunk with named tuple[1]. That is more readable
and more pythonic way of doing stuff like this.
 
<snipped> 
>+def dump_reg(bc, reg, value, jmp_format=None, jmp_ctx=None):
>+ rtype = bc[reg]
>+ is_jmp = rtype == 'jump'
>+ padding = ':' + ' ' * (5 - len(rtype))
>+
>+ if rtype == 'jump':
>+ # Destination of jump instruction encoded as offset from BCBIAS_J.
>+ delta = value - 0x7fff
>+ if jmp_format:
>+ value = jmp_format(jmp_ctx, delta)
>+ else:
>+ prefix = '+' if delta >= 0 else ''
>+ value = prefix + str(delta)
>+ else:
>+ value = '{:3d}'.format(value)
>+
>+ return '{rtype}{padding} {value}'.format(
>+ rtype = rtype,
>+ padding = padding,
>+ value = value,
>+ )
The padding variable is unnecessary. You can use format-string
width specifier[2] instead.
 
<snipped>
>
>+class LJDumpFunc(LJBase):
>+ '''
>+lj-funcl <GCfunc *>
Typo: s/lj-funcl/lj-func
 
<snipped>
 
Side note:
In the present moment, passing an invalid pointer to `lj-func`/`lj-proto` results in empty
output, which is not a gdb-way (and not a UNIX-way either) of reporting errors.
Maybe it is a good idea to check the type of the  value under the pointer and
report corresponding errors. 
 
[1]:  https://docs.python.org/3/library/collections.html#collections.namedtuple
[2]:  https://docs.python.org/3/library/string.html#format-examples
 
--
Best regards,
Maxim Kokryashkin

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