[PATCH] bug fix for gdb 16039

[PATCH] bug fix for gdb 16039

[Dave.Tian]

1 Description: Gdb bug 16039 created by me

  Title: Gdb next" command stop working when shared library unloaded.
  Root Cause: There is "libc++" static linked into the shared library,
and since gdb insert internal breakpoints on std::terminate/longjump/...
so after dlclose, the memory address is
invalid,remove_breakpoints/insert_breakpoints
failed with EIO error.

  Fix: Disable the internal breakpoints when dlclose hit.

2 ChangeLog:

2013-10-16  Tian Ye  <xhengdf@gmail.com>

	PR gdb/16039
	* breakpoint.c (is_removable_in_unloaded_shlib): New.
	(set_longjmp_breakpoint): Check if breakpoint's location
	address is not in an unloaded shared library
	(disable_breakpoints_in_unloaded_shlib): Disable the
	internal breakpoints in the hook function of shared
	library unload.

3 Patch Diffs:

--- breakpoint.c.bak	2013-10-12 01:15:09.044081000 -0700
+++ breakpoint.c	2013-10-15 18:50:59.842116000 -0700
@@ -1118,6 +1118,17 @@ is_tracepoint (const struct breakpoint *
   return is_tracepoint_type (b->type);
 }

+/* Breakpoints should be disable when shlib unload.  */
+
+static int
+is_removable_in_unloaded_shlib(const struct breakpoint *b)
+{
+  return (b->type == bp_longjmp_master
+	  || b->type == bp_std_terminate_master
+	  || b->type == bp_exception_master
+	  || b->type == bp_exception);
+}
+
 /* A helper function that validates that COMMANDS are valid for a
    breakpoint.  This function will throw an exception if a problem is
    found.  */
@@ -7147,8 +7158,8 @@ set_longjmp_breakpoint (struct thread_in
      clones of those and enable them for the requested thread.  */
   ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->pspace == current_program_space
-	&& (b->type == bp_longjmp_master
-	    || b->type == bp_exception_master))
+	&& ((b->type == bp_longjmp_master
+	    || b->type == bp_exception_master) && !b->loc->shlib_disabled))
       {
 	enum bptype type = b->type == bp_longjmp_master ? bp_longjmp : bp_exception;
 	struct breakpoint *clone;
@@ -7463,7 +7474,8 @@ disable_breakpoints_in_unloaded_shlib (s
 	      || b->type == bp_hardware_breakpoint)
 	     && (loc->loc_type == bp_loc_hardware_breakpoint
 		 || loc->loc_type == bp_loc_software_breakpoint))
-	    || is_tracepoint (b))
+	    || is_tracepoint (b)
+	    || is_removable_in_unloaded_shlib(b))
 	&& solib_contains_address_p (solib, loc->address))
       {
 	loc->shlib_disabled = 1;

Re: [PATCH] bug fix for gdb 16039

[Sergio Durigan Junior]

On Tuesday, October 15 2013, Dave Tian wrote:

> 1 Description: Gdb bug 16039 created by me
>
>   Title: Gdb next" command stop working when shared library unloaded.
>   Root Cause: There is "libc++" static linked into the shared library,
> and since gdb insert internal breakpoints on std::terminate/longjump/...
> so after dlclose, the memory address is
> invalid,remove_breakpoints/insert_breakpoints
> failed with EIO error.
>
>   Fix: Disable the internal breakpoints when dlclose hit.

Almost there, Dave!  Thanks :-).

> 2 ChangeLog:
>
> 2013-10-16  Tian Ye  <xhengdf@gmail.com>
>
> 	PR gdb/16039
> 	* breakpoint.c (is_removable_in_unloaded_shlib): New.
                                                         ^^^^

Write "New function" instead.

