This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] bug fix for gdb 16039
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: "Dave.Tian" <xhengdf at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Sat, 19 Oct 2013 01:38:25 -0300
- Subject: Re: [PATCH] bug fix for gdb 16039
- Authentication-results: sourceware.org; auth=none
- References: <CAKjgB1NmhyvBFfBF2HgQc-9KP0OMPMb9fNXJAe3QxejdD3-X1g at mail dot gmail dot com> <m3bo2pj3ha dot fsf at redhat dot com> <CAKjgB1OyrUtz9xqcqDSxEe72kDGEWBKy9LxZEkXDDd7RbX7y=A at mail dot gmail dot 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