[Tarantool-patches] [PATCH] gdb: fix the extension to be loaded with Python 2
Igor Munkin
imun at tarantool.org
Wed Jul 22 15:00:26 MSK 2020
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 <sergos at tarantool.org>
> 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 <module>
> > | 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
> > <str.format>, more precisely in <gdb.events.new_objfile.disconnect>.
> > However, the handled exception is preserved until <str.format> 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:
> > <connect> and <disconnect> 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 <load> 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 <load> instance kept in callback list.
> >
> > Fixes tarantool/tarantool#4828
> >
> > Reported-by: Oleg Babin <olegrok at tarantool.org>
> > Signed-off-by: Igor Munkin <imun at tarantool.org>
> > ---
> >
> > 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
More information about the Tarantool-patches
mailing list