From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id xNbtOT3iE2ATGwAAWB0awg (envelope-from ) for ; Fri, 29 Jan 2021 05:23:57 -0500 Received: by simark.ca (Postfix, from userid 112) id DDB411EF80; Fri, 29 Jan 2021 05:23:57 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id B23711E939 for ; Fri, 29 Jan 2021 05:23:56 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 033453858009; Fri, 29 Jan 2021 10:23:56 +0000 (GMT) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id E45503858009 for ; Fri, 29 Jan 2021 10:23:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E45503858009 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tdevries@suse.de X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 0386CACBA; Fri, 29 Jan 2021 10:23:53 +0000 (UTC) From: Tom de Vries Subject: Re: [PATCH][gdb/breakpoint] Fix stepping past non-stmt line-table entries To: Bernd Edlinger , gdb-patches@sourceware.org References: <20210114132632.GA31266@delia> Message-ID: <53da543a-2ed3-de6e-4b11-b7dfdd522291@suse.de> Date: Fri, 29 Jan 2021 11:23:52 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 8bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 1/23/21 9:07 AM, Bernd Edlinger wrote: > On 1/14/21 2:26 PM, Tom de Vries wrote: >> Hi, >> >> Consider the test-case small.c: >> ... >> $ cat -n small.c >> 1 __attribute__ ((noinline, noclone)) >> 2 int foo (char *c) >> 3 { >> 4 asm volatile ("" : : "r" (c) : "memory"); >> 5 return 1; >> 6 } >> 7 >> 8 int main () >> 9 { >> 10 char tpl1[20] = "/tmp/test.XXX"; >> 11 char tpl2[20] = "/tmp/test.XXX"; >> 12 int fd1 = foo (tpl1); >> 13 int fd2 = foo (tpl2); >> 14 if (fd1 == -1) { >> 15 return 1; >> 16 } >> 17 >> 18 return 0; >> 19 } >> ... >> >> Compiled with gcc-8 and optimization: >> ... >> $ gcc-8 -O2 -g small.c >> ... >> >> We step through the calls to foo, but fail to visit line 13: >> ... >> 12 int fd1 = foo (tpl1); >> (gdb) step >> foo (c=c@entry=0x7fffffffdea0 "/tmp/test.XXX") at small.c:5 >> 5 return 1; >> (gdb) step >> foo (c=c@entry=0x7fffffffdec0 "/tmp/test.XXX") at small.c:5 >> 5 return 1; >> (gdb) step >> main () at small.c:14 >> 14 if (fd1 == -1) { >> (gdb) >> ... >> >> This is caused by the following. The calls to foo are implemented by these >> insns: >> .... >> 4003df: 0f 29 04 24 movaps %xmm0,(%rsp) >> 4003e3: 0f 29 44 24 20 movaps %xmm0,0x20(%rsp) >> 4003e8: e8 03 01 00 00 callq 4004f0 >> 4003ed: 48 8d 7c 24 20 lea 0x20(%rsp),%rdi >> 4003f2: 89 c2 mov %eax,%edx >> 4003f4: e8 f7 00 00 00 callq 4004f0 >> 4003f9: 31 c0 xor %eax,%eax >> ... >> with corresponding line table entries: >> ... >> INDEX LINE ADDRESS IS-STMT >> 8 12 0x00000000004003df Y >> 9 10 0x00000000004003df >> 10 11 0x00000000004003e3 >> 11 12 0x00000000004003e8 >> 12 13 0x00000000004003ed >> 13 12 0x00000000004003f2 >> 14 13 0x00000000004003f4 Y >> 15 13 0x00000000004003f4 >> 16 14 0x00000000004003f9 Y >> 17 14 0x00000000004003f9 >> ... >> >> Once we step out of the call to foo at 4003e8, we land at 4003ed, and gdb >> enters process_event_stop_test to figure out what to do. >> >> That entry has is-stmt=n, so it's not the start of a line, so we don't stop >> there. However, we do update ecs->event_thread->current_line to line 13, >> because the frame has changed (because we stepped out of the function). >> >> Next we land at 4003f2. Again the entry has is-stmt=n, so it's not the start >> of a line, so we don't stop there. However, because the frame hasn't changed, >> we don't update update ecs->event_thread->current_line, so it stays 13. >> >> Next we land at 4003f4. Now is-stmt=y, so it's the start of a line, and we'd >> like to stop here. >> >> But we don't stop because this test fails: >> ... >> if ((ecs->event_thread->suspend.stop_pc == stop_pc_sal.pc) >> && (ecs->event_thread->current_line != stop_pc_sal.line >> || ecs->event_thread->current_symtab != stop_pc_sal.symtab)) >> { >> ... >> because ecs->event_thread->current_line == 13 and stop_pc_sal.line == 13. >> >> Fix this by resetting ecs->event_thread->current_line to 0 if is-stmt=n and >> the frame has changed, such that we have: >> ... >> 12 int fd1 = foo (tpl1); >> (gdb) step >> foo (c=c@entry=0x7fffffffdbc0 "/tmp/test.XXX") at small.c:5 >> 5 return 1; >> (gdb) step >> main () at small.c:13 >> 13 int fd2 = foo (tpl2); >> (gdb) >> ... >> >> Tested on x86_64-linux, with gcc-7 and gcc-8. >> >> Any comments? >> > > I agree that this should be fixed. > Hi Bernd, thanks for the review. > It is interesting that the small.c example does not miss to > step at line 13 if I manually add a nop after the call foo. > In this case we are still at line 12, where the call is coming > from. > I can reproduce that like so: ... call foo + nop .LVL1: .loc 1 13 13 view .LVU15 leaq 32(%rsp), %rdi .loc 1 12 13 view .LVU16 movl %eax, %edx .LVL2: .loc 1 13 3 is_stmt 1 view .LVU17 .loc 1 13 13 is_stmt 0 view .LVU18 call foo ... resulting in: ... 4003d7: 0f 29 04 24 movaps %xmm0,(%rsp) 4003db: 0f 29 44 24 20 movaps %xmm0,0x20(%rsp) 4003e0: c7 44 24 30 00 00 00 movl $0x0,0x30(%rsp) 4003e7: 00 4003e8: e8 03 01 00 00 callq 4004f0 4003ed: 90 nop 4003ee: 48 8d 7c 24 20 lea 0x20(%rsp),%rdi 4003f3: 89 c2 mov %eax,%edx 4003f5: e8 f6 00 00 00 callq 4004f0 4003fa: 31 c0 xor %eax,%eax ... So, once we step out of the call to foo at 4003e8, we land at the nop at 4003ed, and gdb enters process_event_stop_test to figure out what to do. Since there's no line number entry at that address, we get: ... (gdb) p stop_pc_sal.line $7 = 12 ... instead of line 13, which causes the stepping to stop at the subsequent is-stmt=y entry at line 13. > I think when we step away from the call instruction we should > use the line info from the call statement which is 12 in this > example, not the non-stmt line immediately after the call > stmt which is 13. > > find_pc_line can be used to find the line immediately before > the current pc when the second parameter is non-zero. > > So how about this: > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 7bbfe04..c823586 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -7048,6 +7048,19 @@ struct wait_one_event > infrun_debug_printf ("stepped to a different line, but " > "it's not the start of a statement"); > } > + else > + { > + struct symtab_and_line call_line; > + > + /* We are returning from a call immediatley before the stop_pc. > + Therefore we are stepping away from the call line. */ > + call_line = find_pc_line (ecs->event_thread->suspend.stop_pc, 1); > + if (call_line.line != 0) > + { > + stop_pc_sal.line = call_line.line; > + stop_pc_sal.symtab = call_line.symtab; > + } > + } > } > > /* We aren't done stepping. I think we should use either the line number indicated by the line table, or 0. I don't see a need to make things more complicated than that. The preferred solution would be to use the one from the line table, but I think that will require more changes to the stepping mechanism, so setting the line to 0 is a nice, localized solution. I've added a debug print to note the case: ... + infrun_debug_printf ("stepped to a different frame, but " + "it's not the start of a statement"); + ... and I'll commit shortly. Thanks, - Tom