Re: [PATCH] bug fix for gdb 16039

[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