Re: [PATCH] Stale breakpoint instructions, spurious SIGTRAPS.

[Pedro Alves <palves@redhat.com>]

On 04/16/2014 06:18 PM, Blanc, Nicolas wrote:
> Hi Pedro,
> 
>> Then, the reason that disable_breakpoints_in_freed_objfile was clearing the inserted flag isn't clear, but likely to avoid breakpoint removal errors, assuming remove-symbol-file was called after the dynamic object was already unmapped from the inferior. 
> 
> Indeed that was the intend. The clearing of the flag copied from disable_breakpoints_in_unloaded_shlib.

I see.

> 
>>       if (val)
>> @@ -7666,7 +7693,7 @@ disable_breakpoints_in_freed_objfile (struct objfile *objfile)
>> ?   /* If the file is a shared library not loaded by the user then
>>      solib_unloaded was notified and disable_breakpoints_in_unloaded_shlib
>>       was called.  In that case there is no need to take action again.  */
>> -  if ((objfile->flags & OBJF_SHARED) && !(objfile->flags & OBJF_USERLOADED))
>> +  if ((objfile->flags & OBJF_USERLOADED) == 0)
>>      return;
> 
> The comment above should be updated. The condition is  now weaker. 

I've updated the comment.

> 
> 
>    ALL_BREAKPOINTS (b)
> @@ -7698,7 +7725,11 @@ disable_breakpoints_in_freed_objfile (struct objfile *objfile)
>> 	  if (is_addr_in_objfile (loc_addr, objfile))
>> 	    {
>> 	      loc->shlib_disabled = 1;
>> -	      loc->inserted = 0;
>> +	      /* At this point, we don't know whether the object was
>> +		 unmapped from the inferior or not, so leave the
>> +		 inserted flag alone.  We'll handle failure to
>> +		 uninsert quietly, in case the object was indeed
>> +		 unmapped.  */
> 
> Would it work to simplify disable_breakpoints_in_unloaded_shlib in the same way?
> Note that I understand it might be unnecessary risky to fix a function that is not broken.

Yeah.  In fact, it seems to me that by just clearing the
inserted flag, we leave stale HW breakpoints planted in the target,
just like in the "remove-symbol-file" case.  So we should either fix
it in the same way, or only clear the flag for software breakpoints.

> 
>> +  /* Make sure we see the memory breakpoints.  */  cleanup = 
>> + make_show_memory_breakpoints_cleanup (1);  val = target_read_memory 
>> + (addr, old_contents, bplen);
> 
> It was not immediately clear to me that old_contents means current content.

Indeed.  I had just copied this from ppc_linux_memory_remove_breakpoint
and not noticed that.  I've renamed it in the version pushed.

Thanks,
-- 
Pedro Alves