> 	(set_longjmp_breakpoint): Check if breakpoint's location
> 	address is not in an unloaded shared library
> 	(disable_breakpoints_in_unloaded_shlib): Disable the
> 	internal breakpoints in the hook function of shared
> 	library unload.
>
> 3 Patch Diffs:
>
> --- breakpoint.c.bak	2013-10-12 01:15:09.044081000 -0700
> +++ breakpoint.c	2013-10-15 18:50:59.842116000 -0700
> @@ -1118,6 +1118,17 @@ is_tracepoint (const struct breakpoint *
>    return is_tracepoint_type (b->type);
>  }
>
> +/* Breakpoints should be disable when shlib unload.  */

The comment should describe what the function does.  Something like:

  /* Return 1 if B is an internal breakpoint that can be removed when a
     shared library is unloaded, or 0 otherwise.  */

> +
> +static int
> +is_removable_in_unloaded_shlib(const struct breakpoint *b)
                                ^^^

Space between function name and parenthesis.

> +{
> +  return (b->type == bp_longjmp_master
> +	  || b->type == bp_std_terminate_master
> +	  || b->type == bp_exception_master
> +	  || b->type == bp_exception);
> +}

Also, would you mind explaining how you came to the conclusion that only
those types of internal breakpoints can be present in a shared library?
I looked a little bit through the code and could not find any
explanation about this assumption.  I remember that I commented that
maybe you should extend this function in order to identify the other
kinds of internal breakpoints...

It would be nice to hear what other maintainers think of Dave's
approach, so that he doesn't waste his with this detail.

> +
>  /* A helper function that validates that COMMANDS are valid for a
>     breakpoint.  This function will throw an exception if a problem is
>     found.  */
> @@ -7147,8 +7158,8 @@ set_longjmp_breakpoint (struct thread_in
>       clones of those and enable them for the requested thread.  */
>    ALL_BREAKPOINTS_SAFE (b, b_tmp)
>      if (b->pspace == current_program_space
> -	&& (b->type == bp_longjmp_master
> -	    || b->type == bp_exception_master))
> +	&& ((b->type == bp_longjmp_master
> +	    || b->type == bp_exception_master) && !b->loc->shlib_disabled))
>        {
>  	enum bptype type = b->type == bp_longjmp_master ? bp_longjmp : bp_exception;
>  	struct breakpoint *clone;
> @@ -7463,7 +7474,8 @@ disable_breakpoints_in_unloaded_shlib (s
>  	      || b->type == bp_hardware_breakpoint)
>  	     && (loc->loc_type == bp_loc_hardware_breakpoint
>  		 || loc->loc_type == bp_loc_software_breakpoint))
> -	    || is_tracepoint (b))
> +	    || is_tracepoint (b)
> +	    || is_removable_in_unloaded_shlib(b))
                                            ^^^

Space between function name and parenthesis.

