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

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

[Igor Munkin]

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

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

[Oleg Babin]

Hi! Thanks for your patch!

I just add that problem appeared on CentOS 7 that is still popular.

But seems your patch solves a problem. LGTM.


On 04/07/2020 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

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

[Igor Munkin]

Oleg,

Thanks for your review!

On 06.07.20, Oleg Babin wrote:
> Hi! Thanks for your patch!
> 
> I just add that problem appeared on CentOS 7 that is still popular.

As I mentioned in the commit message the problem relates to gdb built
against Python 2 (viz. 2.7.5, since I can't claim anything about later
Python 2 versions). CentOS 7 is just particular distro providing this
gdb and I guess one can install a new one from SCL. However, as we
discussed with Sasha some time before it would be nice to support the
toolchain provided by default. Thereby we should support Python 2 even
after its EOL until CentOS 7 is gone.

> 
> But seems your patch solves a problem. LGTM.

Added your tag:
| Reviewed-by: Oleg Babin <olegrok@tarantool.org>

> 
> 
> On 04/07/2020 22:58, Igor Munkin wrote:
> > There was a mystic error when the extension was loaded against old gdb
> > versions build against Python 2:

Meh, this line looks bad, I've reworded it a bit:
| There was a mystic error when the extension was loaded by the old gdb
| versions built against Python 2:

Updated the upstream.

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

-- 
Best regards,
IM

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

[Sergey Ostanevich]

Hi! 

Thanks for the patch, it LGTM.
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()

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.

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

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

[Igor Munkin]

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

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

[Kirill Yukhin]

Hello,

On 04 июл 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>

I've checked your patch into Tarantool's luajit repo and bumped a
new version in 1.10, 2.4, 2.5 and master branches.

--
Regards, Kirill Yukhin