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

Luis Machado
2015-06-16T17:39:38Z

[-- 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 ())