>  	&& solib_contains_address_p (solib, loc->address))
>        {
>  	loc->shlib_disabled = 1;

-- 
Sergio

Re: [PATCH] bug fix for gdb 16039

[Dave.Tian]

1 Description: Gdb bug 16039 created by me

  Title: Gdb next" command stop working when shared library unloaded.
  Root Cause: There is "libc++" static linked into the shared library,
and since gdb insert internal breakpoints on std::terminate/longjump/...
so after dlclose, the memory address is
invalid,remove_breakpoints/insert_breakpoints
failed with EIO error.

  Fix: Disable the internal breakpoints when dlclose hit.

2 ChangeLog:

2013-10-16  Tian Ye  <xhengdf@gmail.com>

        PR gdb/16039
        * breakpoint.c (is_removable_in_unloaded_shlib): New function.
        (set_longjmp_breakpoint): Check if breakpoint's location
        address is not in an unloaded shared library
        (disable_breakpoints_in_unloaded_shlib): Disable the
        internal breakpoints in the hook function of shared
        library unload.

3 Patch Diffs:

--- breakpoint.c.bak	2013-10-12 01:15:09.044081000 -0700
+++ breakpoint.c	2013-10-15 23:10:50.241167000 -0700
@@ -1118,6 +1118,23 @@ is_tracepoint (const struct breakpoint *
   return is_tracepoint_type (b->type);
 }

+/* Return 1 if B is an internal memory breakpoint that
+ * can be removed when shlib is unloaded, or 0 otherwise.  */
+
+static int
+is_removable_membreak_in_unloaded_shlib (const struct breakpoint *b)
+{
+  return (b->type == bp_longjmp
+	  || b->type == bp_longjmp_resume
+	  || b->type == bp_longjmp_call_dummy
+	  || b->type == bp_exception
+	  || b->type == bp_exception_resume
+	  || b->type == bp_std_terminate
+	  || b->type == bp_longjmp_master
+	  || b->type == bp_std_terminate_master
+	  || b->type == bp_exception_master);
+}
+
 /* A helper function that validates that COMMANDS are valid for a
    breakpoint.  This function will throw an exception if a problem is
    found.  */
@@ -7147,8 +7164,8 @@ set_longjmp_breakpoint (struct thread_in
      clones of those and enable them for the requested thread.  */
   ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->pspace == current_program_space
-	&& (b->type == bp_longjmp_master
-	    || b->type == bp_exception_master))
+	&& ((b->type == bp_longjmp_master
+	    || b->type == bp_exception_master) && !b->loc->shlib_disabled))
       {
 	enum bptype type = b->type == bp_longjmp_master ? bp_longjmp : bp_exception;
 	struct breakpoint *clone;
@@ -7463,7 +7480,8 @@ disable_breakpoints_in_unloaded_shlib (s
 	      || b->type == bp_hardware_breakpoint)
 	     && (loc->loc_type == bp_loc_hardware_breakpoint
 		 || loc->loc_type == bp_loc_software_breakpoint))
