Re: [Tarantool-patches] [PATCH] gdb: fix the extension to be loaded with Python 2

[Igor Munkin <imun@tarantool.org>]

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@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@tarantool.org>
> > Signed-off-by: Igor Munkin <imun@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