Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maxim Kokryashkin <m.kokryashkin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 2/2] gdb: introduce lj-bc, lj-func and lj-proto dumpers
Date: Thu, 14 Jul 2022 18:41:08 +0300	[thread overview]
Message-ID: <YtA5FAtNQxgy0xBi@root> (raw)
In-Reply-To: <1656951016.765494116@f748.i.mail.ru>

Hi, Maxim!

Thanks for the review!

On 04.07.22, Maxim Kokryashkin wrote:
> 
> Hi, Sergey!
> Thanks for the patch!
>  
> Please consider my comments below.

Fixed, see iterative patches below.
Branch is force-pushed.

Side note: I've fixed you suggestions from the last one to the top one,
so don't be confused by diff in them.

>  
> >
> >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'],

<snipped>

> >+ ['FUNCCW', 'rbase', ___, ___, ___],
> >+]
> I suggest rewriting that chunk with named tuple[1]. That is more readable
> and more pythonic way of doing stuff like this.

I've rewrited it with tuple (instead namedtuple) to avoid usage of
"collections", also it helps to read each bytecode declaration.

See the iterative patch below.

===================================================================
diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
index be86f599..98feebec 100644
--- a/src/luajit-gdb.py
+++ b/src/luajit-gdb.py
@@ -129,136 +129,131 @@ def bc_d(ins):
     return int(ins) >> 16
 
 ___ = 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'],
+    {'name': 'ISLT',   'ra': 'var',   'rb': ___,     'rcd': 'var',   'mm': 'lt'},
+    {'name': 'ISGE',   'ra': 'var',   'rb': ___,     'rcd': 'var',   'mm': 'lt'},
+    {'name': 'ISLE',   'ra': 'var',   'rb': ___,     'rcd': 'var',   'mm': 'le'},
+    {'name': 'ISGT',   'ra': 'var',   'rb': ___,     'rcd': 'var',   'mm': 'le'},
+
+    {'name': 'ISEQV',  'ra': 'var',   'rb': ___,     'rcd': 'var',   'mm': 'eq'},
+    {'name': 'ISNEV',  'ra': 'var',   'rb': ___,     'rcd': 'var',   'mm': 'eq'},
+    {'name': 'ISEQS',  'ra': 'var',   'rb': ___,     'rcd': 'str',   'mm': 'eq'},
+    {'name': 'ISNES',  'ra': 'var',   'rb': ___,     'rcd': 'str',   'mm': 'eq'},
+    {'name': 'ISEQN',  'ra': 'var',   'rb': ___,     'rcd': 'num',   'mm': 'eq'},
+    {'name': 'ISNEN',  'ra': 'var',   'rb': ___,     'rcd': 'num',   'mm': 'eq'},
+    {'name': 'ISEQP',  'ra': 'var',   'rb': ___,     'rcd': 'pri',   'mm': 'eq'},
+    {'name': 'ISNEP',  'ra': 'var',   'rb': ___,     'rcd': 'pri',   'mm': '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'],
+    {'name': 'ISTC',   'ra': 'dst',   'rb': ___,     'rcd': 'var',   'mm': ___},
+    {'name': 'ISFC',   'ra': 'dst',   'rb': ___,     'rcd': 'var',   'mm': ___},
+    {'name': 'IST',    'ra': ___,     'rb': ___,     'rcd': 'var',   'mm': ___},
+    {'name': 'ISF',    'ra': ___,     'rb': ___,     'rcd': 'var',   'mm': ___},
+    {'name': 'ISTYPE', 'ra': 'var',   'rb': ___,     'rcd': 'lit',   'mm': ___},
+    {'name': 'ISNUM',  'ra': 'var',   'rb': ___,     'rcd': 'lit',   'mm': ___},
+    {'name': 'MOV',    'ra': 'dst',   'rb': ___,     'rcd': 'var',   'mm': ___},
+    {'name': 'NOT',    'ra': 'dst',   'rb': ___,     'rcd': 'var',   'mm': ___},
+    {'name': 'UNM',    'ra': 'dst',   'rb': ___,     'rcd': 'var',   'mm': 'unm'},
+    {'name': 'LEN',    'ra': 'dst',   'rb': ___,     'rcd': 'var',   'mm': 'len'},
+    {'name': 'ADDVN',  'ra': 'dst',   'rb': 'var',   'rcd': 'num',   'mm': 'add'},
+    {'name': 'SUBVN',  'ra': 'dst',   'rb': 'var',   'rcd': 'num',   'mm': 'sub'},
+    {'name': 'MULVN',  'ra': 'dst',   'rb': 'var',   'rcd': 'num',   'mm': 'mul'},
+    {'name': 'DIVVN',  'ra': 'dst',   'rb': 'var',   'rcd': 'num',   'mm': 'div'},
+    {'name': 'MODVN',  'ra': 'dst',   'rb': 'var',   'rcd': 'num',   'mm': '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'],
+    {'name': 'ADDNV',  'ra': 'dst',   'rb': 'var',   'rcd': 'num',   'mm': 'add'},
+    {'name': 'SUBNV',  'ra': 'dst',   'rb': 'var',   'rcd': 'num',   'mm': 'sub'},
+    {'name': 'MULNV',  'ra': 'dst',   'rb': 'var',   'rcd': 'num',   'mm': 'mul'},
+    {'name': 'DIVNV',  'ra': 'dst',   'rb': 'var',   'rcd': 'num',   'mm': 'div'},
+    {'name': 'MODNV',  'ra': 'dst',   'rb': 'var',   'rcd': 'num',   'mm': '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'],
+    {'name': 'ADDVV',  'ra': 'dst',   'rb': 'var',   'rcd': 'var',   'mm': 'add'},
+    {'name': 'SUBVV',  'ra': 'dst',   'rb': 'var',   'rcd': 'var',   'mm': 'sub'},
+    {'name': 'MULVV',  'ra': 'dst',   'rb': 'var',   'rcd': 'var',   'mm': 'mul'},
+    {'name': 'DIVVV',  'ra': 'dst',   'rb': 'var',   'rcd': 'var',   'mm': 'div'},
+    {'name': 'MODVV',  'ra': 'dst',   'rb': 'var',   'rcd': 'var',   'mm': 'mod'},
 
