* [PATCH] Fix problems with finishing a dummy function call on simulators. @ 2015-06-09 15:01 Luis Machado 2015-06-09 17:51 ` Pedro Alves 0 siblings, 1 reply; 18+ messages in thread From: Luis Machado @ 2015-06-09 15:01 UTC (permalink / raw) To: gdb-patches 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). 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. 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). gdb/ChangeLog: 2015-06-09 Luis Machado <lgustavo@codesourcery.com> * breakpoint.c (bp_loc_is_permanent): Return 0 for bp_finish as well. --- gdb/breakpoint.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 657c58e..eb3df02 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -8984,8 +8984,16 @@ bp_loc_is_permanent (struct bp_location *loc) 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) + bp_call_dummy breakpoint locations as non-permanent. + + Another situation arises when we have a bp_call_dummy breakpoint inserted + and then the user issues a finish, triggering GDB to create a bp_finish + breakpoint to handle the return from the inferior function call. When + both bp_call_dummy and bp_finish breakpoints are present, GDB will not + force the insertion of these locations, triggering, once again, the + problem described above. Therefore we check for bp_finish here as + well. */ + if (loc->owner->type == bp_call_dummy || loc->owner->type == bp_finish) return 0; cleanup = save_current_space_and_thread (); -- 1.9.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-06-09 15:01 [PATCH] Fix problems with finishing a dummy function call on simulators Luis Machado @ 2015-06-09 17:51 ` Pedro Alves 2015-06-09 18:10 ` Luis Machado 2015-06-16 17:39 ` Luis Machado 0 siblings, 2 replies; 18+ messages in thread From: Pedro Alves @ 2015-06-09 17:51 UTC (permalink / raw) To: Luis Machado, gdb-patches 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 Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-06-09 17:51 ` Pedro Alves @ 2015-06-09 18:10 ` Luis Machado 2015-06-09 18:13 ` Pedro Alves 2015-06-16 17:39 ` Luis Machado 1 sibling, 1 reply; 18+ messages in thread From: Luis Machado @ 2015-06-09 18:10 UTC (permalink / raw) To: Pedro Alves, gdb-patches 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. Yes, i meant breakpoints as the address themselves, so a location. It is probably the case that using permanent breakpoints and mixing them with other types of non-permanent breakpoints is causing issues, though the only well-exercised testcase is the finish-after-dummy-call one. I do recall once getting stuck with a stepi inside a dummy function call, so i may have hit what you suggested here. > > 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? That is correct. And one of them is already considered inserted. > > 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 That sounds a bit hacky. Doesn't that defeat the purpose of having permanent breakpoints in the first place? It looks like non-gdbserver targets are not ready to support these tricky constructs/optimizations unfortunately. I'm afraid adding more hacks here and there will cause the code to get even more confusing without a generous amount of code comments. And i'm not even sure the bp_finish check is the best solution either. After all, there is the stepi case too. We could probably fix the simulators, but then again there are proprietary ones we cannot easily fix. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-06-09 18:10 ` Luis Machado @ 2015-06-09 18:13 ` Pedro Alves 2015-06-09 18:22 ` Luis Machado 2015-06-09 18:34 ` Luis Machado 0 siblings, 2 replies; 18+ messages in thread From: Pedro Alves @ 2015-06-09 18:13 UTC (permalink / raw) To: Luis Machado, gdb-patches On 06/09/2015 07:10 PM, Luis Machado wrote: >> 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 > > That sounds a bit hacky. Can you clarify? There are two suggestions above, in addition to a url showing even more ideas. So I don't know what you're referring to. :-) Thanks, Pedro Alves > Doesn't that defeat the purpose of having > permanent breakpoints in the first place? > > It looks like non-gdbserver targets are not ready to support these > tricky constructs/optimizations unfortunately. I'm afraid adding more > hacks here and there will cause the code to get even more confusing > without a generous amount of code comments. And i'm not even sure the > bp_finish check is the best solution either. After all, there is the > stepi case too. > > We could probably fix the simulators, but then again there are > proprietary ones we cannot easily fix. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-06-09 18:13 ` Pedro Alves @ 2015-06-09 18:22 ` Luis Machado 2015-06-09 18:34 ` Luis Machado 1 sibling, 0 replies; 18+ messages in thread From: Luis Machado @ 2015-06-09 18:22 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 06/09/2015 03:13 PM, Pedro Alves wrote: > On 06/09/2015 07:10 PM, Luis Machado wrote: > >>> 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 >> >> That sounds a bit hacky. > > Can you clarify? There are two suggestions above, in addition > to a url showing even more ideas. So I don't know what you're > referring to. :-) 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. Having more special cases feel like they defeat the purpose of having those permanent breakpoints if we have to IF our way through them. Am i missing something? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-06-09 18:13 ` Pedro Alves 2015-06-09 18:22 ` Luis Machado @ 2015-06-09 18:34 ` Luis Machado 1 sibling, 0 replies; 18+ messages in thread From: Luis Machado @ 2015-06-09 18:34 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 06/09/2015 03:13 PM, Pedro Alves wrote: > On 06/09/2015 07:10 PM, Luis Machado wrote: > >>> 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 >> >> That sounds a bit hacky. > > Can you clarify? There are two suggestions above, in addition > to a url showing even more ideas. So I don't know what you're > referring to. :-) > > Thanks, > Pedro Alves > >> Doesn't that defeat the purpose of having >> permanent breakpoints in the first place? >> >> It looks like non-gdbserver targets are not ready to support these >> tricky constructs/optimizations unfortunately. I'm afraid adding more >> hacks here and there will cause the code to get even more confusing >> without a generous amount of code comments. And i'm not even sure the >> bp_finish check is the best solution either. After all, there is the >> stepi case too. >> >> We could probably fix the simulators, but then again there are >> proprietary ones we cannot easily fix. > > For the record, i'm fine with any of those workarounds if there is no reasonable fix for the fact that permanent breakpoints don't work as GDB expects on some targets. :-) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-06-09 17:51 ` Pedro Alves 2015-06-09 18:10 ` Luis Machado @ 2015-06-16 17:39 ` Luis Machado 2015-06-17 12:41 ` Pedro Alves 1 sibling, 1 reply; 18+ messages in thread From: Luis Machado @ 2015-06-16 17:39 UTC (permalink / raw) To: Pedro Alves, gdb-patches [-- 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 ()) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-06-16 17:39 ` Luis Machado @ 2015-06-17 12:41 ` Pedro Alves 2015-06-17 13:26 ` Luis Machado 0 siblings, 1 reply; 18+ messages in thread From: Pedro Alves @ 2015-06-17 12:41 UTC (permalink / raw) To: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-06-17 12:41 ` Pedro Alves @ 2015-06-17 13:26 ` Luis Machado 2015-06-17 13:43 ` Pedro Alves 0 siblings, 1 reply; 18+ messages in thread From: Luis Machado @ 2015-06-17 13:26 UTC (permalink / raw) To: Pedro Alves, gdb-patches [-- Attachment #1: Type: text/plain, Size: 3647 bytes --] 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! [-- Attachment #2: bp_permanent_v2.diff --] [-- Type: text/x-patch, Size: 4863 bytes --] 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. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 657c58e..d731f1f 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -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 ()) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-06-17 13:26 ` Luis Machado @ 2015-06-17 13:43 ` Pedro Alves 2015-06-17 20:16 ` Luis Machado 0 siblings, 1 reply; 18+ messages in thread From: Pedro Alves @ 2015-06-17 13:43 UTC (permalink / raw) To: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-06-17 13:43 ` Pedro Alves @ 2015-06-17 20:16 ` Luis Machado 2015-07-06 15:06 ` Pedro Alves 0 siblings, 1 reply; 18+ messages in thread From: Luis Machado @ 2015-06-17 20:16 UTC (permalink / raw) To: Pedro Alves, gdb-patches 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-06-17 20:16 ` Luis Machado @ 2015-07-06 15:06 ` Pedro Alves 2015-07-06 15:33 ` Luis Machado 0 siblings, 1 reply; 18+ messages in thread From: Pedro Alves @ 2015-07-06 15:06 UTC (permalink / raw) To: Luis Machado, gdb-patches 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-07-06 15:06 ` Pedro Alves @ 2015-07-06 15:33 ` Luis Machado 2015-07-06 16:15 ` Pedro Alves 0 siblings, 1 reply; 18+ messages in thread From: Luis Machado @ 2015-07-06 15:33 UTC (permalink / raw) To: Pedro Alves, gdb-patches 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-07-06 15:33 ` Luis Machado @ 2015-07-06 16:15 ` Pedro Alves 2015-07-06 16:18 ` Luis Machado 2015-07-06 18:34 ` Luis Machado 0 siblings, 2 replies; 18+ messages in thread From: Pedro Alves @ 2015-07-06 16:15 UTC (permalink / raw) To: Luis Machado, gdb-patches On 07/06/2015 04:33 PM, Luis Machado wrote: > I'll take a look at it. I suppose this will block the branching? I think so, or at least the release. Broken infcalls seems pretty nasty. > Then again, simply reverting this will still have bad results with some > simulators. True. Might be the fix is simple though. I'm seeing this: (gdb) PASS: gdb.base/shlib-call.exp: step out of shr2 to main (stopped in shr2 epilogue) step main () at /home/pedro/gdb/mygit/build/../src/gdb/testsuite/gdb.base/shmain.c:37 37 g = mainshr1(g); (gdb) PASS: gdb.base/shlib-call.exp: step out of shr2 epilogue to main print mainshr1(1) Program received signal SIGSEGV, Segmentation fault. mainshr1 (g=1) at /home/pedro/gdb/mygit/build/../src/gdb/testsuite/gdb.base/shmain.c:29 29 } The program being debugged was signaled while in a function called from GDB. GDB remains in the frame where the signal was received. To change this behavior use "set unwindonsignal on". Evaluation of the expression containing the function (mainshr1) will be abandoned. When the function is done executing, GDB will silently stop. (gdb) FAIL: gdb.base/shlib-call.exp: print mainshr1(1) step The SIGSEGV look scary until one remembers that the dummy breakpoints are placed on the stack, which is non-executable. gdb translates those SIGSEGVs back to SIGTRAPs, provided it knows there's a breakpoint at that address. Looking a bit at breakpoint.c, I notice that a few ->permanent checks seem to have been left behind, and as result we don't actually remove from the target the breakpoints that were placed on top of the permanent breakpoints? This seems to fix the FAILs here, but I didn't run full regression testing. Could you take this, test it on qemu, and and finish it off? --- From 9cbd03b61441072c71d2076c1deb6766fecf25d2 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Mon, 6 Jul 2015 17:04:05 +0100 Subject: [PATCH] remove left behind permanent breakpoint special casing --- gdb/breakpoint.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 1481112..af0d167 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -3892,10 +3892,6 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is) /* BL is never in moribund_locations by our callers. */ gdb_assert (bl->owner != NULL); - if (bl->permanent) - /* Permanent breakpoints cannot be inserted or removed. */ - return 0; - /* The type of none suggests that owner is actually deleted. This should not ever happen. */ gdb_assert (bl->owner->type != bp_none); @@ -4042,10 +4038,6 @@ remove_breakpoint (struct bp_location *bl, insertion_state_t is) /* BL is never in moribund_locations by our callers. */ gdb_assert (bl->owner != NULL); - if (bl->permanent) - /* Permanent breakpoints cannot be inserted or removed. */ - return 0; - /* The type of none suggests that owner is actually deleted. This should not ever happen. */ gdb_assert (bl->owner->type != bp_none); @@ -4068,8 +4060,7 @@ mark_breakpoints_out (void) struct bp_location *bl, **blp_tmp; ALL_BP_LOCATIONS (bl, blp_tmp) - if (bl->pspace == current_program_space - && !bl->permanent) + if (bl->pspace == current_program_space) bl->inserted = 0; } -- 1.9.3 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-07-06 16:15 ` Pedro Alves @ 2015-07-06 16:18 ` Luis Machado 2015-07-06 18:34 ` Luis Machado 1 sibling, 0 replies; 18+ messages in thread From: Luis Machado @ 2015-07-06 16:18 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 07/06/2015 01:15 PM, Pedro Alves wrote: > On 07/06/2015 04:33 PM, Luis Machado wrote: > >> I'll take a look at it. I suppose this will block the branching? > > I think so, or at least the release. Broken infcalls seems > pretty nasty. > Indeed. >> Then again, simply reverting this will still have bad results with some >> simulators. > > True. Might be the fix is simple though. I'm seeing this: > > (gdb) PASS: gdb.base/shlib-call.exp: step out of shr2 to main (stopped in shr2 epilogue) > step > main () at /home/pedro/gdb/mygit/build/../src/gdb/testsuite/gdb.base/shmain.c:37 > 37 g = mainshr1(g); > (gdb) PASS: gdb.base/shlib-call.exp: step out of shr2 epilogue to main > print mainshr1(1) > > Program received signal SIGSEGV, Segmentation fault. > mainshr1 (g=1) at /home/pedro/gdb/mygit/build/../src/gdb/testsuite/gdb.base/shmain.c:29 > 29 } > The program being debugged was signaled while in a function called from GDB. > GDB remains in the frame where the signal was received. > To change this behavior use "set unwindonsignal on". > Evaluation of the expression containing the function > (mainshr1) will be abandoned. > When the function is done executing, GDB will silently stop. > (gdb) FAIL: gdb.base/shlib-call.exp: print mainshr1(1) > step > That's what i see too. > The SIGSEGV look scary until one remembers that the dummy breakpoints > are placed on the stack, which is non-executable. gdb translates those > SIGSEGVs back to SIGTRAPs, provided it knows there's a breakpoint at that > address. > > Looking a bit at breakpoint.c, I notice that a few ->permanent > checks seem to have been left behind, and as result we don't actually > remove from the target the breakpoints that were placed on top of the > permanent breakpoints? > > This seems to fix the FAILs here, but I didn't run full regression > testing. Could you take this, test it on qemu, and and finish it off? > Yes, of course. Thanks for having a go at it. Luis ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-07-06 16:15 ` Pedro Alves 2015-07-06 16:18 ` Luis Machado @ 2015-07-06 18:34 ` Luis Machado 2015-07-06 19:07 ` Pedro Alves 1 sibling, 1 reply; 18+ messages in thread From: Luis Machado @ 2015-07-06 18:34 UTC (permalink / raw) To: Pedro Alves, gdb-patches [-- Attachment #1: Type: text/plain, Size: 3773 bytes --] On 07/06/2015 01:15 PM, Pedro Alves wrote: > On 07/06/2015 04:33 PM, Luis Machado wrote: > >> I'll take a look at it. I suppose this will block the branching? > > I think so, or at least the release. Broken infcalls seems > pretty nasty. > >> Then again, simply reverting this will still have bad results with some >> simulators. > > True. Might be the fix is simple though. I'm seeing this: > > (gdb) PASS: gdb.base/shlib-call.exp: step out of shr2 to main (stopped in shr2 epilogue) > step > main () at /home/pedro/gdb/mygit/build/../src/gdb/testsuite/gdb.base/shmain.c:37 > 37 g = mainshr1(g); > (gdb) PASS: gdb.base/shlib-call.exp: step out of shr2 epilogue to main > print mainshr1(1) > > Program received signal SIGSEGV, Segmentation fault. > mainshr1 (g=1) at /home/pedro/gdb/mygit/build/../src/gdb/testsuite/gdb.base/shmain.c:29 > 29 } > The program being debugged was signaled while in a function called from GDB. > GDB remains in the frame where the signal was received. > To change this behavior use "set unwindonsignal on". > Evaluation of the expression containing the function > (mainshr1) will be abandoned. > When the function is done executing, GDB will silently stop. > (gdb) FAIL: gdb.base/shlib-call.exp: print mainshr1(1) > step > > The SIGSEGV look scary until one remembers that the dummy breakpoints > are placed on the stack, which is non-executable. gdb translates those > SIGSEGVs back to SIGTRAPs, provided it knows there's a breakpoint at that > address. > > Looking a bit at breakpoint.c, I notice that a few ->permanent > checks seem to have been left behind, and as result we don't actually > remove from the target the breakpoints that were placed on top of the > permanent breakpoints? > > This seems to fix the FAILs here, but I didn't run full regression > testing. Could you take this, test it on qemu, and and finish it off? > > --- > From 9cbd03b61441072c71d2076c1deb6766fecf25d2 Mon Sep 17 00:00:00 2001 > From: Pedro Alves <palves@redhat.com> > Date: Mon, 6 Jul 2015 17:04:05 +0100 > Subject: [PATCH] remove left behind permanent breakpoint special casing > > --- > gdb/breakpoint.c | 11 +---------- > 1 file changed, 1 insertion(+), 10 deletions(-) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 1481112..af0d167 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -3892,10 +3892,6 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is) > /* BL is never in moribund_locations by our callers. */ > gdb_assert (bl->owner != NULL); > > - if (bl->permanent) > - /* Permanent breakpoints cannot be inserted or removed. */ > - return 0; > - > /* The type of none suggests that owner is actually deleted. > This should not ever happen. */ > gdb_assert (bl->owner->type != bp_none); > @@ -4042,10 +4038,6 @@ remove_breakpoint (struct bp_location *bl, insertion_state_t is) > /* BL is never in moribund_locations by our callers. */ > gdb_assert (bl->owner != NULL); > > - if (bl->permanent) > - /* Permanent breakpoints cannot be inserted or removed. */ > - return 0; > - > /* The type of none suggests that owner is actually deleted. > This should not ever happen. */ > gdb_assert (bl->owner->type != bp_none); > @@ -4068,8 +4060,7 @@ mark_breakpoints_out (void) > struct bp_location *bl, **blp_tmp; > > ALL_BP_LOCATIONS (bl, blp_tmp) > - if (bl->pspace == current_program_space > - && !bl->permanent) > + if (bl->pspace == current_program_space) > bl->inserted = 0; > } > > I've confirmed it fixes the gdbserver-specific issue here and it still works correctly for other setups like native GDB and simulator-based debugging. I'll go ahead and push the following in, if it is ok. [-- Attachment #2: 0001-Fix-problems-with-finishing-a-dummy-function-call-on.patch --] [-- Type: text/x-patch, Size: 3862 bytes --] From c1bc516aae7a2293b4119f92432a73021865f538 Mon Sep 17 00:00:00 2001 From: Luis Machado <lgustavo@codesourcery.com> Date: Mon, 6 Jul 2015 15:24:37 -0300 Subject: [PATCH] Fix problems with finishing a dummy function call on simulators. This fixes regressions introduced with the original change to not consider permanent breakpoints always inserted: 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. Some checks were mistakenly left out of the original patch, which caused the following failures: -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.cp/chained-calls.exp: q(p()) +FAIL: gdb.cp/chained-calls.exp: q(p()) -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(g(f() + f())) + f()) +FAIL: gdb.cp/chained-calls.exp: g(f(g(f() + f())) + f()) -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: calling method for small class +FAIL: gdb.cp/classes.exp: calling method for small class The above is likely caused by GDB not removing the permanent breakpoints from the target, leading to the inferior executing the breakpoint instruction and tripping on a SIGSEGV. 2015-07-06 Luis Machado <lgustavo@codesourcery.com> * breakpoint.c (remove_breakpoint_1): Don't handle permanent breakpoints in a special way. (remove_breakpoint): Likewise. (mark_breakpoints_out): Likewise. --- gdb/ChangeLog | 7 +++++++ gdb/breakpoint.c | 11 +---------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 4636653..5688b4a 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,10 @@ +2015-07-06 Luis Machado <lgustavo@codesourcery.com> + + * breakpoint.c (remove_breakpoint_1): Don't handle permanent + breakpoints in a special way. + (remove_breakpoint): Likewise. + (mark_breakpoints_out): Likewise. + 2015-07-06 Andrew Burgess <andrew.burgess@embecosm.com> * tui/tui-data.c (tui_partial_win_by_name): Window name is const. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 1481112..af0d167 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -3892,10 +3892,6 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is) /* BL is never in moribund_locations by our callers. */ gdb_assert (bl->owner != NULL); - if (bl->permanent) - /* Permanent breakpoints cannot be inserted or removed. */ - return 0; - /* The type of none suggests that owner is actually deleted. This should not ever happen. */ gdb_assert (bl->owner->type != bp_none); @@ -4042,10 +4038,6 @@ remove_breakpoint (struct bp_location *bl, insertion_state_t is) /* BL is never in moribund_locations by our callers. */ gdb_assert (bl->owner != NULL); - if (bl->permanent) - /* Permanent breakpoints cannot be inserted or removed. */ - return 0; - /* The type of none suggests that owner is actually deleted. This should not ever happen. */ gdb_assert (bl->owner->type != bp_none); @@ -4068,8 +4060,7 @@ mark_breakpoints_out (void) struct bp_location *bl, **blp_tmp; ALL_BP_LOCATIONS (bl, blp_tmp) - if (bl->pspace == current_program_space - && !bl->permanent) + if (bl->pspace == current_program_space) bl->inserted = 0; } -- 1.9.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-07-06 18:34 ` Luis Machado @ 2015-07-06 19:07 ` Pedro Alves 2015-07-06 19:11 ` Luis Machado 0 siblings, 1 reply; 18+ messages in thread From: Pedro Alves @ 2015-07-06 19:07 UTC (permalink / raw) To: Luis Machado, gdb-patches On 07/06/2015 07:33 PM, Luis Machado wrote: > I've confirmed it fixes the gdbserver-specific issue here and it still > works correctly for other setups like native GDB and simulator-based > debugging. Great. > I'll go ahead and push the following in, if it is ok. Yes, this is OK. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Fix problems with finishing a dummy function call on simulators. 2015-07-06 19:07 ` Pedro Alves @ 2015-07-06 19:11 ` Luis Machado 0 siblings, 0 replies; 18+ messages in thread From: Luis Machado @ 2015-07-06 19:11 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 07/06/2015 04:07 PM, Pedro Alves wrote: > On 07/06/2015 07:33 PM, Luis Machado wrote: > >> I've confirmed it fixes the gdbserver-specific issue here and it still >> works correctly for other setups like native GDB and simulator-based >> debugging. > > Great. > >> I'll go ahead and push the following in, if it is ok. > > Yes, this is OK. I've pushed this now. Sorry for the breakage. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2015-07-06 19:11 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-06-09 15:01 [PATCH] Fix problems with finishing a dummy function call on simulators Luis Machado 2015-06-09 17:51 ` Pedro Alves 2015-06-09 18:10 ` Luis Machado 2015-06-09 18:13 ` Pedro Alves 2015-06-09 18:22 ` Luis Machado 2015-06-09 18:34 ` Luis Machado 2015-06-16 17:39 ` Luis Machado 2015-06-17 12:41 ` Pedro Alves 2015-06-17 13:26 ` Luis Machado 2015-06-17 13:43 ` Pedro Alves 2015-06-17 20:16 ` Luis Machado 2015-07-06 15:06 ` Pedro Alves 2015-07-06 15:33 ` Luis Machado 2015-07-06 16:15 ` Pedro Alves 2015-07-06 16:18 ` Luis Machado 2015-07-06 18:34 ` Luis Machado 2015-07-06 19:07 ` Pedro Alves 2015-07-06 19:11 ` Luis Machado
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).