From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24528 invoked by alias); 21 Jun 2019 03:20:35 -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 24520 invoked by uid 89); 21 Jun 2019 03:20:35 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=compelling 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 ESMTP; Fri, 21 Jun 2019 03:20:33 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 28766308624B; Fri, 21 Jun 2019 03:20:32 +0000 (UTC) Received: from f29-4.lan (ovpn-117-224.phx2.redhat.com [10.3.117.224]) by smtp.corp.redhat.com (Postfix) with ESMTPS id ED1EB60605; Fri, 21 Jun 2019 03:20:31 +0000 (UTC) Date: Fri, 21 Jun 2019 03:20:00 -0000 From: Kevin Buettner To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler Message-ID: <20190620202031.57c9b478@f29-4.lan> In-Reply-To: <20190620232314.GJ23204@embecosm.com> References: <20190612123403.14348-1-andrew.burgess@embecosm.com> <20190619181147.69974f43@f29-4.lan> <20190620205759.GI23204@embecosm.com> <20190620232314.GJ23204@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-06/txt/msg00406.txt.bz2 On Fri, 21 Jun 2019 00:23:14 +0100 Andrew Burgess wrote: > * Andrew Burgess [2019-06-20 21:57:59 +0100]: > > > * Kevin Buettner [2019-06-19 18:11:47 -0700]: > > > > > On Wed, 12 Jun 2019 13:34:03 +0100 > > > Andrew Burgess wrote: > > > > > > > It was observed that in some cases, placing a breakpoint in an > > > > assembler file using filename:line-number syntax would result in the > > > > breakpoint being placed at a different line within the file. > > > > > > > > For example, consider this x86-64 assembler: > > > > > > > > test: > > > > push %rbp /* Break here. */ > > > > mov %rsp, %rbp > > > > nop /* Stops here. */ > > > > > > > > The user places the breakpoint using file:line notation targeting the > > > > line marked 'Break here', GDB actually stops at the line marked 'Stops > > > > here'. > > > > > > > > The reason is that the label 'test' is identified as the likely start > > > > of a function, and the call to symtab.c:skip_prologue_sal causes GDB > > > > to skip forward over the instructions that GDB believes to be part of > > > > the prologue. > > > > > > > > I believe however, that when debugging assembler code, where the user > > > > has instruction-by-instruction visibility, if they ask for a specific > > > > line, GDB should (as far as possible) stop on that line, and not > > > > perform any prologue skipping. I don't believe that the behaviour of > > > > higher level languages should change, in these cases skipping the > > > > prologue seems like the correct thing to do. > > > > > > I agree with all of this. > > > > > > > In order to implement this change I needed to extend our current > > > > tracking of when the user has requested an explicit line number. We > > > > already tracked this in some cases, but not in others (see the changes > > > > in linespec.c). However, once I did this I started to see some > > > > additional failures (in tests gdb.base/break-include.exp > > > > gdb.base/ending-run.exp gdb.mi/mi-break.exp gdb.mi/mi-reverse.exp > > > > gdb.mi/mi-simplerun.exp) where we currently expected a breakpoint > > > > placed at one file and line number to be updated to reference a > > > > different line number, this was fixed by removing some code in > > > > symtab.c:skip_prologue_sal. My concern here is that removing this > > > > check didn't cause anything else to fail. > > > > > > Did you investigate the reason for the failures with this hunk > > > left in place?... > > > > > > > @@ -3812,12 +3821,6 @@ skip_prologue_sal (struct symtab_and_line *sal) > > > > > > > > sal->pc = pc; > > > > sal->section = section; > > > > - > > > > - /* Unless the explicit_line flag was set, update the SAL line > > > > - and symtab to correspond to the modified PC location. */ > > > > - if (sal->explicit_line) > > > > - return; > > > > - > > > > sal->symt> FAIL: gdb.base/break-include.exp: break break-include.c:53 > > > ab = start_sal.symtab; > > > > sal->line = start_sal.line; > > > > sal->end = start_sal.end; > > > > > > The rest of the patch looks fine to me. Deleting those lines might > > > be okay also, but I'd like to understand why adding additional > > > explicit line number tracking caused these failures: > > > > > > FAIL: gdb.base/break-include.exp: break break-include.c:53 > > > FAIL: gdb.base/ending-run.exp: Cleared 2 by line > > > FAIL: gdb.base/ending-run.exp: clear 2 by default > > > > Thanks for taking a look at this. > > > > Just to be clear, this is the hunk that is in question (in symtab.c): > > > > @@ -3812,12 +3821,6 @@ skip_prologue_sal (struct symtab_and_line *sal) > > > > sal->pc = pc; > > sal->section = section; > > - > > - /* Unless the explicit_line flag was set, update the SAL line > > - and symtab to correspond to the modified PC location. */ > > - if (sal->explicit_line) > > - return; > > - > > sal->symtab = start_sal.symtab; > > sal->line = start_sal.line; > > sal->end = start_sal.end; > > > > > > If we take the first of these 'gdb.base/break-include.exp: break > > break-include.c:53', then in a pre-patched test run the log looks like > > this: > > > > (gdb) break break-include.c:53 > > Breakpoint 1 at 0x4004af: file /path/to/gdb/src/gdb/testsuite/gdb.base/break-include.inc, line 18. > > (gdb) PASS: gdb.base/break-include.exp: break break-include.c:53 > > > > And in a post-patch world, but with the hunk above removed here's the > > same part of the log file: > > > > (gdb) break break-include.c:53 > > Breakpoint 1 at 0x4004af: file /path/to/gdb/src/gdb/testsuite/gdb.base/break-include.c, line 54. > > (gdb) FAIL: gdb.base/break-include.exp: break break-include.c:53 > > > > What we see is that in the failing case the line number has failed to > > update from break-include.c:53 to break-include.inc:18, instead it has > > updated to break-include.c:54. > > > > To understand what's going on we need to consider two steps, first in > > convert_linespec_to_sals the file and line is used to index into the > > line table, this is where the break-include.c:54 comes from, there is > > no entry for line 53, and 54 is the next available line. > > > > Then skip_prologue_sal is called, this is where we move forward to > > break-include.inc line 18, and this is where the difference is. > > > > In the pre-patch world, the sal that is passe into skip_prologue_sal > > is not marked as explicit_line, and so we successfully update the file > > and line. > > > > In the post patch world, the sal now is marked as explicit_line, so > > the above check triggers and GDB doesn't update the file/line in the > > sal, this leaves us stuck on break-include.c:54. > > > > The other two failures all stem from the exact same problem, in > > gdb.base/ending-run.exp many breakpoints are placed, and then cleared > > using the 'clear' command. One of the breakpoints (breakpoint 4) is > > placed at the wrong file/line as a result of not updating, this then > > causes the 'clear' tests to not clear the expected breakpoints. > > > > What worries more about the above hunk is that it never triggers > > during testing (in a pre-patched world). I applied this patch to > > master: > > > > diff --git a/gdb/symtab.c b/gdb/symtab.c > > index 4920d94a247..5cd5bb69147 100644 > > --- a/gdb/symtab.c > > +++ b/gdb/symtab.c > > @@ -3816,7 +3816,11 @@ skip_prologue_sal (struct symtab_and_line *sal) > > /* Unless the explicit_line flag was set, update the SAL line > > and symtab to correspond to the modified PC location. */ > > if (sal->explicit_line) > > - return; > > + { > > + fprintf ("Got here!\n"); > > Ooops, that should be 'fprintf (stderr, "Got here!\n");' of course. > > > + abort (); > > + return; > > + } > > > > sal->symtab = start_sal.symtab; > > sal->line = start_sal.line; > > > > And all the tests still pass. This code has been in place for ~9 > > years and unfortunately didn't have any tests associated when it was > > added. > > > > I've spent some time trying to figure out what conditions might need > > to be true in order to trigger this code, but so far I've not managed > > to figure it out - any suggestions would be appreciated. > > I spent some more time trying to find a path that would call both > 'decode_digits_list_mode' and then 'skip_prologue_sal', but I still > can't find one. > > Looking back at how the explicit_line flag was originally used when > it was added in commit ed0616c6b78a0966, things have changed quite a > bit in the 10+ years since. There were some tests added along with > the explicit_line flag (gdb.cp/mb-*.exp) and these all pass both in > master and in my patched branch. > > My current thinking is that the explicit_line flag was no longer doing > anything useful in master, but if someone disagrees I'd love to > understand more about this. Hi Andrew, Thanks for the additional analysis and explanation. I found the part about the "Got here!" printf to be especially compelling. Anyway, I'm convinced, so... Okay. Kevin