-    ['POW',    'dst',  'var',   'var',    'pow'],
-    ['CAT',    'dst',  'rbase', 'rbase',  'concat'],
+    {'name': 'POW',    'ra': 'dst',   'rb': 'var',   'rcd': 'var',   'mm': 'pow'},
+    {'name': 'CAT',    'ra': 'dst',   'rb': 'rbase', 'rcd': 'rbase', 'mm': 'concat'},
 
     # Constant ops.
-    ['KSTR',   'dst',   ___,    'str',    ___],
-    ['KCDATA', 'dst',   ___,    'cdata',  ___],
-    ['KSHORT', 'dst',   ___,    'lits',   ___],
-    ['KNUM',   'dst',   ___,    'num',    ___],
-    ['KPRI',   'dst',   ___,    'pri',    ___],
-    ['KNIL',   'base',  ___,    'base',   ___],
+    {'name': 'KSTR',   'ra': 'dst',   'rb':  ___,    'rcd': 'str',   'mm': ___},
+    {'name': 'KCDATA', 'ra': 'dst',   'rb':  ___,    'rcd': 'cdata', 'mm': ___},
+    {'name': 'KSHORT', 'ra': 'dst',   'rb':  ___,    'rcd': 'lits',  'mm': ___},
+    {'name': 'KNUM',   'ra': 'dst',   'rb':  ___,    'rcd': 'num',   'mm': ___},
+    {'name': 'KPRI',   'ra': 'dst',   'rb':  ___,    'rcd': 'pri',   'mm': ___},
+    {'name': 'KNIL',   'ra': 'base',  'rb':  ___,    'rcd': 'base',  'mm': ___},
 
     # 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',   ___],
+    {'name': 'UGET',   'ra': 'dst',   'rb':  ___,    'rcd':  'uv',   'mm':  ___},
+    {'name': 'USETV',  'ra': 'uv',    'rb':  ___,    'rcd':  'var',  'mm':  ___},
+    {'name': 'USETS',  'ra': 'uv',    'rb':  ___,    'rcd':  'str',  'mm':  ___},
+    {'name': 'USETN',  'ra': 'uv',    'rb':  ___,    'rcd':  'num',  'mm':  ___},
+    {'name': 'USETP',  'ra': 'uv',    'rb':  ___,    'rcd':  'pri',  'mm':  ___},
+    {'name': 'UCLO',   'ra': 'rbase', 'rb':  ___,    'rcd':  'jump', 'mm':  ___},
+    {'name': 'FNEW',   'ra': 'dst',   'rb':  ___,    'rcd':  'func', 'mm':  ___},
 
     # 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'],
