Message ID | 1459759138-1494-3-git-send-email-yao.qi@linaro.org |
---|---|
State | New, archived |
Headers |
Received: (qmail 92614 invoked by alias); 4 Apr 2016 08:39:20 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 92245 invoked by uid 89); 4 Apr 2016 08:39:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=XNEW, xnew, HX-Received:10.67.4.69, Hx-languages-length:4700 X-HELO: mail-pa0-f42.google.com Received: from mail-pa0-f42.google.com (HELO mail-pa0-f42.google.com) (209.85.220.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Mon, 04 Apr 2016 08:39:10 +0000 Received: by mail-pa0-f42.google.com with SMTP id td3so139143420pab.2 for <gdb-patches@sourceware.org>; Mon, 04 Apr 2016 01:39:09 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:subject:date:message-id:in-reply-to :references; bh=+9KFk4wOoA1+NmcXkrOcgXbmXIRGcJG6XzjoFAzxk7Y=; b=HprPrR735gnQy7hg8ZCi2mwVB6R/X10s9OIhvRYnfsJTRL/slMK1dPS1n6+J4RTmJJ Jp3xXfNmXmIMJmUoeSW5MODL5oBPzJP9065SZuhEXK24qBwrxUQ/p18wAwdB7fecJVcX 1/77dJUNhBUhwhxVtFdDnTzPj2fHDuAasOlftjilPDIEgxmylJDMtIeE2kxLoM7lCZHL uLCLuJA26BeERwg7/cgLDDAZqfEntPj/kxXXx3nJclFVSeEJwS2KTh6HAxKo26CNfixB KA03OTZpPbw7LTq1+UtXj9giTvjEalKpzUAbGRNK263jL9O9ilfuNm2XXcIil54QsubU FD9g== X-Gm-Message-State: AD7BkJKyeh1decyy8Nvb+8zMBxvOdD3F7faAXqZ1bhJwroPsCRHx/3ddWIw2ZOvHpafCUg== X-Received: by 10.67.4.69 with SMTP id cc5mr51322307pad.11.1459759148341; Mon, 04 Apr 2016 01:39:08 -0700 (PDT) Received: from E107787-LIN.cambridge.arm.com (gcc1-power7.osuosl.org. [140.211.15.137]) by smtp.gmail.com with ESMTPSA id dg1sm39662470pad.18.2016.04.04.01.39.07 for <gdb-patches@sourceware.org> (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 04 Apr 2016 01:39:07 -0700 (PDT) From: Yao Qi <qiyaoltc@gmail.com> X-Google-Original-From: Yao Qi <yao.qi@linaro.org> To: gdb-patches@sourceware.org Subject: [PATCH 2/2] Make breakpoint handling in record-full idempotent Date: Mon, 4 Apr 2016 09:38:58 +0100 Message-Id: <1459759138-1494-3-git-send-email-yao.qi@linaro.org> In-Reply-To: <1459759138-1494-1-git-send-email-yao.qi@linaro.org> References: <1459759138-1494-1-git-send-email-yao.qi@linaro.org> X-IsSubscribed: yes |
Commit Message
Yao Qi
April 4, 2016, 8:38 a.m. UTC
Some test fails in gdb.reverse/break-reverse.exp on arm-linux lead me seeing the following error message, continue^M Continuing.^M Cannot remove breakpoints because program is no longer writable.^M ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Further execution is probably impossible.^M ^M Breakpoint 3, bar () at /home/yao/SourceCode/gnu/gdb/git/gdb/testsuite/gdb.reverse/break-reverse.c:22^M 22 xyz = 2; /* break in bar */^M (gdb) PASS: gdb.reverse/break-reverse.exp: continue to breakpoint: bar backward this is caused by two entries in record_full_breakpoints, and their addr is the same, but in_target_beneath is different. during the record, we do continue, Continuing. infrun: clear_proceed_status_thread (Thread 13772.13772) infrun: proceed (addr=0xffffffff, signal=GDB_SIGNAL_DEFAULT) infrun: step-over queue now empty infrun: resuming [Thread 13772.13772] for step-over infrun: skipping breakpoint: stepping past insn at: 0x8620 Sending packet: $Z0,85f4,4#1d...Packet received: OK <---- ..... Sending packet: $vCont;c#a8...infrun: target_wait (-1.0.0, status) = infrun: -1.0.0 [process -1], infrun: status->kind = ignore infrun: TARGET_WAITKIND_IGNORE infrun: prepare_to_wait infrun: target_wait (-1.0.0, status) = infrun: -1.0.0 [process -1], infrun: status->kind = ignore infrun: TARGET_WAITKIND_IGNORE infrun: prepare_to_wait Packet received: T05swbreak:;0b:9cf5ffbe;0d:9cf5ffbe;0f:f4850000;thread:p35cc.35cc;core:1; Sending packet: $Z0,85f4,4#1d...Packet received: OK <----- .... Sending packet: $z0,85f4,4#3d...Packet received: OK <----- we can see breakpoint on 0x85f4 are inserted *twice*, but only removed once. That is fine to remote target, because Z/z packets are idempotent, but there is a leftover in record_full_breakpoints in record-full target. The flow can be described as below, record_full_breakpoints remote target ----------------------------------------------------------------------- forward execution, continue, in_target_beneath 1 breakpoint inserted insert breakpoints on 0x85f4 in_target_beneath 1 twice program stops, remove breakpoint on 0x85f4 in_target_beneath 1 breakpoint removed reverse execution, continue, in_target_beneath 1 none is requested insert breakpoints on 0x85f4, in_target_beneath 0 program stops, remote breakpoint on 0x85f4, in_target_beneath 0 request to remove, but GDBserver doesn't know now, the question is why breakoint on 0x85f4 is inserted twice? One is the normal breakpoint, and the other is the single step breakpoint. GDB inserts single step breakpoint to do single step. When program stops at 0x85f4, both of them are set on 0x85f4, and GDB deletes single step breakpoint, so in update_global_location_list, this breakpoint location is no longer found, GDB call force_breakpoint_reinsertion to mark it condition_updated, and insert it again. The reason force_breakpoint_reinsertion is called to update the conditions in the target side, because the conditions may be changed. My original fix is to not call force_breakpoint_reinsertion if OLD_LOC->cond is NULL, but it is not correct if another location on the same address has condition, GDB doesn't produce condition for target side, but GDB should do. Then, I change my mind back to make record-full handling breakpoint idempotent, to align with remote target. Before insert a new entry into record_full_breakpoints, look for existing one on the same address first. I also add an assert on "bp->in_target_beneath == in_target_beneath", to be safer. gdb: 2016-04-04 Yao Qi <yao.qi@linaro.org> * record-full.c (record_full_insert_breakpoint): Return early if entry on the address is found in record_full_breakpoints. --- gdb/record-full.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
Comments
On 04/04/2016 03:38 AM, Yao Qi wrote: > Some test fails in gdb.reverse/break-reverse.exp on arm-linux lead me > seeing the following error message, > > continue^M > Continuing.^M > Cannot remove breakpoints because program is no longer writable.^M > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Further execution is probably impossible.^M > ^M > Breakpoint 3, bar () at /home/yao/SourceCode/gnu/gdb/git/gdb/testsuite/gdb.reverse/break-reverse.c:22^M > 22 xyz = 2; /* break in bar */^M > (gdb) PASS: gdb.reverse/break-reverse.exp: continue to breakpoint: bar backward > > this is caused by two entries in record_full_breakpoints, and their addr > is the same, but in_target_beneath is different. > > during the record, we do continue, > > Continuing. > infrun: clear_proceed_status_thread (Thread 13772.13772) > infrun: proceed (addr=0xffffffff, signal=GDB_SIGNAL_DEFAULT) > infrun: step-over queue now empty > infrun: resuming [Thread 13772.13772] for step-over > infrun: skipping breakpoint: stepping past insn at: 0x8620 > Sending packet: $Z0,85f4,4#1d...Packet received: OK <---- > ..... > Sending packet: $vCont;c#a8...infrun: target_wait (-1.0.0, status) = > infrun: -1.0.0 [process -1], > infrun: status->kind = ignore > infrun: TARGET_WAITKIND_IGNORE > infrun: prepare_to_wait > infrun: target_wait (-1.0.0, status) = > infrun: -1.0.0 [process -1], > infrun: status->kind = ignore > infrun: TARGET_WAITKIND_IGNORE > infrun: prepare_to_wait > Packet received: T05swbreak:;0b:9cf5ffbe;0d:9cf5ffbe;0f:f4850000;thread:p35cc.35cc;core:1; > Sending packet: $Z0,85f4,4#1d...Packet received: OK <----- > .... > Sending packet: $z0,85f4,4#3d...Packet received: OK <----- > > we can see breakpoint on 0x85f4 are inserted *twice*, but only removed > once. That is fine to remote target, because Z/z packets are > idempotent, but there is a leftover in record_full_breakpoints > in record-full target. The flow can be described as below, > > record_full_breakpoints remote target > ----------------------------------------------------------------------- > forward execution, continue, in_target_beneath 1 breakpoint inserted > insert breakpoints on 0x85f4 in_target_beneath 1 > twice > > program stops, > remove breakpoint on 0x85f4 in_target_beneath 1 breakpoint removed > > reverse execution, continue, in_target_beneath 1 none is requested > insert breakpoints on 0x85f4, in_target_beneath 0 > > program stops, > remote breakpoint on 0x85f4, in_target_beneath 0 request to remove, > but GDBserver > doesn't know > > now, the question is why breakoint on 0x85f4 is inserted twice? One > is the normal breakpoint, and the other is the single step breakpoint. > GDB inserts single step breakpoint to do single step. When program > stops at 0x85f4, both of them are set on 0x85f4, and GDB deletes > single step breakpoint, so in update_global_location_list, this > breakpoint location is no longer found, GDB call > force_breakpoint_reinsertion to mark it condition_updated, and insert > it again. > > The reason force_breakpoint_reinsertion is called to update the > conditions in the target side, because the conditions may be > changed. My original fix is to not call force_breakpoint_reinsertion > if OLD_LOC->cond is NULL, but it is not correct if another location > on the same address has condition, GDB doesn't produce condition for > target side, but GDB should do. > > Then, I change my mind back to make record-full handling breakpoint > idempotent, to align with remote target. Before insert a new entry > into record_full_breakpoints, look for existing one on the same > address first. I also add an assert on > "bp->in_target_beneath == in_target_beneath", to be safer. > > gdb: > > 2016-04-04 Yao Qi <yao.qi@linaro.org> > > * record-full.c (record_full_insert_breakpoint): Return > early if entry on the address is found in > record_full_breakpoints. > --- > gdb/record-full.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/gdb/record-full.c b/gdb/record-full.c > index 066a8e7..9e7694e 100644 > --- a/gdb/record-full.c > +++ b/gdb/record-full.c > @@ -1650,6 +1650,7 @@ record_full_insert_breakpoint (struct target_ops *ops, > { > struct record_full_breakpoint *bp; > int in_target_beneath = 0; > + int ix; > > if (!RECORD_FULL_IS_REPLAY) > { > @@ -1681,6 +1682,20 @@ record_full_insert_breakpoint (struct target_ops *ops, > bp_tgt->placed_size = bplen; > } > > + /* Find any existing entries. */ Should this say... /* Make sure there are no duplicate breakpoint entries. */ ... instead? Otherwise it looks good to me. > + for (ix = 0; > + VEC_iterate (record_full_breakpoint_p, > + record_full_breakpoints, ix, bp); > + ++ix) > + { > + if (bp->addr == bp_tgt->placed_address > + && bp->address_space == bp_tgt->placed_address_space) > + { > + gdb_assert (bp->in_target_beneath == in_target_beneath); > + return 0; > + } > + } > + > bp = XNEW (struct record_full_breakpoint); > bp->addr = bp_tgt->placed_address; > bp->address_space = bp_tgt->placed_address_space; >
Luis Machado <lgustavo@codesourcery.com> writes: Hi Luis, >> + /* Find any existing entries. */ > > Should this say... > > /* Make sure there are no duplicate breakpoint entries. */ > > ... instead? I put tow comments together, /* Use the existing entries if found in order to avoid duplication in record_full_breakpoints. */
On 04/06/2016 03:50 AM, Yao Qi wrote: > Luis Machado <lgustavo@codesourcery.com> writes: > > Hi Luis, > >>> + /* Find any existing entries. */ >> >> Should this say... >> >> /* Make sure there are no duplicate breakpoint entries. */ >> >> ... instead? > > I put tow comments together, > > /* Use the existing entries if found in order to avoid duplication > in record_full_breakpoints. */ > Thanks. That reads better.
diff --git a/gdb/record-full.c b/gdb/record-full.c index 066a8e7..9e7694e 100644 --- a/gdb/record-full.c +++ b/gdb/record-full.c @@ -1650,6 +1650,7 @@ record_full_insert_breakpoint (struct target_ops *ops, { struct record_full_breakpoint *bp; int in_target_beneath = 0; + int ix; if (!RECORD_FULL_IS_REPLAY) { @@ -1681,6 +1682,20 @@ record_full_insert_breakpoint (struct target_ops *ops, bp_tgt->placed_size = bplen; } + /* Find any existing entries. */ + for (ix = 0; + VEC_iterate (record_full_breakpoint_p, + record_full_breakpoints, ix, bp); + ++ix) + { + if (bp->addr == bp_tgt->placed_address + && bp->address_space == bp_tgt->placed_address_space) + { + gdb_assert (bp->in_target_beneath == in_target_beneath); + return 0; + } + } + bp = XNEW (struct record_full_breakpoint); bp->addr = bp_tgt->placed_address; bp->address_space = bp_tgt->placed_address_space;