-	    || is_tracepoint (b))
+	    || is_tracepoint (b)
+	    || is_removable_in_unloaded_shlib (b))
 	&& solib_contains_address_p (solib, loc->address))
       {
 	loc->shlib_disabled = 1;

thank you,
-YT

2013/10/15 Sergio Durigan Junior <sergiodj@redhat.com>:
> On Tuesday, October 15 2013, Dave Tian wrote:
>
>> 1 Description: Gdb bug 16039 created by me
>>
>>   Title: Gdb next" command stop working when shared library unloaded.
>>   Root Cause: There is "libc++" static linked into the shared library,
>> and since gdb insert internal breakpoints on std::terminate/longjump/...
>> so after dlclose, the memory address is
>> invalid,remove_breakpoints/insert_breakpoints
>> failed with EIO error.
>>
>>   Fix: Disable the internal breakpoints when dlclose hit.
>
> Almost there, Dave!  Thanks :-).
>
>> 2 ChangeLog:
>>
>> 2013-10-16  Tian Ye  <xhengdf@gmail.com>
>>
>>       PR gdb/16039
>>       * breakpoint.c (is_removable_in_unloaded_shlib): New.
>                                                          ^^^^
>
> Write "New function" instead.
>
>>       (set_longjmp_breakpoint): Check if breakpoint's location
>>       address is not in an unloaded shared library
>>       (disable_breakpoints_in_unloaded_shlib): Disable the
>>       internal breakpoints in the hook function of shared
>>       library unload.
>>
>> 3 Patch Diffs:
>>
>> --- breakpoint.c.bak  2013-10-12 01:15:09.044081000 -0700
>> +++ breakpoint.c      2013-10-15 18:50:59.842116000 -0700
>> @@ -1118,6 +1118,17 @@ is_tracepoint (const struct breakpoint *
>>    return is_tracepoint_type (b->type);
>>  }
>>
>> +/* Breakpoints should be disable when shlib unload.  */
>
> The comment should describe what the function does.  Something like:
>
>   /* Return 1 if B is an internal breakpoint that can be removed when a
>      shared library is unloaded, or 0 otherwise.  */
>
>> +
>> +static int
>> +is_removable_in_unloaded_shlib(const struct breakpoint *b)
>                                 ^^^
>
> Space between function name and parenthesis.
>
>> +{
>> +  return (b->type == bp_longjmp_master
>> +       || b->type == bp_std_terminate_master
>> +       || b->type == bp_exception_master
>> +       || b->type == bp_exception);
>> +}
>
> Also, would you mind explaining how you came to the conclusion that only
> those types of internal breakpoints can be present in a shared library?
> I looked a little bit through the code and could not find any
> explanation about this assumption.  I remember that I commented that
> maybe you should extend this function in order to identify the other
> kinds of internal breakpoints...
>
> It would be nice to hear what other maintainers think of Dave's
> approach, so that he doesn't waste his with this detail.
>
>> +
>>  /* A helper function that validates that COMMANDS are valid for a
>>     breakpoint.  This function will throw an exception if a problem is
>>     found.  */
>> @@ -7147,8 +7158,8 @@ set_longjmp_breakpoint (struct thread_in
>>       clones of those and enable them for the requested thread.  */
>>    ALL_BREAKPOINTS_SAFE (b, b_tmp)
>>      if (b->pspace == current_program_space
>> -     && (b->type == bp_longjmp_master
>> -         || b->type == bp_exception_master))
>> +     && ((b->type == bp_longjmp_master
>> +         || b->type == bp_exception_master) && !b->loc->shlib_disabled))
>>        {
>>       enum bptype type = b->type == bp_longjmp_master ? bp_longjmp : bp_exception;
>>       struct breakpoint *clone;
>> @@ -7463,7 +7474,8 @@ disable_breakpoints_in_unloaded_shlib (s
>>             || b->type == bp_hardware_breakpoint)
>>            && (loc->loc_type == bp_loc_hardware_breakpoint
>>                || loc->loc_type == bp_loc_software_breakpoint))
>> -         || is_tracepoint (b))
>> +         || is_tracepoint (b)
>> +         || is_removable_in_unloaded_shlib(b))
>                                             ^^^
>
> Space between function name and parenthesis.
>
>>       && solib_contains_address_p (solib, loc->address))
>>        {
>>       loc->shlib_disabled = 1;
>
> --
> Sergio

Re: [PATCH] bug fix for gdb 16039

[Tom Tromey]

>>>>> ">" == Dave Tian <xhengdf@gmail.com> writes:

>> 1 Description: Gdb bug 16039 created by me
>>   Title: Gdb next" command stop working when shared library unloaded.
>>   Root Cause: There is "libc++" static linked into the shared library,
>> and since gdb insert internal breakpoints on std::terminate/longjump/...
>> so after dlclose, the memory address is
>> invalid,remove_breakpoints/insert_breakpoints
>> failed with EIO error.

The patch seems reasonable to me, but I think it would be good to have a
regression test for it.

Tom

Re: [PATCH] bug fix for gdb 16039

[Sergio Durigan Junior]

On Wednesday, October 16 2013, Dave Tian wrote:

> 1 Description: Gdb bug 16039 created by me
>
>   Title: Gdb next" command stop working when shared library unloaded.
>   Root Cause: There is "libc++" static linked into the shared library,
> and since gdb insert internal breakpoints on std::terminate/longjump/...
> so after dlclose, the memory address is
> invalid,remove_breakpoints/insert_breakpoints
> failed with EIO error.
>
>   Fix: Disable the internal breakpoints when dlclose hit.
>
> 2 ChangeLog:
>
> 2013-10-16  Tian Ye  <xhengdf@gmail.com>
>
>         PR gdb/16039
>         * breakpoint.c (is_removable_in_unloaded_shlib): New function.
>         (set_longjmp_breakpoint): Check if breakpoint's location
>         address is not in an unloaded shared library
>         (disable_breakpoints_in_unloaded_shlib): Disable the
>         internal breakpoints in the hook function of shared
>         library unload.

Thanks for the patch.

Just FYI: the formatting of this ChangeLog entry is wrong because it
doesn't use TABs.  However, the entry you posted on the previous version
of this patch was right, so I think this was just a copy & paste done
wrong :-).  Anyway, just FYI as I said.

I would also like to point that you still haven't contacted me about the
copyright assignment.  You need to do that paperwork before your patch
can be committed (don't worry, it's just this time).  Just send me an
email offlist and I can send you the instructions.