+    {'name': 'TNEW',   'ra': 'dst',   'rb':  ___,    'rcd': 'lit',   'mm': ___},
+    {'name': 'TDUP',   'ra': 'dst',   'rb':  ___,    'rcd': 'tab',   'mm': ___},
+    {'name': 'GGET',   'ra': 'dst',   'rb':  ___,    'rcd': 'str',   'mm': 'index'},
+    {'name': 'GSET',   'ra': 'var',   'rb':  ___,    'rcd': 'str',   'mm': 'newindex'},
+    {'name': 'TGETV',  'ra': 'dst',   'rb':  'var',  'rcd': 'var',   'mm': 'index'},
+    {'name': 'TGETS',  'ra': 'dst',   'rb':  'var',  'rcd': 'str',   'mm': 'index'},
+    {'name': 'TGETB',  'ra': 'dst',   'rb':  'var',  'rcd': 'lit',   'mm': 'index'},
+    {'name': 'TGETR',  'ra': 'dst',   'rb':  'var',  'rcd': 'var',   'mm': 'index'},
+    {'name': 'TSETV',  'ra': 'var',   'rb':  'var',  'rcd': 'var',   'mm': 'newindex'},
+    {'name': 'TSETS',  'ra': 'var',   'rb':  'var',  'rcd': 'str',   'mm': 'newindex'},
+    {'name': 'TSETB',  'ra': 'var',   'rb':  'var',  'rcd': 'lit',   'mm': 'newindex'},
+    {'name': 'TSETM',  'ra': 'base',  'rb':  ___,    'rcd': 'num',   'mm': 'newindex'},
+    {'name': 'TSETR',  'ra': 'var',   'rb':  'var',  'rcd': 'var',   'mm': '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',   ___],
+    {'name': 'CALLM',  'ra': 'base',  'rb':  'lit',  'rcd': 'lit',   'mm': 'call'},
+    {'name': 'CALL',   'ra': 'base',  'rb':  'lit',  'rcd': 'lit',   'mm': 'call'},
+    {'name': 'CALLMT', 'ra': 'base',  'rb':  ___,    'rcd': 'lit',   'mm': 'call'},
+    {'name': 'CALLT',  'ra': 'base',  'rb':  ___,    'rcd': 'lit',   'mm': 'call'},
+    {'name': 'ITERC',  'ra': 'base',  'rb':  'lit',  'rcd': 'lit',   'mm': 'call'},
+    {'name': 'ITERN',  'ra': 'base',  'rb':  'lit',  'rcd': 'lit',   'mm': 'call'},
+    {'name': 'VARG',   'ra': 'base',  'rb':  'lit',  'rcd': 'lit',   'mm': ___},
+    {'name': 'ISNEXT', 'ra': 'base',  'rb':  ___,    'rcd': 'jump',  'mm': ___},
 
     # Returns.
-    ['RETM',   'base',  ___,    'lit',    ___],
-    ['RET',    'rbase', ___,    'lit',    ___],
-    ['RET0',   'rbase', ___,    'lit',    ___],
-    ['RET1',   'rbase', ___,    'lit',    ___],
+    {'name': 'RETM',   'ra': 'base',  'rb':  ___,    'rcd': 'lit',   'mm': ___},
+    {'name': 'RET',    'ra': 'rbase', 'rb':  ___,    'rcd': 'lit',   'mm': ___},
+    {'name': 'RET0',   'ra': 'rbase', 'rb':  ___,    'rcd': 'lit',   'mm': ___},
+    {'name': 'RET1',   'ra': 'rbase', 'rb':  ___,    'rcd': 'lit',   'mm': ___},
 
     # Loops and branches. I/J = interp/JIT, I/C/L = init/call/loop.
-    ['FORI',   'base',  ___,    'jump',   ___],
-    ['JFORI',  'base',  ___,    'jump',   ___],
+    {'name': 'FORI',   'ra': 'base',  'rb':  ___,    'rcd': 'jump',  'mm': ___},
+    {'name': 'JFORI',  'ra': 'base',  'rb':  ___,    'rcd': 'jump',  'mm': ___},
 
-    ['FORL',   'base',  ___,    'jump',   ___],
-    ['IFORL',  'base',  ___,    'jump',   ___],
-    ['JFORL',  'base',  ___,    'lit',    ___],
+    {'name': 'FORL',   'ra': 'base',  'rb':  ___,    'rcd': 'jump',  'mm': ___},
+    {'name': 'IFORL',  'ra': 'base',  'rb':  ___,    'rcd': 'jump',  'mm': ___},
+    {'name': 'JFORL',  'ra': 'base',  'rb':  ___,    'rcd': 'lit',   'mm': ___},
 
