From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 91969 invoked by alias); 20 Jun 2019 23:23:21 -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 91960 invoked by uid 89); 20 Jun 2019 23:23:20 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail-wr1-f43.google.com Received: from mail-wr1-f43.google.com (HELO mail-wr1-f43.google.com) (209.85.221.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 20 Jun 2019 23:23:18 +0000 Received: by mail-wr1-f43.google.com with SMTP id r16so4652815wrl.11 for ; Thu, 20 Jun 2019 16:23:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=hFXdkdVXL0tO3AvrYpMbXmrrtdQ/h1M99kpE68QmkxI=; b=FAdJnqVSUlvlwx2u+Qlklj3YV6iU57oMne+3hu55TnNTXlGpFc3GIgwRgKbhWSSgHP vcGPQ2wqtAGL/kRiSURM//pCmBmFHlHzLgZG90R8R1fcIAuhtfxyeSgYjfS90W3dPUBE u1q6hKrj+nQKsG5IciCdRx2xU/x1gkfVhCINxnJ9Fk6gG/31tc7/9GQSd3GZ0dzhi3Fd AdIEGyod+2rfityebc0axN8rPB+dCoHHESz36GT5v+F6DrxnhxF4vjio0Y8DwLhfFRHI TcwcE/5bVBXyPrk7QA0gXa2ARcnamEnz5lUl+CHT3W6C8fmFWSvt8roeYwSaJwksvZmt HPlw== Return-Path: Received: from localhost (host86-180-62-212.range86-180.btcentralplus.com. [86.180.62.212]) by smtp.gmail.com with ESMTPSA id x16sm731995wmj.4.2019.06.20.16.23.15 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 20 Jun 2019 16:23:15 -0700 (PDT) Date: Thu, 20 Jun 2019 23:23:00 -0000 From: Andrew Burgess To: Kevin Buettner Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler Message-ID: <20190620232314.GJ23204@embecosm.com> References: <20190612123403.14348-1-andrew.burgess@embecosm.com> <20190619181147.69974f43@f29-4.lan> <20190620205759.GI23204@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190620205759.GI23204@embecosm.com> X-Fortune: Variables don't User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2019-06/txt/msg00404.txt.bz2 * 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. Thanks, Andrew