Daniel Jacobowitz wrote:
> On Tue, Aug 10, 2004 at 03:09:37PM -0400, Jeff Johnston wrote:
> 
>>The following patch fixes a problem with breakpoints set in shlibs that are 
>>manually loaded/unloaded by the program.  What currently happens is that 
>>pending breakpoints work properly for the first run of the program.  On the 
>>2nd run, the resolved breakpoint(s) can end up at the start of the 
>>breakpoint list and is marked bp_shlib_disabled.  This is fine for a bit 
>>and we reach the breakpoint again when the shared library is loaded.  
>>However, when we unload the 2nd time, there is trouble.  We eventually get 
>>a shlib_event from the dlclose() and we attempt to remove the breakpoint to 
>>step over it.  Unfortunately, we try and remove all breakpoints and we end 
>>attempting to remove a breakpoint that no longer exists (remember the 
>>breakpoint for the shared library routine is now at the start of the 
>>breakpoint list).  We fail trying to remove the first breakpoint and end up 
>>failing remove_breakpoints.  We subsequently keep running into the 
>>shlib_event breakpoint over and over again ad-infinitum.
> 
> 
> I couldn't quite follow your explanation of the problem, but FWIW your
> patch does make sense to me.
> 
> Please check it for coding style problems; I noticed a lot of operators
> at the end of lines instead of the beginning of the next line.
>

Noted.  Fixed in appended patch.

> 
>>+#if defined (PC_SOLIB)
>>+    if (((b->type == bp_breakpoint) ||
>>+	 (b->type == bp_hardware_breakpoint)) &&
>>+	breakpoint_enabled (b) &&
>>+	!b->loc->duplicate)
> 
> 
> You are just grabbing this from disable_breakpoints_in_shlibs, but the
> b->type check is not correct.  Try
>   (b->loc->type == bp_loc_hardware_breakpoint
>    || b->loc->type == bp_loc_software_breakpoint)
> 

Done.

> [Conceptually, you want any breakpoint which corresponds to a code
> address.]
> 
> Can this code be commonized rather than duplicated?
>

I don't see much value; the code is small, the test is now different, the input 
parameter is different, and there is resetting of the inserted flag.

> 
>>+      {
>>+	char *so_name = PC_SOLIB (b->loc->address);
>>+	if (so_name &&
>>+	    !strcmp (so_name, solib->so_name))
>>+          {
>>+	    b->enable_state = bp_shlib_disabled;
>>+	    b->loc->inserted = 0;
> 
> 
> Are we guaranteed that the breakpoint is not inserted right now?  This
> is the only place in breakpoint.c that changes the inserted flag
> directly other than initialization, insertion, and a hack for exec
> following.
> 
> If you expect that the library has already been unmapped, so removing
> it would fail, please add a comment saying so.
>

Putting remove_breakpoint() at that point does not work.  I have put the 
following comment:

+           /* At this point, we cannot rely on remove_breakpoint
+              succeeding so we must mark the breakpoint as not inserted
+              to prevent future errors occurring in remove_breakpoints.  */

Ok?

-- Jeff J.