Re: [PATCH] bug fix for gdb 16039

["Dave.Tian" <xhengdf@gmail.com>]

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