From patchwork Wed Nov 12 10:55:14 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 3672 Received: (qmail 1595 invoked by alias); 12 Nov 2014 10:55:22 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 1582 invoked by uid 89); 12 Nov 2014 10:55:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 12 Nov 2014 10:55:19 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sACAtGB9002305 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 12 Nov 2014 05:55:16 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id sACAtFj9031148; Wed, 12 Nov 2014 05:55:15 -0500 Message-ID: <54633C92.1080304@redhat.com> Date: Wed, 12 Nov 2014 10:55:14 +0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: Yao Qi CC: gdb-patches@sourceware.org Subject: Re: [PATCH 2/3] make "permanent breakpoints" per location and disableable References: <1415127790-15091-1-git-send-email-palves@redhat.com> <1415127790-15091-3-git-send-email-palves@redhat.com> <8761eu8863.fsf@codesourcery.com> In-Reply-To: <8761eu8863.fsf@codesourcery.com> On 11/05/2014 06:37 AM, Yao Qi wrote: > Pedro Alves writes: > > Hi Pedro, > I go through this patch, and don't see anything wrong except this below, > >> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h >> index 118a37f..c383cd4 100644 >> --- a/gdb/breakpoint.h >> +++ b/gdb/breakpoint.h >> @@ -193,12 +193,6 @@ enum enable_state >> automatically enabled and reset when the >> call "lands" (either completes, or stops >> at another eventpoint). */ >> - bp_permanent /* There is a breakpoint instruction >> - hard-wired into the target's code. Don't >> - try to write another breakpoint >> - instruction on top of it, or restore its >> - value. Step over it using the >> - architecture's SKIP_INSN macro. */ >> }; > > This causes a compilation error in guile/scm-breakpoint.c where > bp_permanent is in use. > > ../../../git/gdb/guile/scm-breakpoint.c: In function ‘bpscm_enable_state_to_string’: > ../../../git/gdb/guile/scm-breakpoint.c:154:10: error: ‘bp_permanent’ undeclared (first use in this function) > case bp_permanent: return "permanent"; > > static const char * > bpscm_enable_state_to_string (enum enable_state enable_state) > { > switch (enable_state) > { > case bp_disabled: return "disabled"; > case bp_enabled: return "enabled"; > case bp_call_disabled: return "call_disabled"; > case bp_permanent: return "permanent"; > ^^^^^^^^^^^^ > default: return "unknown"; > } > } Thanks, I hadn't noticed I didn't have guile-devel installed on this machine. Now fixed and pushed, as below. --- From 1a853c5224e2b8fedfac6d029365522b83080b40 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 12 Nov 2014 10:10:49 +0000 Subject: [PATCH 2/3] make "permanent breakpoints" per location and disableable "permanent"-ness is currently a property of the breakpoint. But, it should actually be an implementation detail of a _location_. Consider this bit in infrun.c: /* Normally, by the time we reach `resume', the breakpoints are either removed or inserted, as appropriate. The exception is if we're sitting at a permanent breakpoint; we need to step over it, but permanent breakpoints can't be removed. So we have to test for it here. */ if (breakpoint_here_p (aspace, pc) == permanent_breakpoint_here) { if (gdbarch_skip_permanent_breakpoint_p (gdbarch)) gdbarch_skip_permanent_breakpoint (gdbarch, regcache); else error (_("\ The program is stopped at a permanent breakpoint, but GDB does not know\n\ how to step past a permanent breakpoint on this architecture. Try using\n\ a command like `return' or `jump' to continue execution.")); } This will wrongly skip a non-breakpoint instruction if we have a multiple location breakpoint where the whole breakpoint was set to "permanent" because one of the locations happened to be permanent, even if the one GDB is resuming from is not. Related, because the permanent breakpoints are only marked as such in init_breakpoint_sal, we currently miss marking momentary breakpoints as permanent. A test added by a following patch trips on that. Making permanent-ness be per-location, and marking locations as such in add_location_to_breakpoint, the natural place to do this, fixes this issue... ... and then exposes a latent issue with mark_breakpoints_out. It's clearing the inserted flag of permanent breakpoints. This results in assertions failing like this: Breakpoint 1, main () at testsuite/gdb.base/callexit.c:32 32 return 0; (gdb) call callexit() [Inferior 1 (process 15849) exited normally] gdb/breakpoint.c:12854: internal-error: allegedly permanent breakpoint is not actually inserted A problem internal to GDB has been detected, further debugging may prove unreliable. The call dummy breakpoint, which is a momentary breakpoint, is set on top of a manually inserted breakpoint instruction, and so is now rightfully marked as a permanent breakpoint. See "Write a legitimate instruction at the point where the infcall breakpoint is going to be inserted." comment in infcall.c. Re. make_breakpoint_permanent. That's only called by solib-pa64.c. Permanent breakpoints were actually originally invented for HP-UX [1]. I believe that that call (the only one in the tree) is unnecessary nowadays, given that nowadays the core breakpoints code analyzes the instruction under the breakpoint to automatically detect whether it's setting a breakpoint on top of a breakpoint instruction in the program. I know close to nothing about HP-PA/HP-UX, though. [1] https://sourceware.org/ml/gdb-patches/1999-q3/msg00245.html, and https://sourceware.org/ml/gdb-patches/1999-q3/msg00242.html In addition to the per-location issue, "permanent breakpoints" are currently always displayed as enabled=='n': (gdb) b main Breakpoint 3 at 0x40053c: file ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S, line 29. (gdb) info breakpoints Num Type Disp Enb Address What 3 breakpoint keep n 0x000000000040053c ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S:29 But OTOH they're always enabled; there's no way to disable them... In turn, this means that if one adds commands to such a breakpoint, they're _always_ run: (gdb) start Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.arch/i386-permbkpt ... Temporary breakpoint 1, main () at ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S:29 29 int3 (gdb) b main Breakpoint 2 at 0x40053c: file ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S, line 29. (gdb) info breakpoints Num Type Disp Enb Address What 2 breakpoint keep n 0x000000000040053c ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S:29 (gdb) commands Type commands for breakpoint(s) 2, one per line. End with a line saying just "end". >echo "hello!" >end (gdb) disable 2 (gdb) start The program being debugged has been started already. Start it from the beginning? (y or n) y Temporary breakpoint 3 at 0x40053c: file ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S, line 29. Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.arch/i386-permbkpt Breakpoint 2, main () at ../../../src/gdb/testsuite/gdb.arch/i386-permbkpt.S:29 29 int3 "hello!"(gdb) IMO, one should be able to disable such a breakpoint, and GDB should then behave just like if the user hadn't created the breakpoint in the first place (that is, report a SIGTRAP). By making permanent-ness a property of the location, and eliminating the bp_permanent enum enable_state state ends up fixing that as well. No tests are added for these changes yet; they'll be added in a follow up patch, as skipping permanent breakpoints is currently broken and trips on an assertion in infrun. Tested on x86_64 Fedora 20, native and gdbserver. gdb/ChangeLog: 2014-11-12 Pedro Alves Mark locations as permanent, not the whole breakpoint. * breakpoint.c (remove_breakpoint_1, remove_breakpoint): Adjust. (mark_breakpoints_out): Don't mark permanent breakpoints as uninserted. (breakpoint_init_inferior): Use mark_breakpoints_out. (breakpoint_here_p): Adjust. (bpstat_stop_status, describe_other_breakpoints): Remove handling of permanent breakpoints. (make_breakpoint_permanent): Mark each location as permanent, instead of marking the breakpoint. (add_location_to_breakpoint): If the location is permanent, mark it as such, and as inserted. (init_breakpoint_sal): Don't make the breakpoint permanent here. (bp_location_compare, update_global_location_list): Adjust. (update_breakpoint_locations): Don't make the breakpoint permanent here. (disable_breakpoint, enable_breakpoint_disp): Don't skip permanent breakpoints. * breakpoint.h (enum enable_state) : Delete field. (struct bp_location) : New field. * guile/scm-breakpoint.c (bpscm_enable_state_to_string): Remove reference to bp_permanent. --- gdb/ChangeLog | 25 ++++++++++++++++ gdb/breakpoint.c | 71 ++++++++++++++++++---------------------------- gdb/breakpoint.h | 13 +++++---- gdb/guile/scm-breakpoint.c | 1 - 4 files changed, 60 insertions(+), 50 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 74db317..a50f67e 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,30 @@ 2014-11-12 Pedro Alves + Mark locations as permanent, not the whole breakpoint. + * breakpoint.c (remove_breakpoint_1, remove_breakpoint): Adjust. + (mark_breakpoints_out): Don't mark permanent breakpoints as + uninserted. + (breakpoint_init_inferior): Use mark_breakpoints_out. + (breakpoint_here_p): Adjust. + (bpstat_stop_status, describe_other_breakpoints): Remove handling + of permanent breakpoints. + (make_breakpoint_permanent): Mark each location as permanent, + instead of marking the breakpoint. + (add_location_to_breakpoint): If the location is permanent, mark + it as such, and as inserted. + (init_breakpoint_sal): Don't make the breakpoint permanent here. + (bp_location_compare, update_global_location_list): Adjust. + (update_breakpoint_locations): Don't make the breakpoint permanent + here. + (disable_breakpoint, enable_breakpoint_disp): Don't skip permanent + breakpoints. + * breakpoint.h (enum enable_state) : Delete field. + (struct bp_location) : New field. + * guile/scm-breakpoint.c (bpscm_enable_state_to_string): Remove + reference to bp_permanent. + +2014-11-12 Pedro Alves + * arch-utils.c (default_skip_permanent_breakpoint): New function. * arch-utils.h (default_skip_permanent_breakpoint): New declaration. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index bd51f5d..3ebe9c9 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -3883,7 +3883,7 @@ 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->owner->enable_state == bp_permanent) + if (bl->permanent) /* Permanent breakpoints cannot be inserted or removed. */ return 0; @@ -4033,7 +4033,7 @@ 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->owner->enable_state == bp_permanent) + if (bl->permanent) /* Permanent breakpoints cannot be inserted or removed. */ return 0; @@ -4059,7 +4059,8 @@ mark_breakpoints_out (void) struct bp_location *bl, **blp_tmp; ALL_BP_LOCATIONS (bl, blp_tmp) - if (bl->pspace == current_program_space) + if (bl->pspace == current_program_space + && !bl->permanent) bl->inserted = 0; } @@ -4088,13 +4089,7 @@ breakpoint_init_inferior (enum inf_context context) if (gdbarch_has_global_breakpoints (target_gdbarch ())) return; - ALL_BP_LOCATIONS (bl, blp_tmp) - { - /* ALL_BP_LOCATIONS bp_location has BL->OWNER always non-NULL. */ - if (bl->pspace == pspace - && bl->owner->enable_state != bp_permanent) - bl->inserted = 0; - } + mark_breakpoints_out (); ALL_BREAKPOINTS_SAFE (b, b_tmp) { @@ -4202,14 +4197,14 @@ breakpoint_here_p (struct address_space *aspace, CORE_ADDR pc) /* ALL_BP_LOCATIONS bp_location has BL->OWNER always non-NULL. */ if ((breakpoint_enabled (bl->owner) - || bl->owner->enable_state == bp_permanent) + || bl->permanent) && breakpoint_location_address_match (bl, aspace, pc)) { if (overlay_debugging && section_is_overlay (bl->section) && !section_is_mapped (bl->section)) continue; /* unmapped overlay -- can't be a match */ - else if (bl->owner->enable_state == bp_permanent) + else if (bl->permanent) return permanent_breakpoint_here; else any_breakpoint_here = 1; @@ -5475,7 +5470,7 @@ bpstat_stop_status (struct address_space *aspace, ALL_BREAKPOINTS (b) { - if (!breakpoint_enabled (b) && b->enable_state != bp_permanent) + if (!breakpoint_enabled (b)) continue; for (bl = b->loc; bl != NULL; bl = bl->next) @@ -5571,8 +5566,7 @@ bpstat_stop_status (struct address_space *aspace, if (b->disposition == disp_disable) { --(b->enable_count); - if (b->enable_count <= 0 - && b->enable_state != bp_permanent) + if (b->enable_count <= 0) b->enable_state = bp_disabled; removed_any = 1; } @@ -6856,8 +6850,6 @@ describe_other_breakpoints (struct gdbarch *gdbarch, ((b->enable_state == bp_disabled || b->enable_state == bp_call_disabled) ? " (disabled)" - : b->enable_state == bp_permanent - ? " (permanent)" : ""), (others > 1) ? "," : ((others == 1) ? " and" : "")); @@ -7389,15 +7381,16 @@ make_breakpoint_permanent (struct breakpoint *b) { struct bp_location *bl; - b->enable_state = bp_permanent; - /* 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->inserted = 1; + { + bl->permanent = 1; + bl->inserted = 1; + } } /* Call this routine when stepping and nexting to enable a breakpoint @@ -9227,6 +9220,8 @@ mention (struct breakpoint *b) } +static int bp_loc_is_permanent (struct bp_location *loc); + static struct bp_location * add_location_to_breakpoint (struct breakpoint *b, const struct symtab_and_line *sal) @@ -9268,6 +9263,13 @@ add_location_to_breakpoint (struct breakpoint *b, set_breakpoint_location_function (loc, sal->explicit_pc || sal->explicit_line); + + if (bp_loc_is_permanent (loc)) + { + loc->inserted = 1; + loc->permanent = 1; + } + return loc; } @@ -9510,9 +9512,6 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch, loc->inserted = 1; } - if (bp_loc_is_permanent (loc)) - make_breakpoint_permanent (b); - if (b->cond_string) { const char *arg = b->cond_string; @@ -12352,7 +12351,7 @@ breakpoint_auto_delete (bpstat bs) /* A comparison function for bp_location AP and BP being interfaced to qsort. Sort elements primarily by their ADDRESS (no matter what does breakpoint_address_is_meaningful say for its OWNER), - secondarily by ordering first bp_permanent OWNERed elements and + secondarily by ordering first permanent elements and terciarily just ensuring the array is sorted stable way despite qsort being an unstable algorithm. */ @@ -12361,9 +12360,6 @@ bp_location_compare (const void *ap, const void *bp) { struct bp_location *a = *(void **) ap; struct bp_location *b = *(void **) bp; - /* A and B come from existing breakpoints having non-NULL OWNER. */ - int a_perm = a->owner->enable_state == bp_permanent; - int b_perm = b->owner->enable_state == bp_permanent; if (a->address != b->address) return (a->address > b->address) - (a->address < b->address); @@ -12377,8 +12373,8 @@ bp_location_compare (const void *ap, const void *bp) - (a->pspace->num < b->pspace->num)); /* Sort permanent breakpoints first. */ - if (a_perm != b_perm) - return (a_perm < b_perm) - (a_perm > b_perm); + if (a->permanent != b->permanent) + return (a->permanent < b->permanent) - (a->permanent > b->permanent); /* Make the internal GDB representation stable across GDB runs where A and B memory inside GDB can differ. Breakpoint locations of @@ -12849,7 +12845,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode) } /* Permanent breakpoint should always be inserted. */ - if (b->enable_state == bp_permanent && ! loc->inserted) + if (loc->permanent && ! loc->inserted) internal_error (__FILE__, __LINE__, _("allegedly permanent breakpoint is not " "actually inserted")); @@ -12890,8 +12886,8 @@ update_global_location_list (enum ugll_insert_mode insert_mode) /* Clear the condition modification flag. */ loc->condition_changed = condition_unchanged; - if ((*loc_first_p)->owner->enable_state == bp_permanent && loc->inserted - && b->enable_state != bp_permanent) + if (loc->inserted && !loc->permanent + && (*loc_first_p)->permanent) internal_error (__FILE__, __LINE__, _("another breakpoint was inserted on top of " "a permanent breakpoint")); @@ -14437,10 +14433,6 @@ update_breakpoint_locations (struct breakpoint *b, } } - /* Update locations of permanent breakpoints. */ - if (b->enable_state == bp_permanent) - make_breakpoint_permanent (b); - /* If possible, carry over 'disable' status from existing breakpoints. */ { @@ -14923,10 +14915,6 @@ disable_breakpoint (struct breakpoint *bpt) if (bpt->type == bp_watchpoint_scope) return; - /* You can't disable permanent breakpoints. */ - if (bpt->enable_state == bp_permanent) - return; - bpt->enable_state = bp_disabled; /* Mark breakpoint locations modified. */ @@ -15047,9 +15035,6 @@ enable_breakpoint_disp (struct breakpoint *bpt, enum bpdisp disposition, } } - if (bpt->enable_state != bp_permanent) - bpt->enable_state = bp_enabled; - bpt->enable_state = bp_enabled; /* Mark breakpoint locations modified. */ diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h index 118a37f..c383cd4 100644 --- a/gdb/breakpoint.h +++ b/gdb/breakpoint.h @@ -193,12 +193,6 @@ enum enable_state automatically enabled and reset when the call "lands" (either completes, or stops at another eventpoint). */ - bp_permanent /* There is a breakpoint instruction - hard-wired into the target's code. Don't - try to write another breakpoint - instruction on top of it, or restore its - value. Step over it using the - architecture's SKIP_INSN macro. */ }; @@ -376,6 +370,13 @@ struct bp_location /* Nonzero if this breakpoint is now inserted. */ char inserted; + /* Nonzero if this is a permanent breakpoint. There is a breakpoint + instruction hard-wired into the target's code. Don't try to + write another breakpoint instruction on top of it, or restore its + value. Step over it using the architecture's + gdbarch_skip_permanent_breakpoint method. */ + char permanent; + /* Nonzero if this is not the first breakpoint in the list for the given address. location of tracepoint can _never_ be duplicated with other locations of tracepoints and other diff --git a/gdb/guile/scm-breakpoint.c b/gdb/guile/scm-breakpoint.c index aed9f9a..92b3242 100644 --- a/gdb/guile/scm-breakpoint.c +++ b/gdb/guile/scm-breakpoint.c @@ -151,7 +151,6 @@ bpscm_enable_state_to_string (enum enable_state enable_state) case bp_disabled: return "disabled"; case bp_enabled: return "enabled"; case bp_call_disabled: return "call_disabled"; - case bp_permanent: return "permanent"; default: return "unknown"; } }