> 3 Patch Diffs:
>
> --- breakpoint.c.bak	2013-10-12 01:15:09.044081000 -0700
> +++ breakpoint.c	2013-10-15 23:10:50.241167000 -0700
> @@ -1118,6 +1118,23 @@ is_tracepoint (const struct breakpoint *
>    return is_tracepoint_type (b->type);
>  }
>
> +/* Return 1 if B is an internal memory breakpoint that
> + * can be removed when shlib is unloaded, or 0 otherwise.  */
> +
> +static int
> +is_removable_membreak_in_unloaded_shlib (const struct breakpoint *b)
> +{
> +  return (b->type == bp_longjmp
> +	  || b->type == bp_longjmp_resume
> +	  || b->type == bp_longjmp_call_dummy
> +	  || b->type == bp_exception
> +	  || b->type == bp_exception_resume
> +	  || b->type == bp_std_terminate
> +	  || b->type == bp_longjmp_master
> +	  || b->type == bp_std_terminate_master
> +	  || b->type == bp_exception_master);
> +}

Thanks for extending this function, but I still think it's incomplete.
My rationale here is: I liked your previous idea of creating a function
to identify whether a breakpoint is internal.  I think it is more
complete this way.  However, such a function (IMO) should cover all the
known possibilites of internal breakpoints.  And your list is not
exhaustive, for sure.  So, IMO, you could take a closer at the possible
internal breakpoints and properly extend this list.  Also, when you do
that, you could rename your function to "is_internal_breakpoint".

Well, this is my opinion.  I don't know what others think.  But I
certainly don't like this new function the way it is.

The rest of the patch looks good to me, but you should also provide a
testcase for it as Tom noted.

Thanks a lot for doing that,

-- 
Sergio

Re: [PATCH] bug fix for gdb 16039

[Dave.Tian]

Testcase below:

----Makefile----
so:
      g++ -shared -static-libstdc++ -static-libgcc -fPIC -rdynamic
dy.cxx -m32 -o libdy.so
all: so
      g++ -m32 -ldl main.cxx -g -o main

----dy.cxx----
#include <string>
using namespace std;

extern "C" const char * getVerson()
{
    std::string verson = "1.0";
    return verson.c_str();
}

----main.cxx----
#include <dlfcn.h>
#include <stdio.h>
#include <string>
#include <iostream>
using namespace std;

int main()
{
    void *handle = dlopen("libdy.so", RTLD_LAZY);
    if (!handle) {
        fprintf (stderr, "%s\n", dlerror());
        return 1;
    }

    typedef const char*(*FUNCTION)();
    FUNCTION f = (FUNCTION)dlsym(handle, "getVerson");
    std::cout << f();

    dlclose(handle);

    // 'next' cmd stop work here
    int a, b, c;
    a = 1;
    b = 2;
    c = 3;
    std::cout << a << b << c;
}

thank you,
-YT

