Re: [PATCH] Fix problems with finishing a dummy function call on simulators.

[Luis Machado <lgustavo@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 4494 bytes --]

On 06/09/2015 02:51 PM, Pedro Alves wrote:
> On 06/09/2015 04:00 PM, Luis Machado wrote:
>> This is in line with what was done by Joel's patch here:
>>
>> https://sourceware.org/ml/gdb-patches/2014-11/msg00478.html
>>
>> And it also answers Pedro's question about whether this is specific to SPARC
>> QEMU or not.  This indeed seems to affect multiple QEMU targets and also other
>> simulators (proprietary).
>
> Sounds like a different issue, although related.
>
>>
>> I ran into this weird issue of not being able to "finish" an inferior function
>> call. It looks as if the program is running away, but it really is stuck
>> somewhere.  "finish" still works fine for regular functions not called manually
>> by GDB.
>
> Sounds like that would fail on SPARC qemu as well.
>
>>
>> I tracked this failure down to GDB having both a bp_call_dummy and bp_finish in
>> its breakpoint list. As a result of one not being considered permanent and the
>> other considered permanent, GDB will not issue a Z packet to force the insertion
>> of that location's breakpoint, confusing the simulator that does not know how
>> to deal properly with these permanent breakpoints that GDB inserted beforehand.
>>
>> The attached patch fixes this, though i'm inclined to say we could probably
>> check if both bp_call_dummy and bp_finish are present and force the
>> insertion of that location's breakpoint.  It isn't clear to me where exactly that
>> check would go or if it would be cleaner than checking that information in
>> the same function Joel used.
>>
>> I see no regressions on x86-64 and it fixes a bunch of failures for simulator
>> targets we use (MIPS and PowerPC to name two).
>
> If it happens that you "finish" from a normal function, and the finish
> breakpoint ends up on top of a real permanent breakpoint, then this patch
> will make us end up inserting a breakpoint on top of that permanent
> breakpoint.  I don't see what's special about finish breakpoints;
> it's the address (dummy breakpoint location) that is special.  It very much
> sounds like that any kind of breakpoint that is placed on top of the dummy
> breakpoint ends up with the same issue.  E.g., if you stepi out of
> the called function, with a software single-step breakpoint, sounds like
> GDB will miss inserting the software step breakpoint because that's
> at the same address as the dummy breakpoint.
>
> As a data point, I assume that GDB is considering the non-permanent
> dummy breakpoint a duplicate of the permanent finish breakpoint and
> then none ends up inserted.  Is that right?
>
> Not exactly sure what to do here.  Maybe we should stop considering
> permanent and non-permanent breakpoints at the same address as
> duplicates.  That should result in GDB inserting the non-permanent
> one, I think.  Or we could get stop marking permanent breakpoints
> as always inserted, and let normal breakpoints insert on top of
> permanent breakpoints normally.  See also:
>   https://sourceware.org/ml/gdb-patches/2015-03/msg00174.html

I gave the strategy of not marking permanent breakpoints/locations as 
inserted a try, and it fixes the simulator problems i've been seeing 
with the permanent breakpoint locations.

One strange side effect of this change on my local machine (x86-64) is 
that gdb.threads/attach-many-short-lived-threads.exp gives me PASS 
instead of FAIL when always-inserted mode is ON. I didn't investigate 
this further though. Is it known that this testcase is affected by 
permanent breakpoint locations?

For example:

XFAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 5: attach
(EPERM)
PASS: gdb.threads/attach-many-short-lived-threads.exp: iter 5: no new
threads
PASS: gdb.threads/attach-many-short-lived-threads.exp: iter 5: set
breakpoint always-inserted on
PASS: gdb.threads/attach-many-short-lived-threads.exp: iter 5: break
break_fn
PASS: gdb.threads/attach-many-short-lived-threads.exp: iter 5: break at
break_fn: 1
PASS: gdb.threads/attach-many-short-lived-threads.exp: iter 5: break at
break_fn: 2
PASS: gdb.threads/attach-many-short-lived-threads.exp: iter 5: break at
break_fn: 3
PASS: gdb.threads/attach-many-short-lived-threads.exp: iter 5: reset
timer in the inferior
PASS: gdb.threads/attach-many-short-lived-threads.exp: iter 5: print
seconds_left
PASS: gdb.threads/attach-many-short-lived-threads.exp: iter 5: detach
PASS: gdb.threads/attach-many-short-lived-threads.exp: iter 5: set
breakpoint always-inserted off

Is this patch what you had in mind?

Luis

[-- Attachment #2: bp_permanent.diff --]
[-- Type: text/x-patch, Size: 3056 bytes --]

2015-06-16  Luis Machado  <lgustavo@codesourcery.com>

	* breakpoint.c (make_breakpoint_permanent): Expand comment.
	Don't mark permanent locations as inserted.
	(add_location_to_breakpoint): Likewise
	(update_global_location_list): Don't error out if a permanent
	breakpoint is not marked inserted.
	Don't error out if a non-permanent breakpoint location is inserted on
	top of a permanent breakpoint.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index eb3df02..768ce59 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7440,15 +7440,16 @@ make_breakpoint_permanent (struct breakpoint *b)
   struct bp_location *bl;
 
   /* By definition, permanent breakpoints are already present in the
-     code.  Mark all locations as inserted.  For now,
-     make_breakpoint_permanent is called in just one place, so it's
-     hard to say if it's reasonable to have permanent breakpoint with
-     multiple locations or not, but it's easy to implement.  */
+     code.  For now, make_breakpoint_permanent is called in just one place, so
+     it's hard to say if it's reasonable to have permanent breakpoint with
+     multiple locations or not, but it's easy to implement.
+
+     Permanent breakpoints are not marked as inserted so we allow other
+     non-permanent locations at the same address to be inserted on top
+     of it.  This is required due to some targets, simulators mostly, not
+     dealing properly with hardwired breakpoints in the code.  */
   for (bl = b->loc; bl; bl = bl->next)
-    {
-      bl->permanent = 1;
-      bl->inserted = 1;
-    }
+    bl->permanent = 1;
 }
 
 /* Call this routine when stepping and nexting to enable a breakpoint
@@ -8918,11 +8919,10 @@ add_location_to_breakpoint (struct breakpoint *b,
   set_breakpoint_location_function (loc,
 				    sal->explicit_pc || sal->explicit_line);
 
+  /* See comment in make_breakpoint_permanent for the reason why we don't mark
+     permanent breakpoints as always inserted.  */
   if (bp_loc_is_permanent (loc))
-    {
-      loc->inserted = 1;
-      loc->permanent = 1;
-    }
+    loc->permanent = 1;
 
   return loc;
 }
@@ -12438,12 +12438,6 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 	  continue;
 	}
 
-      /* Permanent breakpoint should always be inserted.  */
-      if (loc->permanent && ! loc->inserted)
-	internal_error (__FILE__, __LINE__,
-			_("allegedly permanent breakpoint is not "
-			"actually inserted"));
-
       if (b->type == bp_hardware_watchpoint)
 	loc_first_p = &wp_loc_first;
       else if (b->type == bp_read_watchpoint)
@@ -12479,12 +12473,6 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 
       /* Clear the condition modification flag.  */
       loc->condition_changed = condition_unchanged;
-
-      if (loc->inserted && !loc->permanent
-	  && (*loc_first_p)->permanent)
-	internal_error (__FILE__, __LINE__,
-			_("another breakpoint was inserted on top of "
-			"a permanent breakpoint"));
     }
 
   if (insert_mode == UGLL_INSERT || breakpoints_should_be_inserted_now ())