-    ['ITERL',  'base',  ___,    'jump',   ___],
-    ['IITERL', 'base',  ___,    'jump',   ___],
-    ['JITERL', 'base',  ___,    'lit',    ___],
+    {'name': 'ITERL',  'ra': 'base',  'rb':  ___,    'rcd': 'jump',  'mm': ___},
+    {'name': 'IITERL', 'ra': 'base',  'rb':  ___,    'rcd': 'jump',  'mm': ___},
+    {'name': 'JITERL', 'ra': 'base',  'rb':  ___,    'rcd': 'lit',   'mm': ___},
 
-    ['LOOP',   'rbase', ___,    'jump',   ___],
-    ['ILOOP',  'rbase', ___,    'jump',   ___],
-    ['JLOOP',  'rbase', ___,    'lit',    ___],
+    {'name': 'LOOP',   'ra': 'rbase', 'rb':  ___,    'rcd': 'jump',  'mm': ___},
+    {'name': 'ILOOP',  'ra': 'rbase', 'rb':  ___,    'rcd': 'jump',  'mm': ___},
+    {'name': 'JLOOP',  'ra': 'rbase', 'rb':  ___,    'rcd': 'lit',   'mm': ___},
 
-    ['JMP',    'rbase', ___,    'jump',   ___],
+    {'name': 'JMP',    'ra': 'rbase', 'rb':  ___,    'rcd': 'jump',  'mm': ___},
 
     # 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', ___,    ___,      ___],
+    {'name': 'FUNCF',  'ra': 'rbase', 'rb':  ___,    'rcd': ___,     'mm': ___},
+    {'name': 'IFUNCF', 'ra': 'rbase', 'rb':  ___,    'rcd': ___,     'mm': ___},
+    {'name': 'JFUNCF', 'ra': 'rbase', 'rb':  ___,    'rcd': 'lit',   'mm': ___},
+    {'name': 'FUNCV',  'ra': 'rbase', 'rb':  ___,    'rcd': ___,     'mm': ___},
+    {'name': 'IFUNCV', 'ra': 'rbase', 'rb':  ___,    'rcd': ___,     'mm': ___},
+    {'name': 'JFUNCV', 'ra': 'rbase', 'rb':  ___,    'rcd': 'lit',   'mm': ___},
+    {'name': 'FUNCC',  'ra': 'rbase', 'rb':  ___,    'rcd': ___,     'mm': ___},
+    {'name': 'FUNCCW', 'ra': 'rbase', 'rb':  ___,    'rcd': ___,     'mm': ___},
 ]
 
 def proto_bc(proto):
@@ -725,8 +720,7 @@ def funcuvname(pt, idx):
 
     return str(cast('char *', uvinfo))
 
-def dump_reg(bc, reg, value, jmp_format=None, jmp_ctx=None):
-    rtype = bc[reg]
+def dump_reg(rtype, value, jmp_format=None, jmp_ctx=None):
     is_jmp = rtype == 'jump'
 
     if rtype == 'jump':
@@ -745,8 +739,7 @@ def dump_reg(bc, reg, value, jmp_format=None, jmp_ctx=None):
         value = value,
     )
 
-def dump_kc(bc, reg, value, proto):
-    rtype = bc[reg]
+def dump_kc(rtype, value, proto):
     kc = ''
     if proto:
         if rtype == 'str' or rtype == 'func':
@@ -766,23 +759,21 @@ def dump_bc(ins, jmp_format=None, jmp_ctx=None, proto=None):
         return 'INVALID'
 
     bc = BYTECODES[op]
-    bc_name = bc[BC_NAME]
 
-    bc_hasa = bc[BC_A]
-    bc_hasb = bc[BC_B]
-
-    kca = dump_kc(bc, BC_A, bc_a(ins), proto) if bc_hasa else ''
-    kcc = dump_kc(bc, BC_CD, bc_c(ins) if bc_hasb else bc_d(ins), proto) if bc[BC_CD] else ''
+    kca = dump_kc(bc['ra'], bc_a(ins), proto) if bc['ra'] else ''
+    kcc = dump_kc(
+        bc['rcd'], bc_c(ins) if bc['rb'] else bc_d(ins), proto
+    ) if bc['rcd'] else ''
 
     return '{name:6} {ra}{rb}{rcd}{kc}'.format(
-        name = bc_name,
-        ra = dump_reg(bc, BC_A, bc_a(ins)) + ' ' if bc_hasa else '',
-        rb = dump_reg(bc, BC_B, bc_b(ins)) + ' ' if bc_hasb else '',
+        name = bc['name'],
+        ra = dump_reg(bc['ra'], bc_a(ins)) + ' ' if bc['ra'] else '',
+        rb = dump_reg(bc['rb'], bc_b(ins)) + ' ' if bc['rb'] else '',
         rcd = dump_reg(
-            bc, BC_CD, bc_c(ins) if bc_hasb else bc_d(ins),
+            bc['rcd'], bc_c(ins) if bc['rb'] else bc_d(ins),
             jmp_format=jmp_format, jmp_ctx=jmp_ctx
-        ) if bc[BC_CD] else '',
-        kc=kca+kcc
+        ) if bc['rcd'] else '',
+        kc = kca + kcc
     )
 
 def dump_proto(proto):
