Fix problems with finishing a dummy function call on simulators.

  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
  

On 06/09/2015 07:22 PM, Luis Machado wrote:
> Both the above and the mail sound like workaround ideas. You mentioned
> even more special casing in the mail. It is the amount of special casing
> that i'm afraid of.

I was actually proposing to remove the special casing.  :-)

On 06/16/2015 06:39 PM, Luis Machado wrote:

> 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.

Thanks.

> 
> 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. 

You mean you _always_ get a FAIL before your patch?  This test sometimes
FAILs for an unknown reason, but it's racy -- it should be passing most of
the time.

> Is it known that this testcase is affected by 
> permanent breakpoint locations?

No.

> Is this patch what you had in mind?

Yep.  Close, but also remove the bp_call_dummy check in
bp_loc_is_permanent, and merge in its comment, like ...

> 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.  */

... this:

    /* While by definition, permanent breakpoints are already present in the
       code, we don't mark the location as inserted.  Normally one would expect
       that GDB could rely on that breakpoint instruction to stop the program, thus
       removing the need to insert its own breakpoint, except that executing the
       breakpoint instruction can kill the target instead of reporting a SIGTRAP.
       E.g., on SPARC, when interrupts are disabled, executing the instruction
       resets the CPU, so QEMU 2.0.0 for SPARC correspondingly dies
       with "Trap 0x02 while interrupts disabled, Error state".  Letting the
       breakpoint be inserted normally results in QEMU knowing about the GDB
       breakpoint, and thus trap before the breakpoint instruction is executed.
       (If GDB later needs to continue execution past the permanent breakpoint,
       it manually increments the PC, thus avoiding executing the breakpoint
       instruction.)

>    for (bl = b->loc; bl; bl = bl->next)
> -    {
> -      bl->permanent = 1;
> -      bl->inserted = 1;
> -    }
> +    bl->permanent = 1;
>  }
>

Actually, make_breakpoint_permanent is dead and should be deleted.  The
last remaining caller is finally gone - it was one of the old Unix ports
we removed.  So the comment should be moved to add_location_to_breakpoint
instead.

Thanks,
Pedro Alves