Fix problems with finishing a dummy function call on simulators.
Commit Message
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!
Comments
On 06/17/2015 02:26 PM, Luis Machado wrote:
> On 06/17/2015 09:40 AM, Pedro Alves wrote:
>> Yep. Close, but also remove the bp_call_dummy check in
>> bp_loc_is_permanent, and merge in its comment, like ...
>>
>
> Fixed now.
Looks good now. Thanks.
> The attached updated patch also removes this dead function. I can commit
> it as a separate patch if you're not removing it yourself.
Nah, I was hoping you'd remove it. :-) Please also remove the
make_breakpoint_permanent declaration from breakpoint.h though.
Thanks,
Pedro Alves
On 06/17/2015 10:43 AM, Pedro Alves wrote:
> On 06/17/2015 02:26 PM, Luis Machado wrote:
>> On 06/17/2015 09:40 AM, Pedro Alves wrote:
>
>>> Yep. Close, but also remove the bp_call_dummy check in
>>> bp_loc_is_permanent, and merge in its comment, like ...
>>>
>>
>> Fixed now.
>
> Looks good now. Thanks.
>
>> The attached updated patch also removes this dead function. I can commit
>> it as a separate patch if you're not removing it yourself.
>
> Nah, I was hoping you'd remove it. :-) Please also remove the
> make_breakpoint_permanent declaration from breakpoint.h though.
Thanks. I've pushed this in now and pushed the removal with a different
patch.
Hi Luis,
Looks like this regressed gdbserver somehow.
On 06/17/2015 09:16 PM, Luis Machado wrote:
> On 06/17/2015 10:43 AM, Pedro Alves wrote:
>> On 06/17/2015 02:26 PM, Luis Machado wrote:
>>> On 06/17/2015 09:40 AM, Pedro Alves wrote:
>>
>>>> Yep. Close, but also remove the bp_call_dummy check in
>>>> bp_loc_is_permanent, and merge in its comment, like ...
>>>>
>>>
>>> Fixed now.
>>
>> Looks good now. Thanks.
>>
>>> The attached updated patch also removes this dead function. I can commit
>>> it as a separate patch if you're not removing it yourself.
>>
>> Nah, I was hoping you'd remove it. :-) Please also remove the
>> make_breakpoint_permanent declaration from breakpoint.h though.
>
> Thanks. I've pushed this in now and pushed the removal with a different
> patch.
>
Compared to before the patch, I see these:
-PASS: gdb.base/shlib-call.exp: print mainshr1(1)
-PASS: gdb.base/shlib-call.exp: step into mainshr1
+FAIL: gdb.base/shlib-call.exp: print mainshr1(1)
+FAIL: gdb.base/shlib-call.exp: step into mainshr1
PASS: gdb.base/shlib-call.exp: set print sevenbit-strings
PASS: gdb.cp/chained-calls.exp: g(f())
-PASS: gdb.cp/chained-calls.exp: q(p())
+FAIL: gdb.cp/chained-calls.exp: q(p())
PASS: gdb.cp/chained-calls.exp: p() + r()
-PASS: gdb.cp/chained-calls.exp: q(p() + r())
+FAIL: gdb.cp/chained-calls.exp: q(p() + r())
PASS: gdb.cp/chained-calls.exp: g(f() + f())
-PASS: gdb.cp/chained-calls.exp: g(f(g(f() + f())) + f())
+FAIL: gdb.cp/chained-calls.exp: g(f(g(f() + f())) + f())
PASS: gdb.cp/chained-calls.exp: getb(makeb(), ...)
-PASS: gdb.cp/chained-calls.exp: *c
-PASS: gdb.cp/chained-calls.exp: *c + *c
-PASS: gdb.cp/chained-calls.exp: q(*c + *c)
+FAIL: gdb.cp/chained-calls.exp: *c
+FAIL: gdb.cp/chained-calls.exp: *c + *c
+FAIL: gdb.cp/chained-calls.exp: q(*c + *c)
PASS: gdb.cp/classes.exp: print cnsi with static members
PASS: gdb.cp/classes.exp: finish from marker_reg1
-PASS: gdb.cp/classes.exp: calling method for small class
+FAIL: gdb.cp/classes.exp: calling method for small class
git bisect pointed to this commit:
6ae8866180bf90e9ec76c2dd34c07fd826d11a83 is the first bad commit
commit 6ae8866180bf90e9ec76c2dd34c07fd826d11a83
Author: Luis Machado <lgustavo@codesourcery.com>
Date: Wed Jun 17 16:50:57 2015 -0300
Fix problems with finishing a dummy function call on simulators.
I don't have an offhand explanation for the regressions.
This is just:
make check RUNTESTFLAGS="--target_board=native-gdbserver"
on x86_64 Fedora 20.
Thanks,
Pedro Alves
On 07/06/2015 12:06 PM, Pedro Alves wrote:
> Hi Luis,
>
> Looks like this regressed gdbserver somehow.
>
> On 06/17/2015 09:16 PM, Luis Machado wrote:
>> On 06/17/2015 10:43 AM, Pedro Alves wrote:
>>> On 06/17/2015 02:26 PM, Luis Machado wrote:
>>>> On 06/17/2015 09:40 AM, Pedro Alves wrote:
>>>
>>>>> Yep. Close, but also remove the bp_call_dummy check in
>>>>> bp_loc_is_permanent, and merge in its comment, like ...
>>>>>
>>>>
>>>> Fixed now.
>>>
>>> Looks good now. Thanks.
>>>
>>>> The attached updated patch also removes this dead function. I can commit
>>>> it as a separate patch if you're not removing it yourself.
>>>
>>> Nah, I was hoping you'd remove it. :-) Please also remove the
>>> make_breakpoint_permanent declaration from breakpoint.h though.
>>
>> Thanks. I've pushed this in now and pushed the removal with a different
>> patch.
>>
>
> Compared to before the patch, I see these:
>
> -PASS: gdb.base/shlib-call.exp: print mainshr1(1)
> -PASS: gdb.base/shlib-call.exp: step into mainshr1
> +FAIL: gdb.base/shlib-call.exp: print mainshr1(1)
> +FAIL: gdb.base/shlib-call.exp: step into mainshr1
> PASS: gdb.base/shlib-call.exp: set print sevenbit-strings
> PASS: gdb.cp/chained-calls.exp: g(f())
> -PASS: gdb.cp/chained-calls.exp: q(p())
> +FAIL: gdb.cp/chained-calls.exp: q(p())
> PASS: gdb.cp/chained-calls.exp: p() + r()
> -PASS: gdb.cp/chained-calls.exp: q(p() + r())
> +FAIL: gdb.cp/chained-calls.exp: q(p() + r())
> PASS: gdb.cp/chained-calls.exp: g(f() + f())
> -PASS: gdb.cp/chained-calls.exp: g(f(g(f() + f())) + f())
> +FAIL: gdb.cp/chained-calls.exp: g(f(g(f() + f())) + f())
> PASS: gdb.cp/chained-calls.exp: getb(makeb(), ...)
> -PASS: gdb.cp/chained-calls.exp: *c
> -PASS: gdb.cp/chained-calls.exp: *c + *c
> -PASS: gdb.cp/chained-calls.exp: q(*c + *c)
> +FAIL: gdb.cp/chained-calls.exp: *c
> +FAIL: gdb.cp/chained-calls.exp: *c + *c
> +FAIL: gdb.cp/chained-calls.exp: q(*c + *c)
>
> PASS: gdb.cp/classes.exp: print cnsi with static members
> PASS: gdb.cp/classes.exp: finish from marker_reg1
> -PASS: gdb.cp/classes.exp: calling method for small class
> +FAIL: gdb.cp/classes.exp: calling method for small class
>
> git bisect pointed to this commit:
>
> 6ae8866180bf90e9ec76c2dd34c07fd826d11a83 is the first bad commit
> commit 6ae8866180bf90e9ec76c2dd34c07fd826d11a83
> Author: Luis Machado <lgustavo@codesourcery.com>
> Date: Wed Jun 17 16:50:57 2015 -0300
>
> Fix problems with finishing a dummy function call on simulators.
>
> I don't have an offhand explanation for the regressions.
>
> This is just:
>
> make check RUNTESTFLAGS="--target_board=native-gdbserver"
>
> on x86_64 Fedora 20.
>
> Thanks,
> Pedro Alves
>
I'll take a look at it. I suppose this will block the branching?
Then again, simply reverting this will still have bad results with some
simulators.
2015-06-17 Luis Machado <lgustavo@codesourcery.com>
* breakpoint.c (make_breakpoint_permanent): Remove unused
function.
(add_location_to_breakpoint): Don't mark permanent locations as
inserted.
Update and expand comment about permanent locations.
(bp_loc_is_permanent): Don't return 0 for bp_call_dummy.
Move comment to add_location_to_breakpoint.
(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.
@@ -7431,26 +7431,6 @@ set_raw_breakpoint (struct gdbarch *gdbarch,
return b;
}
-
-/* Note that the breakpoint object B describes a permanent breakpoint
- instruction, hard-wired into the inferior's code. */
-void
-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. */
- for (bl = b->loc; bl; bl = bl->next)
- {
- bl->permanent = 1;
- bl->inserted = 1;
- }
-}
-
/* Call this routine when stepping and nexting to enable a breakpoint
if we do a longjmp() or 'throw' in TP. FRAME is the frame which
initiated the operation. */
@@ -8918,11 +8898,21 @@ add_location_to_breakpoint (struct breakpoint *b,
set_breakpoint_location_function (loc,
sal->explicit_pc || sal->explicit_line);
+ /* 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.) */
if (bp_loc_is_permanent (loc))
- {
- loc->inserted = 1;
- loc->permanent = 1;
- }
+ loc->permanent = 1;
return loc;
}
@@ -8974,20 +8964,6 @@ bp_loc_is_permanent (struct bp_location *loc)
gdb_assert (loc != NULL);
- /* bp_call_dummy breakpoint locations are usually memory locations
- where GDB just wrote a breakpoint instruction, making it look
- as if there is a permanent breakpoint at that location. Considering
- it permanent makes GDB rely on that breakpoint instruction to stop
- the program, thus removing the need to insert its own breakpoint
- there. This is normally expected to work, except that some versions
- of QEMU (Eg: QEMU 2.0.0 for SPARC) just report a fatal problem (Trap
- 0x02 while interrupts disabled, Error state) instead of reporting
- a SIGTRAP. QEMU should probably be fixed, but in the interest of
- compatibility with versions that behave this way, we always consider
- bp_call_dummy breakpoint locations as non-permanent. */
- if (loc->owner->type == bp_call_dummy)
- return 0;
-
cleanup = save_current_space_and_thread ();
switch_to_program_space_and_thread (loc->pspace);
@@ -12430,12 +12406,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)
@@ -12471,12 +12441,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 ())