Re: [PATCH] bug fix for gdb 16039

[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