From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 36021 invoked by alias); 18 Sep 2015 01:57:56 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 35321 invoked by uid 89); 18 Sep 2015 01:57:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no 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; Fri, 18 Sep 2015 01:57:54 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 6FC44A8B for ; Fri, 18 Sep 2015 01:57:53 +0000 (UTC) Received: from pinnacle.lan (ovpn-113-104.phx2.redhat.com [10.3.113.104]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t8I1vq6F001882 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA256 bits=256 verify=NO) for ; Thu, 17 Sep 2015 21:57:53 -0400 Date: Fri, 18 Sep 2015 01:57:00 -0000 From: Kevin Buettner To: gdb-patches@sourceware.org Subject: Re: [PATCH 3/8] Break at each iteration for breakpoints placed on a while statement Message-ID: <20150917185751.22fe2937@pinnacle.lan> In-Reply-To: <55DC5B13.6030501@redhat.com> References: <20150818235334.1afb0c85@pinnacle.lan> <20150819000334.62f7a867@pinnacle.lan> <55DC5B13.6030501@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-09/txt/msg00433.txt.bz2 On Tue, 25 Aug 2015 13:09:55 +0100 Pedro Alves wrote: > On 08/19/2015 08:03 AM, Kevin Buettner wrote: > > > @@ -1933,6 +1956,18 @@ create_sals_line_offset (struct linespec_state *self, > > struct symbol *sym = (blocks[i] > > ? block_containing_function (blocks[i]) > > : NULL); > > + CORE_ADDR branch_addr = gdbarch_unconditional_branch_address > > + (get_current_arch (), intermediate_results.sals[i].pc); > > It'd be nice to have a comment here explaining why we do this. You have > it in the commit log, but it'd be useful going forward to have it in > the sources. > > Nit, this is about the branch _destination_ not the branch instruction's > address, right? I'd suggest > s/gdbarch_unconditional_branch_address/gdbarch_unconditional_branch_destination/ > > Also, there should be a better gdbarch to use than get_current_arch(). > E.g., get_objfile_arch (SYMTAB_OBJFILE (sals[i].symtab)), the sal's pspace's > gdbarch, etc. > > > + > > + /* Only use branch if it's in the same block and is also > > And here "use branch destination". > > > + within one of the sals from the initial list. */ > > + if (branch_addr != 0 && blocks[i]->startaddr <= branch_addr > > + && branch_addr < blocks[i]->endaddr > > + && addr_in_sals (branch_addr, intermediate_results.nelts, > > + intermediate_results.sals)) > > + { > > + intermediate_results.sals[i].pc = branch_addr; > > + } > > Also, we really shouldn't be adding code that assumes 0 to mean invalid > address, as on some ports, it is a valid address. You could e.g., > change gdbarch_unconditional_branch_address to return an int and > take an output CORE_ADDR * parameter, and/or make it an M gdbarch > method, and check gdbarch_unconditional_branch_address_p before > use. I've fixed each of these in the new patch. See below. > I wonder whether we have to worry about the case of a goto > jumping to the same line? Like: > > goto label; foo (); label: bar (); > > Not the most usual or beautiful code to write, though I'd guess code > generators could output things like that. Writing it in several lines > but behind a #define might be another way to get everything in > the same line. Here is what I tried: volatile int v; ... v = 0; goto d; c: v++; d: if (v < 3) goto c; /* Loop 5 */ Here's what gcc generates for x86_64 (on my machine, anyway): => 0x4005c2 : movl $0x0,0x200a64(%rip) # 0x601030 0x4005cc : nop 0x4005cd : mov 0x200a5d(%rip),%eax # 0x601030 0x4005d3 : cmp $0x2,%eax 0x4005d6 : jg 0x4005ea 0x4005d8 : nop 0x4005d9 : mov 0x200a51(%rip),%eax # 0x601030 0x4005df : add $0x1,%eax 0x4005e2 : mov %eax,0x200a48(%rip) # 0x601030 0x4005e8 : jmp 0x4005cd If I place a breakpoint on the Loop 5 line, it ends up on on 0x4005cc, which is a nop. It appears that gcc has rewritten this code so that the the comparison comes first, which exactly the opposite of what happens when one writes a while loop. The presence of the nop means that things work as expected when we place a breakpoint on that line. It will stop just once on the statement instead of once for each time through the loop. This was an interesting exercise, but it doesn't really answer the question of what would happen if gcc would instead place an actual branch at the beginning. I would expect my patch to cause the breakpoint to be placed within the loop instead of at the beginning of the statement. I don't think this is desirable, but I can't think of a way (within the current work) to stop it from happening. Here's the new patch: Break at each iteration for breakpoints placed on a while statement. This patch changes create_sals_line_offset() in linespec.c so that, for a given SAL, if that SAL's address (pc) refers to an unconditional branch instruction whose branch target also refers to the same SAL, then the branch target is used for the SAL instead. The pratical effect of this is that a breakpoint placed on a while loop will break at the evaluation of the condition instead of at the unconditional branch which transfers control to the starting address for the evaluation of the condition. Consider the following code snippet (which is taken from one of the new tests for this patch set): 9 while (v < 3) /* Loop 1 condition */ 10 { 11 v++; /* Loop 1 increment */ 12 } This is compiled as the following x86_64 code: 0x000000000040059e : jmp 0x4005af 0x00000000004005a0 : mov 0x200a8a(%rip),%eax # 0x601030 0x00000000004005a6 : add $0x1,%eax 0x00000000004005a9 : mov %eax,0x200a81(%rip) # 0x601030 0x00000000004005af : mov 0x200a7b(%rip),%eax # 0x601030 0x00000000004005b5 : cmp $0x2,%eax 0x00000000004005b8 : jle 0x4005a0 If a breakpoint is placed on line 9, which begins at loop_test+14, this change/patch causes the breakpoint to be placed on loop_test+31, which is the starting address for the evaluation of the condition. In order for this to work, an architecture specific method, unconditional_branch_destination, was introduced in an earlier patch in the set. I've implemented this method for x86_64, i386, arm, thumb, powerpc, rx, and rl78. I've tested on each of these architectures and see no regressions. gdb/ChangeLog: * linespec.c (addr_in_sals): New function. (create_sals_line_offset): Adjust SAL whose pc refers to an unconditional branch whose destination is the same line. --- gdb/linespec.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/gdb/linespec.c b/gdb/linespec.c index 4c29c12..4b5cde9 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -1808,6 +1808,29 @@ canonicalize_linespec (struct linespec_state *state, const linespec_p ls) } } +/* Return 1 if one of the SALS between 0 and NELTS contains ADDR. + Return 0 otherwise. */ + +static int +addr_in_sals (CORE_ADDR addr, int nelts, struct symtab_and_line *sals) +{ + int i; + + for (i = 0; i < nelts; i++) + { + struct symtab_and_line sal; + + if (sals[i].end == 0) + sal = find_pc_sect_line (sals[i].pc, sals[i].section, 0); + else + sal = sals[i]; + + if (sal.pc <= addr && addr < sal.end) + return 1; + } + return 0; +} + /* Given a line offset in LS, construct the relevant SALs. */ static struct symtabs_and_lines @@ -1933,6 +1956,59 @@ create_sals_line_offset (struct linespec_state *self, struct symbol *sym = (blocks[i] ? block_containing_function (blocks[i]) : NULL); + int is_branch; + CORE_ADDR branch_dest; + + /* This next bit of code examines the instruction at the + SAL's address (pc). If the instruction is an + unconditional branch whose branch destination also + refers to the same SAL, then the branch destination is + used for the SAL's pc value instead. + + The pratical effect of this is that a breakpoint placed + on a while loop will break at the evaluation of the + condition instead of at an unconditional branch which + transfers control to the starting address for the + evaluation of the condition. + + Consider the following code snippet (which is taken + from the test case for gdb.base/loop-break.exp.) + + while (v < 3) + { + v++; + } + + On x86_64, this might be compiled as follows: + + 0x40059e : jmp 0x4005af + 0x4005a0 : mov 0x200a8a(%rip),%eax + 0x4005a6 : add $0x1,%eax + 0x4005a9 : mov %eax,0x200a81(%rip) + 0x4005af : mov 0x200a7b(%rip),%eax + 0x4005b5 : cmp $0x2,%eax + 0x4005b8 : jle 0x4005a0 + + If a breakpoint is placed on the while statement line + which begins at loop_test+14, the following code causes + the breakpoint to be (instead) placed on loop_test+31, which + is the starting address for the evaluation of the condition. */ + + is_branch = gdbarch_unconditional_branch_destination + (get_objfile_arch + (SYMTAB_OBJFILE (intermediate_results.sals[i].symtab)), + intermediate_results.sals[i].pc, + &branch_dest); + + /* Only use branch destination if it's in the same block and + is also within one of the sals from the initial list. */ + if (is_branch && blocks[i]->startaddr <= branch_dest + && branch_dest < blocks[i]->endaddr + && addr_in_sals (branch_dest, intermediate_results.nelts, + intermediate_results.sals)) + { + intermediate_results.sals[i].pc = branch_dest; + } if (self->funfirstline) skip_prologue_sal (&intermediate_results.sals[i]);