2013/10/19 Sergio Durigan Junior <sergiodj@redhat.com>:
> On Wednesday, October 16 2013, Dave Tian wrote:
>
>> 1 Description: Gdb bug 16039 created by me
>>
>>   Title: Gdb next" command stop working when shared library unloaded.
>>   Root Cause: There is "libc++" static linked into the shared library,
>> and since gdb insert internal breakpoints on std::terminate/longjump/...
>> so after dlclose, the memory address is
>> invalid,remove_breakpoints/insert_breakpoints
>> failed with EIO error.
>>
>>   Fix: Disable the internal breakpoints when dlclose hit.
>>
>> 2 ChangeLog:
>>
>> 2013-10-16  Tian Ye  <xhengdf@gmail.com>
>>
>>         PR gdb/16039
>>         * breakpoint.c (is_removable_in_unloaded_shlib): New function.
>>         (set_longjmp_breakpoint): Check if breakpoint's location
>>         address is not in an unloaded shared library
>>         (disable_breakpoints_in_unloaded_shlib): Disable the
>>         internal breakpoints in the hook function of shared
>>         library unload.
>
> Thanks for the patch.
>
> Just FYI: the formatting of this ChangeLog entry is wrong because it
> doesn't use TABs.  However, the entry you posted on the previous version
> of this patch was right, so I think this was just a copy & paste done
> wrong :-).  Anyway, just FYI as I said.
>
> I would also like to point that you still haven't contacted me about the
> copyright assignment.  You need to do that paperwork before your patch
> can be committed (don't worry, it's just this time).  Just send me an
> email offlist and I can send you the instructions.
>
>> 3 Patch Diffs:
>>
>> --- breakpoint.c.bak  2013-10-12 01:15:09.044081000 -0700
>> +++ breakpoint.c      2013-10-15 23:10:50.241167000 -0700
>> @@ -1118,6 +1118,23 @@ is_tracepoint (const struct breakpoint *
>>    return is_tracepoint_type (b->type);
>>  }
>>
>> +/* Return 1 if B is an internal memory breakpoint that
>> + * can be removed when shlib is unloaded, or 0 otherwise.  */
>> +
>> +static int
>> +is_removable_membreak_in_unloaded_shlib (const struct breakpoint *b)
>> +{
>> +  return (b->type == bp_longjmp
>> +       || b->type == bp_longjmp_resume
>> +       || b->type == bp_longjmp_call_dummy
>> +       || b->type == bp_exception
>> +       || b->type == bp_exception_resume
>> +       || b->type == bp_std_terminate
>> +       || b->type == bp_longjmp_master
>> +       || b->type == bp_std_terminate_master
>> +       || b->type == bp_exception_master);
>> +}
>
> Thanks for extending this function, but I still think it's incomplete.
> My rationale here is: I liked your previous idea of creating a function
> to identify whether a breakpoint is internal.  I think it is more
> complete this way.  However, such a function (IMO) should cover all the
> known possibilites of internal breakpoints.  And your list is not
> exhaustive, for sure.  So, IMO, you could take a closer at the possible
> internal breakpoints and properly extend this list.  Also, when you do
> that, you could rename your function to "is_internal_breakpoint".
>
> Well, this is my opinion.  I don't know what others think.  But I
> certainly don't like this new function the way it is.
>
> The rest of the patch looks good to me, but you should also provide a
> testcase for it as Tom noted.
>
> Thanks a lot for doing that,
>
> --
> Sergio

Re: [PATCH] bug fix for gdb 16039

[Sergio Durigan Junior]

On Monday, October 21 2013, Dave Tian wrote:

> Testcase below:
>
> ----Makefile----
> so:
>       g++ -shared -static-libstdc++ -static-libgcc -fPIC -rdynamic
> dy.cxx -m32 -o libdy.so
> all: so
>       g++ -m32 -ldl main.cxx -g -o main
>
> ----dy.cxx----
> #include <string>
> using namespace std;
>
> extern "C" const char * getVerson()
> {
>     std::string verson = "1.0";
>     return verson.c_str();
> }
>
> ----main.cxx----
> #include <dlfcn.h>
> #include <stdio.h>
> #include <string>
> #include <iostream>
> using namespace std;
>
> int main()
> {
>     void *handle = dlopen("libdy.so", RTLD_LAZY);
>     if (!handle) {
>         fprintf (stderr, "%s\n", dlerror());
>         return 1;
>     }
>
>     typedef const char*(*FUNCTION)();
>     FUNCTION f = (FUNCTION)dlsym(handle, "getVerson");
>     std::cout << f();
>
>     dlclose(handle);
>
>     // 'next' cmd stop work here
>     int a, b, c;
>     a = 1;
>     b = 2;
>     c = 3;
>     std::cout << a << b << c;
> }

Sorry for taking so long to respond.

Thanks, but this testcase should actually be made in the default
fashion, i.e., take a look at the directory gdb/testsuite and see the
many directories and testcases there.

You will also find this link useful:

<https://sourceware.org/gdb/wiki/GDBTestcaseCookbook>

Thanks for persevering.

-- 
Sergio