On 06/17/2015 09:40 AM, Pedro Alves wrote:
> On 06/09/2015 07:22 PM, Luis Machado wrote:
>> 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.
>

I went back to this and yes, it is indeed racy. It just so happens that 
it failed both times i checked the results of an unpatched testsuite 
run. :-)

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

Fixed now.

>> 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;
>>   }
>>

I've updated this now.

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

Ah, that explains why i couldn't find callers of that function, but the 
build didn't fail due to it being unused.

The attached updated patch also removes this dead function. I can commit 
it as a separate patch if you're not removing it yourself.

Thanks!