===================================================================

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

Fixed! Thanks! See the iterative patch below.

===================================================================
diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
index 0827ad45..be86f599 100644
--- a/src/luajit-gdb.py
+++ b/src/luajit-gdb.py
@@ -728,7 +728,6 @@ def funcuvname(pt, idx):
 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.
@@ -741,9 +740,8 @@ def dump_reg(bc, reg, value, jmp_format=None, jmp_ctx=None):
     else:
         value = '{:3d}'.format(value)
 
-    return '{rtype}{padding} {value}'.format(
-        rtype = rtype,
-        padding = padding,
+    return '{rtype:6} {value}'.format(
+        rtype = rtype + ':',
         value = value,
     )
 
@@ -769,7 +767,6 @@ def dump_bc(ins, jmp_format=None, jmp_ctx=None, proto=None):
 
     bc = BYTECODES[op]
     bc_name = bc[BC_NAME]
-    name_padding = ' ' * (6 - len(bc_name))
 
     bc_hasa = bc[BC_A]
     bc_hasb = bc[BC_B]
@@ -777,9 +774,8 @@ def dump_bc(ins, jmp_format=None, jmp_ctx=None, proto=None):
     kca = dump_kc(bc, BC_A, bc_a(ins), proto) if bc_hasa else ''
     kcc = dump_kc(bc, BC_CD, bc_c(ins) if bc_hasb else bc_d(ins), proto) if bc[BC_CD] else ''
 
-    return '{name}{npad} {ra}{rb}{rcd}{kc}'.format(
+    return '{name:6} {ra}{rb}{rcd}{kc}'.format(
         name = bc_name,
-        npad = name_padding,
         ra = dump_reg(bc, BC_A, bc_a(ins)) + ' ' if bc_hasa else '',
         rb = dump_reg(bc, BC_B, bc_b(ins)) + ' ' if bc_hasb else '',
         rcd = dump_reg(
~
===================================================================

>  
> <snipped>
> >
> >+class LJDumpFunc(LJBase):
> >+ '''
> >+lj-funcl <GCfunc *>
> Typo: s/lj-funcl/lj-func

Fixed! Thanks! See the iterative patch below.

===================================================================
diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
index f976b758..0827ad45 100644
--- a/src/luajit-gdb.py
+++ b/src/luajit-gdb.py
@@ -1051,7 +1051,7 @@ The constants or upvalues of the prototype are decoded after ';'.
 
 class LJDumpFunc(LJBase):
     '''
-lj-funcl <GCfunc *>
+lj-func <GCfunc *>
 
 The command receives a <gcr> of the corresponding GCfunc object and dumps
 the chunk name, where the corresponding function is defined, corresponding
===================================================================

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

Yes, maybe. But I suggest now just use the same interface as for other
lj-* dumpers. We can refactor them later.

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

-- 
Best regards,
Sergey Kaplun

  reply	other threads:[~2022-07-14 15:43 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-09 10:11 [Tarantool-patches] [PATCH luajit 0/2] Introduce dumpers for bytecodes in gdb Sergey Kaplun via Tarantool-patches
2022-06-09 10:11 ` [Tarantool-patches] [PATCH luajit 1/2] gdb: introduce dumpers for GCobj Sergey Kaplun via Tarantool-patches
2022-07-04 15:24   ` Maxim Kokryashkin via Tarantool-patches
2022-07-14 12:08     ` Sergey Kaplun via Tarantool-patches
2022-07-19  8:39       ` Maxim Kokryashkin via Tarantool-patches
2022-06-09 10:11 ` [Tarantool-patches] [PATCH luajit 2/2] gdb: introduce lj-bc, lj-func and lj-proto dumpers Sergey Kaplun via Tarantool-patches
2022-07-04 16:10   ` Maxim Kokryashkin via Tarantool-patches
2022-07-14 15:41     ` Sergey Kaplun via Tarantool-patches [this message]
2022-07-19  8:40       ` Maxim Kokryashkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YtA5FAtNQxgy0xBi@root \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=m.kokryashkin@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit 2/2] gdb: introduce lj-bc, lj-func and lj-proto dumpers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox