gdb: Don't skip prologue for explicit line breakpoints in assembler

  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.

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.

I have a new test that covers my original case, this is written for
x86-64 as most folk have access to such a target, however, any
architecture that has a prologue scanner can be impacted by this
change.

gdb/ChangeLog:

	* linespec.c (decode_digits_list_mode): Set explicit_line to a
	bool value.
	(decode_digits_ordinary): Set explicit_line field in sal.
	* symtab.c (skip_prologue_sal): Don't skip prologue for a
	symtab_and_line that was set on an explicit line number in
	assembler code.  Do always update the recorded symtab and line if
	we do skip the prologue.

gdb/testsuite/ChangeLog:

	* gdb.arch/amd64-break-on-asm-line.S: New file.
	* gdb.arch/amd64-break-on-asm-line.exp: New file.
---
 gdb/ChangeLog                                      | 10 +++++++
 gdb/linespec.c                                     |  3 +-
 gdb/symtab.c                                       | 15 ++++++----
 gdb/testsuite/ChangeLog                            |  5 ++++
 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S   | 35 ++++++++++++++++++++++
 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.exp | 35 ++++++++++++++++++++++
 6 files changed, 96 insertions(+), 7 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S
 create mode 100644 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.exp
  

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

Kevin
  

* 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");
  +      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.

Thanks,
Andrew


> 
> Kevin
  

* 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.

Thanks,
Andrew
  

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
  

On 6/21/19 12:23 AM, Andrew Burgess wrote:

> 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.

But won't that change affect any code path that ends up in
skip_prologue_sal with explicit_line set?

E.g.:

/* Helper function for break_command_1 and disassemble_command.  */

void
resolve_sal_pc (struct symtab_and_line *sal)
{
  CORE_ADDR pc;

  if (sal->pc == 0 && sal->symtab != NULL)
    {
      if (!find_line_pc (sal->symtab, sal->line, &pc))
	error (_("No line %d in file \"%s\"."),
	       sal->line, symtab_to_filename_for_display (sal->symtab));
      sal->pc = pc;

      /* If this SAL corresponds to a breakpoint inserted using a line
         number, then skip the function prologue if necessary.  */
      if (sal->explicit_line)
	skip_prologue_sal (sal);
    }

Is that path unreachable today?


> 
> 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.

I seem to recall that GDB didn't use to update a breakpoint's line
number to advance to the next line number that includes some actual
compiled code.  Like if you set a breakpoint at line 10 below:

 10    // just a comment
 11    i++;

you end up with a breakpoint at line 11.  Maybe it's old code
related to that.

Maybe I'm misremembering.

In any case, if you change this, then you also should change
the function's entry comment:

 /* Adjust SAL to the first instruction past the function prologue.
    If the PC was explicitly specified, the SAL is not changed.
    If the line number was explicitly specified, at most the SAL's PC
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    is updated.  If SAL is already past the prologue, then do nothing.  */
    ^^^^^^^^^^
 
 void
 skip_prologue_sal (struct symtab_and_line *sal)
 {

Thanks,
Pedro Alves