From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6290D445320 for ; Wed, 22 Jul 2020 15:10:46 +0300 (MSK) Date: Wed, 22 Jul 2020 15:00:26 +0300 From: Igor Munkin Message-ID: <20200722120026.GN18920@tarantool.org> References: <20200722104604.GA894@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200722104604.GA894@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] gdb: fix the extension to be loaded with Python 2 List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Ostanevich Cc: tarantool-patches@dev.tarantool.org, Alexander Turenko Sergos, Thanks for your review! On 22.07.20, Sergey Ostanevich wrote: > Hi! > > Thanks for the patch, it LGTM. Added your tag: | Reviewed-by: Sergey Ostanevich > Still I have some comment below. > > Sergos. > > > On 04 Jul 22:58, Igor Munkin wrote: > > There was a mystic error when the extension was loaded against old gdb > > versions build against Python 2: > > | (gdb) source luajit-gdb.py > > | Traceback (most recent call last): > > | File "luajit-gdb.py", line 702, in > > | load(None) > > | File "luajit-gdb.py", line 699, in load > > | 'lj-gc': LJGC, > > | File "luajit-gdb.py", line 687, in init > > | command(name) > > | File "luajit-gdb.py", line 468, in __init__ > > | gdb.write('{} command initialized\n'.format(name)) > > | ValueError: sequence.index(x): x not in sequence > > > > I made a little investigation (for more info see the mentioned issue) > > and found the next fun fact: the exception was raised much earlier to > > , more precisely in . > > However, the handled exception is preserved until call and > > hits the condition underneath leading to the extension load failure. > > > > As a result to avoid the exception raise, the special global variable is > > introduced for legacy (i.e. Python 2) environment. It checks whether any > > callback is associated with new_objfile event prior to disconnecting it. > > This variable usage is encapsulated within two introduced routines: > > and which are wrappers for ones provided by gdb. > > > > Furthermore, after diving to gdb sources related to Python embedding, I > > found that callbacks are grouped into an internal list. Previous > > implementation appended the function to this callback list on > > each its unsuccessful call, but only the successful one is removes it > > from the list. Thereby disconnect action is moved prior to connect one > > so there is no more than one instance kept in callback list. > > > > Fixes tarantool/tarantool#4828 > > > > Reported-by: Oleg Babin > > Signed-off-by: Igor Munkin > > --- > > > > Issue: https://github.com/tarantool/tarantool/issues/4828 > > Branch: imun/gh-4828-luajit-gdb-py2-load > > > > @ChangeLog: > > * Fixed the error occurring on loading luajit-gdb.py with Python 2 (gh-4828). > > > > src/luajit-gdb.py | 40 +++++++++++++++++++++++++++++++++------- > > 1 file changed, 33 insertions(+), 7 deletions(-) > > > > diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py > > index f142fc5..652c560 100644 > > --- a/src/luajit-gdb.py > > +++ b/src/luajit-gdb.py > > @@ -7,7 +7,10 @@ import sys > > > > # make script compatible with the ancient Python {{{ > > > > -if re.match(r'^2\.', sys.version): > > +LEGACY = re.match(r'^2\.', sys.version) > > + > > +if LEGACY: > > + CONNECTED = False > > int = long > > range = xrange > > > > @@ -662,19 +665,42 @@ The command requires no args and dumps current GC stats: > > def init(commands): > > global LJ_64, LJ_GC64, LJ_FR2 > > > > + # XXX Fragile: though connecting the callback looks like a crap but it > > + # respects both Python 2 and Python 3 (see #4828). > > + def connect(callback): > > + if LEGACY: > > + global CONNECTED > > + CONNECTED = True > > + gdb.events.new_objfile.connect(callback) > > + > > + # XXX Fragile: though disconnecting the callback looks like a crap but it > > + # respects both Python 2 and Python 3 (see #4828). > > + def disconnect(callback): > > + if LEGACY: > > + global CONNECTED > > + if not CONNECTED: > > + return > > + CONNECTED = False > > + gdb.events.new_objfile.disconnect(callback) > > + > > + try: > > + # Try to remove the callback at first to not append duplicates to > > + # gdb.events.new_objfile internal list. > > + disconnect(load) > > + except: > > + # Callback is not connected. > > If I got it right, that this it true for 3.x version only? Otherwise, if > you don't have callback connected it will be safely returned from > disconnect() Precisely. > > Why don't you use the same 'crap' machinery for all versions? The > variance of the code will be lower, the reasons to do it this way is > clear. In brief because it's a *crap* (not to be confused with a trap). Now globals usage is enclosed within two routines, and loading scenario itself is not changed regarding the root cause with exception handling. Since you're not insisting on that changes, I would like to leave everything as is. > > > + pass > > + > > try: > > + # Detect whether libluajit objfile is loaded. > > gdb.parse_and_eval('luaJIT_setmode') > > except: > > gdb.write('luajit-gdb.py initialization is postponed ' > > 'until libluajit objfile is loaded\n') > > - gdb.events.new_objfile.connect(load) > > + # Add a callback to be executed when the next objfile is loaded. > > + connect(load) > > return > > > > - try: > > - gdb.events.new_objfile.disconnect(load) > > - except: > > - pass # was not connected > > - > > try: > > LJ_64 = str(gdb.parse_and_eval('IRT_PTR')) == 'IRT_P64' > > LJ_FR2 = LJ_GC64 = str(gdb.parse_and_eval('IRT_PGC')) == 'IRT_P64' > > -- > > 2.25.0 > > -- Best regards, IM