[rfc] Fix removing breakpoint from shared library race

[rfc] Fix removing breakpoint from shared library race

[Ulrich Weigand]

Hello,

after a shared library was unloaded, we can no longer insert breakpoints
into its (no longer present) code segment.  Therefore, code in breakpoint.c
(disable_breakpoints_in_unloaded_shlib etc.) takes care to disable such
breakpoints.

However, in a multi-threaded application we cannot really guarantee that
we have noticed the shlib unload event at the time breakpoints are to be
inserted or removed.  For the insertion case, insert_bp_location therefore
has its own check, and handles unloaded shared libraries appropriately.

When *removing* breakpoints, however, there is no such check.  I have a
multi-threaded test case that reproducibly runs into an error when trying
to remove a breakpoint from a shared library that was *just* unloaded.

The patch below fixes this, by simply silently ignoring failures to remove
a breakpoint from a shared library code segment.  The breakpoint will be
cleanly disabled once disable_breakpoints_in_unloaded_shlib gets a chance
to run (or at the next attempt to insert it).

Am I missing some reason why we shouldn't get to this point?  Otherwise,
this seems a reasonble solution to me ...

Tested on powerpc-linux and powerpc64-linux.


Bye,
UIrich


ChangeLog:

	* breakpoint.c (remove_breakpoint): Do not fail if unable to remove
	breakpoint from shared library.


diff -urNp gdb-orig/gdb/breakpoint.c gdb-head/gdb/breakpoint.c
--- gdb-orig/gdb/breakpoint.c	2008-08-08 16:42:41.000000000 +0200
+++ gdb-head/gdb/breakpoint.c	2008-08-13 21:56:44.567419172 +0200
@@ -1642,6 +1642,13 @@ remove_breakpoint (struct bp_location *b
 	      val = 0;
 	    }
 	}
+
+      /* In some cases, we might not be able to remove a breakpoint
+	 in a shared library that has already been removed, but we
+	 have not yet processed the shlib unload event.  */
+      if (val && solib_address (b->address))
+	val = 0;
+
       if (val)
 	return val;
       b->inserted = (is == mark_inserted);
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix removing breakpoint from shared library race

[Joel Brobecker]

> When *removing* breakpoints, however, there is no such check.  I have a
> multi-threaded test case that reproducibly runs into an error when trying
> to remove a breakpoint from a shared library that was *just* unloaded.
[...]
> Am I missing some reason why we shouldn't get to this point?  Otherwise,
> this seems a reasonble solution to me ...

The fix looks reasonable to me, but I'm not sure, though. Perhaps
part of the problem is that I don't see how you got there in your
example (I'm having trouble figuring out a case where the debugger
would end up removing a breakpoint while a shared library has just
been unloaded). Could you post more details?

-- 
Joel

Re: [rfc] Fix removing breakpoint from shared library race

[Ulrich Weigand]

Joel Brobecker wrote:

> > When *removing* breakpoints, however, there is no such check.  I have a
> > multi-threaded test case that reproducibly runs into an error when trying
> > to remove a breakpoint from a shared library that was *just* unloaded.
> [...]
> > Am I missing some reason why we shouldn't get to this point?  Otherwise,
> > this seems a reasonble solution to me ...
> 
> The fix looks reasonable to me, but I'm not sure, though. Perhaps
> part of the problem is that I don't see how you got there in your
> example (I'm having trouble figuring out a case where the debugger
> would end up removing a breakpoint while a shared library has just
> been unloaded). Could you post more details?

The actual test case was using the Cell multi-arch debugger.  However,
I think the problem can potentially also show up in current GDB with
multi-threaded code.

Assume you have a thread A that is just unloading a shared library

 1.) app calls dlclose
 2.) dlclose unmaps shared library
 3.) dlclose calls _dl_debug_event (which GDB has a breakpoint on)
 4.) GDB gets active
 5.) GDB processes the unload event

while at the same time, thread B is doing something that invokes GDB

 A.) app runs into a breakpoint
 B.) GDB gets active
 C.) GDB stops all threads
 D.) GDB removes all breakpoints
 E.) GDB goes to user prompt

(Note that this still assumes all-stop, breakpoints-not-always-inserted
mode.  If those new modes are active, the race would probably be more
difficult to trigger.)

Now imagine the race where thread A is just in-between steps 2.) and 3.)
while thread B runs into the breakpoint (which causes GDB to stop thread
A at this point).  When GDB now performs action D.), the breakpoints that
were inserted into the shared library are still on GDB's list as active,
because step 5.) which would have set them to solib_disabled state did
not yet run.  However, attempting to remove the breakpoint will fail 
because the library was already unmapped.

This is the situation that my patch attempts to address, by having the
remove_breakpoints not fail.  At some later point, either when GDB actually
gets to process the solib event, or when GDB tries to re-insert the
breakpoints, GDB will properly recognize that the library has been 
unloaded and sets the state to solib_disabled.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix removing breakpoint from shared library race

[Ulrich Weigand]

> ChangeLog:
> 
> 	* breakpoint.c (remove_breakpoint): Do not fail if unable to remove
> 	breakpoint from shared library.

I've checked this in now.

Bye,
Ulrich


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix removing breakpoint from shared library race

[Joel Brobecker]

Ulrich,

Sorry for the late reply, but:

> This is the situation that my patch attempts to address, by having the
> remove_breakpoints not fail.  At some later point, either when GDB
> actually gets to process the solib event, or when GDB tries to
> re-insert the breakpoints, GDB will properly recognize that the
> library has been unloaded and sets the state to solib_disabled.

Just wanted to let you know that you patch makes complete sense to me.
As you pointed out in your first message, this is in fact very similar
to what we do in insert_bp_location.

-- 
Joel

Re: [rfc] Fix removing breakpoint from shared library race

[Ulrich Weigand]

Joel Brobecker wrote:

> > This is the situation that my patch attempts to address, by having the
> > remove_breakpoints not fail.  At some later point, either when GDB
> > actually gets to process the solib event, or when GDB tries to
> > re-insert the breakpoints, GDB will properly recognize that the
> > library has been unloaded and sets the state to solib_disabled.
> 
> Just wanted to let you know that you patch makes complete sense to me.
> As you pointed out in your first message, this is in fact very similar
> to what we do in insert_bp_location.

Thanks for looking at this patch!

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com