Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler

[Kevin Buettner <kevinb@redhat.com>]

On Fri, 21 Jun 2019 00:23:14 +0100
Andrew Burgess <andrew.burgess@embecosm.com> wrote:

> * Andrew Burgess <andrew.burgess@embecosm.com> [2019-06-20 21:57:59 +0100]:
> 
> > * Kevin Buettner <kevinb@redhat.com> [2019-06-19 18:11:47 -0700]:
> >   
> > > On Wed, 12 Jun 2019 13:34:03 +0100
> > > Andrew Burgess <andrew.burgess@embecosm.com> 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