diff mbox

[PING**2] Fix an issue with the gdb step-over aka. "n" command

Message ID 20191230221238.GQ3865@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess Dec. 30, 2019, 10:12 p.m. UTC
I've taken a look through this patch, and I think that we should
consider adding real support for the line table is_stmt attribute to
GDB as an alternative.

I had a crack at doing this and it fixes the test case as you
originally describe it, with the inline function defined in the header
file, and the caller in the .cc file - however it still doesn't fix
the case where the inline function is in the same source file as the
caller, however, I'm convinced this is due to incorrect DWARF being
generated by GCC rather than a fault of GDB.

As a test case I'm using next-inline2.cc, which I've included below, I
compile this using GCC 9.2.0 though I've also tested 10.xx, and 8.3.0
which are no different.  I also tried 7.4.0 which is different, and
I'll discuss below.

The interesting part of the source file is as follows, with line
numbers on the left for reference:

  28:  inline tree *
  29:  tree_check (tree *t, int i)
  30:  {
  31:    if (t->x != i)
  32:      abort();
  33:    tree *x = t;
  34:    return x;
  35:  }
  36:
  37:  int __attribute__((noinline, noclone))
  38:  get_alias_set (tree *t)
  39:  {
  40:    if (t != NULL
  41:        && TREE_TYPE (t).z != 1
  42:        && TREE_TYPE (t).z != 2
  43:        && TREE_TYPE (t).z != 3)
  44:      return 0;
  45:    return 1;
  46:  }

I compile the test with 'g++ -O2 -g3 -o next-inline2 next-inline2.cc',
and the decoded line table (or part of it) looks like:

  Contents of the .debug_line section:

  CU: ./next-inline2.cc:
  File name                            Line number    Starting address    View    Stmt
  next-inline2.cc                               39            0x400540               x
  next-inline2.cc                               40            0x400540       1       x
  next-inline2.cc                               43            0x400540       2
  next-inline2.cc                               31            0x400545               x
  next-inline2.cc                               39            0x400545       1
  next-inline2.cc                               31            0x400549
  next-inline2.cc                               31            0x40054b
  next-inline2.cc                               33            0x400553               x
  next-inline2.cc                               34            0x400553       1       x
  next-inline2.cc                               41            0x400553       2
  next-inline2.cc                               41            0x400556
  next-inline2.cc                               31            0x40055b               x
  next-inline2.cc                               31            0x40055b       1
  next-inline2.cc                               31            0x40055d
  next-inline2.cc                               33            0x400565               x
  next-inline2.cc                               34            0x400565       1       x
  next-inline2.cc                               42            0x400565       2
  next-inline2.cc                               42            0x400568

The other interesting thing we need to understand is where the inline
subroutines are, in the source code above the inline subroutines are
hidden behind the TREE_TYPE macro. Looking at the DWARF we see 3
instances of DW_TAG_inlined_subroutine, they are:

 <2><8e8>: Abbrev Number: 39 (DW_TAG_inlined_subroutine)
    <8e9>   DW_AT_abstract_origin: <0x9a2>
    <8ed>   DW_AT_entry_pc    : 0x400545
    <8f5>   DW_AT_ranges      : 0x30
    <8f9>   DW_AT_call_file   : 1
    <8fa>   DW_AT_call_line   : 41
    <8fb>   DW_AT_call_column : 10
    <8fc>   DW_AT_sibling     : <0x92f>

 <2><92f>: Abbrev Number: 43 (DW_TAG_inlined_subroutine)
    <930>   DW_AT_abstract_origin: <0x9a2>
    <934>   DW_AT_low_pc      : 0x40055b
    <93c>   DW_AT_high_pc     : 0xa
    <944>   DW_AT_call_file   : 1
    <945>   DW_AT_call_line   : 42
    <946>   DW_AT_call_column : 10
    <947>   DW_AT_sibling     : <0x967>

 <2><967>: Abbrev Number: 44 (DW_TAG_inlined_subroutine)
    <968>   DW_AT_abstract_origin: <0x9a2>
    <96c>   DW_AT_low_pc      : 0x40056d
    <974>   DW_AT_high_pc     : 0xa
    <97c>   DW_AT_call_file   : 1
    <97d>   DW_AT_call_line   : 43
    <97e>   DW_AT_call_column : 10

Notice in all cases we reference abstract origin 0x9a2, this is the
same subroutine inlined 3 times.  Also notice the difference with how
the address ranges are defined.  In the last two cases the address
ranges for the inlined subroutines start at 0x40055b and 0x40056d and
continue for 10 bytes.  In the first case however, the address range
is in the .debug_ranges table, and looks like this:

    00000030 0000000000400545 0000000000400545 (start == end)
    00000030 0000000000400549 0000000000400553
    00000030 0000000000400430 0000000000400435
    00000030 <End of list>

Here we have the first clue that something has gone wrong, the 'start
== end' warning tells us that there's at the very least a dummy entry
n in the ranges table (could happen with relaxation maybe, or possibly
linker garbage collection, but its still a warning sign I think).

So the next job is to try and merge all this information into one
table and see what we can learn.  In the following table I use the
following short-hand:

 'In' column indicates a range of addresses covered by an inline
 function range.

 'Ln' column contains line table information, where:
      + A number e.g. '39', indicates line 39 is specified to be
        at this address in the line table, and is a statement, that is a
        good place to insert a breakpoint.
      + An underscore and number, e.g. '_43' indicates line 43 is
        specified to be at this address in the line table, but this is
        not a statement, that is, this is not a good place to put a
        breakpoint if you want to stop at this line.
      + A colon and number, e.g. ':43' indicates we infer that line 43
        is here based on the last entry that was in the line table,
        but there is no direct entry for this address in the line table.

  'Address' and 'Disassembly Text' are just from the disassembly output.

| In | Ln         | Address                    | Disassembly Text                              |
|----+------------+----------------------------+-----------------------------------------------|
|    | 39,40,_43  | 0x0000000000400540 <+0>:   | test   %rdi,%rdi                              |
|    | :43        | 0x0000000000400543 <+3>:   | je     0x400590 <get_alias_set(tree*)+80>     |
|    | 31, _39    | 0x0000000000400545 <+5>:   | sub    $0x8,%rsp                              |
| X  | _31        | 0x0000000000400549 <+9>:   | mov    (%rdi),%eax                            |
| X  | _31        | 0x000000000040054b <+11>:  | test   %eax,%eax                              |
| X  | :31        | 0x000000000040054d <+13>:  | jne    0x400430<_Z13get_alias_setP4tree.cold> |
|    | 33,34,_41  | 0x0000000000400553 <+19>:  | mov    0x4(%rdi),%eax                         |
|    | _41        | 0x0000000000400556 <+22>:  | cmp    $0x1,%eax                              |
|    | :41        | 0x0000000000400559 <+25>:  | je     0x400583 <get_alias_set(tree*)+67>     |
| X  | 31,_31     | 0x000000000040055b <+27>:  | mov    (%rdi),%eax                            |
| X  | _31        | 0x000000000040055d <+29>:  | test   %eax,%eax                              |
| X  | :31        | 0x000000000040055f <+31>:  | jne    0x400430<_Z13get_alias_setP4tree.cold> |
|    | 33, 34,_42 | 0x0000000000400565 <+37>:  | mov    0x4(%rdi),%eax                         |
|    |            | 0x0000000000400568 <+40>:  | cmp    $0x2,%eax                              |
|    |            | 0x000000000040056b <+43>:  | je     0x4005a0 <get_alias_set(tree*)+96>     |
| X  |            | 0x000000000040056d <+45>:  | mov    (%rdi),%eax                            |
| X  |            | 0x000000000040056f <+47>:  | test   %eax,%eax                              |
| X  |            | 0x0000000000400571 <+49>:  | jne    0x400430<_Z13get_alias_setP4tree.cold> |
|    |            | 0x0000000000400577 <+55>:  | mov    0x4(%rdi),%eax                         |
|    |            | 0x000000000040057a <+58>:  | cmp    $0x3,%eax                              |
|    |            | 0x000000000040057d <+61>:  | sete   %al                                    |


My claim is this, in well structured DWARF and entries for lines 31,
32 33, or 34 should be inside an area marked as belonging to the
inline subroutine, or at a minimum, should not be marked as a
statement and be outside an inlined region.

Take for example address 0x400545, this is a statement line for 31,
and a non-statement line for 39, but is NOT in an inline subroutine.
The DWARF is (I claim) clearly indicating that address 0x400545 comes
from line 31, and is in the function get_alias_set.  By looking at the
source code we can clearly see this is not correct.

There are other inconsistencies, like 0x400553, which claims to be
line 33 and 34, yet is outside the inlined range.

I'm hoping that someone can double check my analysis of the above and
call me out on anything I've gotten wrong.

I wont run the full analysis again, but I mentioned previously that I
tried GCC 7.4.0.  The debug information generated here was not as
sophisticated, but nor was it (I think) so wrong.  The debug range for
the first inlined subroutine entry makes much more sense, there's no
longer the silly 'start == end' thing going on, and the line table is
much simpler.  Consequently the debug experience, though not perfect,
is much better than on any of the later compiler - and this is with my
is_stmt supporting GDB.

I've included below the is_stmt support patch as I currently have it,
but please remember, this is a very quick hack just so anyone
interested can test it.  I'm going to try and clean it up and repost
it maybe tomorrow if I have time.

Thanks,
Andrew

--

## START: next-inline2.cc ##

/* This testcase is part of GDB, the GNU debugger.

   Copyright 2019 Free Software Foundation, Inc.

   This program is free software; you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation; either version 3 of the License, or
   (at your option) any later version.

   This program is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   GNU General Public License for more details.

   You should have received a copy of the GNU General Public License
   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */

#include <stdlib.h>

struct tree
{
  volatile int x;
  volatile int z;
};

#define TREE_TYPE(NODE) (*tree_check (NODE, 0))

inline tree *
tree_check (tree *t, int i)
{
  if (t->x != i)
    abort();
  tree *x = t;
  return x;
}

int __attribute__((noinline, noclone))
get_alias_set (tree *t)
{
  if (t != NULL
      && TREE_TYPE (t).z != 1
      && TREE_TYPE (t).z != 2
      && TREE_TYPE (t).z != 3)
    return 0;
  return 1;
}

tree xx;

int main()
{
  get_alias_set (&xx);
  abort ();
}

## END ##

## START: is_stmt patch ##

## END: is_stmt patch ##

Comments

Bernd Edlinger Jan. 1, 2020, 9:40 a.m. UTC | #1
On 12/30/19 11:12 PM, Andrew Burgess wrote:
> I've taken a look through this patch, and I think that we should
> consider adding real support for the line table is_stmt attribute to
> GDB as an alternative.
> 
> I had a crack at doing this and it fixes the test case as you
> originally describe it, with the inline function defined in the header
> file, and the caller in the .cc file - however it still doesn't fix
> the case where the inline function is in the same source file as the
> caller, however, I'm convinced this is due to incorrect DWARF being
> generated by GCC rather than a fault of GDB.
> 

Okay, the way how this fixes the original test case is similar to
how my patch works:

This code in buildsym_compunit::record_line which eliminates
previous is_stmt locations at the same pc:

  /* Normally, we treat lines as unsorted.  But the end of sequence
     marker is special.  We sort line markers at the same PC by line
     number, so end of sequence markers (which have line == 0) appear
     first.  This is right if the marker ends the previous function,
     and there is no padding before the next function.  But it is
     wrong if the previous line was empty and we are now marking a
     switch to a different subfile.  We must leave the end of sequence
     marker at the end of this group of lines, not sort the empty line
     to after the marker.  The easiest way to accomplish this is to
     delete any empty lines from our table, if they are followed by
     end of sequence markers.  All we lose is the ability to set
     breakpoints at some lines which contain no instructions
     anyway.  */
  if (line == 0 && subfile->line_vector->nitems > 0)
    {
      e = subfile->line_vector->item + subfile->line_vector->nitems - 1;
      while (subfile->line_vector->nitems > 0 && e->pc == pc)
        {
          e--;
          subfile->line_vector->nitems--;
        }
    }

This is now invoked by:

      if (m_record_lines_p)
        {
          if (m_last_subfile != m_cu->get_builder ()->get_current_subfile ()
              || end_sequence)
            {
              dwarf_finish_line (m_gdbarch, m_last_subfile, m_address,
                                 m_currently_recording_lines ? m_cu : nullptr);
            }

previously it was guarded by && (producer_is_codewarrior (m_cu) || m_is_stmt))

That explains why that appears to be fixed, but in a way it is just a bug
that record_lines does this.  Especially when !is_stmt locations follow,
it is probably wrong to completely remove the only is_stmt from the line table.


> As a test case I'm using next-inline2.cc, which I've included below, I
> compile this using GCC 9.2.0 though I've also tested 10.xx, and 8.3.0
> which are no different.  I also tried 7.4.0 which is different, and
> I'll discuss below.
> 
> The interesting part of the source file is as follows, with line
> numbers on the left for reference:
> 
>   28:  inline tree *
>   29:  tree_check (tree *t, int i)
>   30:  {
>   31:    if (t->x != i)
>   32:      abort();
>   33:    tree *x = t;
>   34:    return x;
>   35:  }
>   36:
>   37:  int __attribute__((noinline, noclone))
>   38:  get_alias_set (tree *t)
>   39:  {
>   40:    if (t != NULL
>   41:        && TREE_TYPE (t).z != 1
>   42:        && TREE_TYPE (t).z != 2
>   43:        && TREE_TYPE (t).z != 3)
>   44:      return 0;
>   45:    return 1;
>   46:  }
> 
> I compile the test with 'g++ -O2 -g3 -o next-inline2 next-inline2.cc',
> and the decoded line table (or part of it) looks like:
> 
>   Contents of the .debug_line section:
> 
>   CU: ./next-inline2.cc:
>   File name                            Line number    Starting address    View    Stmt
>   next-inline2.cc                               39            0x400540               x
>   next-inline2.cc                               40            0x400540       1       x
>   next-inline2.cc                               43            0x400540       2
>   next-inline2.cc                               31            0x400545               x
>   next-inline2.cc                               39            0x400545       1
>   next-inline2.cc                               31            0x400549
>   next-inline2.cc                               31            0x40054b
>   next-inline2.cc                               33            0x400553               x
>   next-inline2.cc                               34            0x400553       1       x
>   next-inline2.cc                               41            0x400553       2
>   next-inline2.cc                               41            0x400556
>   next-inline2.cc                               31            0x40055b               x
>   next-inline2.cc                               31            0x40055b       1
>   next-inline2.cc                               31            0x40055d
>   next-inline2.cc                               33            0x400565               x
>   next-inline2.cc                               34            0x400565       1       x
>   next-inline2.cc                               42            0x400565       2
>   next-inline2.cc                               42            0x400568
> 
> The other interesting thing we need to understand is where the inline
> subroutines are, in the source code above the inline subroutines are
> hidden behind the TREE_TYPE macro. Looking at the DWARF we see 3
> instances of DW_TAG_inlined_subroutine, they are:
> 
>  <2><8e8>: Abbrev Number: 39 (DW_TAG_inlined_subroutine)
>     <8e9>   DW_AT_abstract_origin: <0x9a2>
>     <8ed>   DW_AT_entry_pc    : 0x400545
>     <8f5>   DW_AT_ranges      : 0x30
>     <8f9>   DW_AT_call_file   : 1
>     <8fa>   DW_AT_call_line   : 41
>     <8fb>   DW_AT_call_column : 10
>     <8fc>   DW_AT_sibling     : <0x92f>
> 
>  <2><92f>: Abbrev Number: 43 (DW_TAG_inlined_subroutine)
>     <930>   DW_AT_abstract_origin: <0x9a2>
>     <934>   DW_AT_low_pc      : 0x40055b
>     <93c>   DW_AT_high_pc     : 0xa
>     <944>   DW_AT_call_file   : 1
>     <945>   DW_AT_call_line   : 42
>     <946>   DW_AT_call_column : 10
>     <947>   DW_AT_sibling     : <0x967>
> 
>  <2><967>: Abbrev Number: 44 (DW_TAG_inlined_subroutine)
>     <968>   DW_AT_abstract_origin: <0x9a2>
>     <96c>   DW_AT_low_pc      : 0x40056d
>     <974>   DW_AT_high_pc     : 0xa
>     <97c>   DW_AT_call_file   : 1
>     <97d>   DW_AT_call_line   : 43
>     <97e>   DW_AT_call_column : 10
> 
> Notice in all cases we reference abstract origin 0x9a2, this is the
> same subroutine inlined 3 times.  Also notice the difference with how
> the address ranges are defined.  In the last two cases the address
> ranges for the inlined subroutines start at 0x40055b and 0x40056d and
> continue for 10 bytes.  In the first case however, the address range
> is in the .debug_ranges table, and looks like this:
> 
>     00000030 0000000000400545 0000000000400545 (start == end)
>     00000030 0000000000400549 0000000000400553
>     00000030 0000000000400430 0000000000400435
>     00000030 <End of list>
> 
> Here we have the first clue that something has gone wrong, the 'start
> == end' warning tells us that there's at the very least a dummy entry
> n in the ranges table (could happen with relaxation maybe, or possibly
> linker garbage collection, but its still a warning sign I think).
> 
> So the next job is to try and merge all this information into one
> table and see what we can learn.  In the following table I use the
> following short-hand:
> 
>  'In' column indicates a range of addresses covered by an inline
>  function range.
> 
>  'Ln' column contains line table information, where:
>       + A number e.g. '39', indicates line 39 is specified to be
>         at this address in the line table, and is a statement, that is a
>         good place to insert a breakpoint.
>       + An underscore and number, e.g. '_43' indicates line 43 is
>         specified to be at this address in the line table, but this is
>         not a statement, that is, this is not a good place to put a
>         breakpoint if you want to stop at this line.
>       + A colon and number, e.g. ':43' indicates we infer that line 43
>         is here based on the last entry that was in the line table,
>         but there is no direct entry for this address in the line table.
> 
>   'Address' and 'Disassembly Text' are just from the disassembly output.
> 
> | In | Ln         | Address                    | Disassembly Text                              |
> |----+------------+----------------------------+-----------------------------------------------|
> |    | 39,40,_43  | 0x0000000000400540 <+0>:   | test   %rdi,%rdi                              |
> |    | :43        | 0x0000000000400543 <+3>:   | je     0x400590 <get_alias_set(tree*)+80>     |
> |    | 31, _39    | 0x0000000000400545 <+5>:   | sub    $0x8,%rsp                              |
> | X  | _31        | 0x0000000000400549 <+9>:   | mov    (%rdi),%eax                            |
> | X  | _31        | 0x000000000040054b <+11>:  | test   %eax,%eax                              |
> | X  | :31        | 0x000000000040054d <+13>:  | jne    0x400430<_Z13get_alias_setP4tree.cold> |
> |    | 33,34,_41  | 0x0000000000400553 <+19>:  | mov    0x4(%rdi),%eax                         |
> |    | _41        | 0x0000000000400556 <+22>:  | cmp    $0x1,%eax                              |
> |    | :41        | 0x0000000000400559 <+25>:  | je     0x400583 <get_alias_set(tree*)+67>     |
> | X  | 31,_31     | 0x000000000040055b <+27>:  | mov    (%rdi),%eax                            |
> | X  | _31        | 0x000000000040055d <+29>:  | test   %eax,%eax                              |
> | X  | :31        | 0x000000000040055f <+31>:  | jne    0x400430<_Z13get_alias_setP4tree.cold> |
> |    | 33, 34,_42 | 0x0000000000400565 <+37>:  | mov    0x4(%rdi),%eax                         |
> |    |            | 0x0000000000400568 <+40>:  | cmp    $0x2,%eax                              |
> |    |            | 0x000000000040056b <+43>:  | je     0x4005a0 <get_alias_set(tree*)+96>     |
> | X  |            | 0x000000000040056d <+45>:  | mov    (%rdi),%eax                            |
> | X  |            | 0x000000000040056f <+47>:  | test   %eax,%eax                              |
> | X  |            | 0x0000000000400571 <+49>:  | jne    0x400430<_Z13get_alias_setP4tree.cold> |
> |    |            | 0x0000000000400577 <+55>:  | mov    0x4(%rdi),%eax                         |
> |    |            | 0x000000000040057a <+58>:  | cmp    $0x3,%eax                              |
> |    |            | 0x000000000040057d <+61>:  | sete   %al                                    |
> 
> 
> My claim is this, in well structured DWARF and entries for lines 31,
> 32 33, or 34 should be inside an area marked as belonging to the
> inline subroutine, or at a minimum, should not be marked as a
> statement and be outside an inlined region.
> 
> Take for example address 0x400545, this is a statement line for 31,
> and a non-statement line for 39, but is NOT in an inline subroutine.
> The DWARF is (I claim) clearly indicating that address 0x400545 comes
> from line 31, and is in the function get_alias_set.  By looking at the
> source code we can clearly see this is not correct.
> 
> There are other inconsistencies, like 0x400553, which claims to be
> line 33 and 34, yet is outside the inlined range.
> 
> I'm hoping that someone can double check my analysis of the above and
> call me out on anything I've gotten wrong.
> 

Yes I agree, with your analysis, however I believe that is not a
problem of the implementation but of the specification.

Maybe it helps to get the background if you look at my attempt of fixing this as
a gcc bug:

https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01459.html

and Alexandre Oliva's response here:

https://gcc.gnu.org/ml/gcc-patches/2019-11/msg01771.html

So this is a problem in the design of the dwarf line info which
has stmt-type line info at the end of a inlined subroutine.

I think the problem appears always at the end of the inline range,
where it is ambiguous if the line number is part of the subroutine
or not. It is less ambiguous if the subroutine is in a header file,
which is fortunately the common case.

What I think gcc is trying to tell us here is the location view
of certain variables at the end of the subroutine. Note I added
this statement on purpose, because I knew that already:

tree *x = t;
return x;

So this introduces a new name, that does require zero instructions.

But it inherits the is_stmt property of the unoptimized code, also
as desinged.  And the location view seems to tell us the value of x at
this very spot.  I am however not sure if it is good to make "s"
stop at every location that is not a statement barrier, I think
these might be good to have in a source-annotation in the
disassembler view.

BTW: if I use the gcc patch from the posting above, it fixes
next-inline.cc and next-inline2.cc at once, but not this one:

## START: next-inline3.cc ##

/* This testcase is part of GDB, the GNU debugger.

   Copyright 2020 Free Software Foundation, Inc.

   This program is free software; you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation; either version 3 of the License, or
   (at your option) any later version.

   This program is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   GNU General Public License for more details.

   You should have received a copy of the GNU General Public License
   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */

#include <stdlib.h>

struct tree
{
  volatile int x;
  volatile int z;
};

#define TREE_TYPE(NODE) (*tree_check (NODE, 0))

inline tree *
tree_check (tree *t, int i);

int __attribute__((noinline, noclone))
get_alias_set (tree *t)
{
  if (t != NULL
      && TREE_TYPE (t).z != 1
      && TREE_TYPE (t).z != 2
      && TREE_TYPE (t).z != 3)
    return 0;
  return 1;
}

tree xx;

inline tree *
tree_check (tree *t, int i)
{
  if (t->x != i)
    abort();
  tree *x = t;
  return x;
}

int main()
{
  get_alias_set (&xx);
  abort ();
}

## END ##

however, together with your other patch about the stable sorting
of the line numbers it would fix next-inline3.cc as well.

I tried your patch against the original problem, that is debugging
in gcc.

$ gcc -S next-inline2.cc -wrapper gdb,--args
GNU gdb (GDB) 10.0.50.20191231-git
(gdb) b get_alias_set
Breakpoint 1 at 0xb2d8e0: file ../../gcc-trunk/gcc/alias.c, line 837.
(gdb) r
Starting program: /home/ed/gnu/install/libexec/gcc/x86_64-pc-linux-gnu/10.0.0/cc1plus -quiet -imultiarch x86_64-linux-gnu -D_GNU_SOURCE next-inline2.cc -quiet -dumpbase next-inline2.cc -mtune=generic -march=x86-64 -auxbase next-inline2 -o next-inline2.s

Breakpoint 1, get_alias_set (t=t@entry=0x7ffff74bd7e0)
    at ../../gcc-trunk/gcc/alias.c:837
837	  if (t == error_mark_node
(gdb) n
829	{
(gdb) n
838	      || (! TYPE_P (t)
(gdb) n
839		  && (TREE_TYPE (t) == 0 || TREE_TYPE (t) == error_mark_node)))
(gdb) n
851	      STRIP_NOPS (t);
(gdb) n
852	      set = lang_hooks.get_alias_set (t);

note the execution jumps back to line 829, that is code from the prologue which
is subject to code motion, I wanted to avoid that, which would affect pretty
much every function.  I tried to break at main and the code just twice to the
prologue:

(gdb) b main
Breakpoint 2 at 0x893e10: main. (2 locations)
(gdb) r
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: /home/ed/gnu/install/libexec/gcc/x86_64-pc-linux-gnu/10.0.0/cc1plus -quiet -imultiarch x86_64-linux-gnu -D_GNU_SOURCE next-inline2.cc -quiet -dumpbase next-inline2.cc -mtune=generic -march=x86-64 -auxbase next-inline2 -o next-inline2.s

Breakpoint 2, main (argc=15, argv=0x7fffffffdd78)
    at ../../gcc-trunk/gcc/main.c:36
36	  toplev toplev (NULL, /* external_timer */
(gdb) n
37			 true /* init_signals */);
(gdb) n
35	{
(gdb) n
37			 true /* init_signals */);
(gdb) n
35	{
(gdb) n
37			 true /* init_signals */);
(gdb) n
39	  return toplev.main (argc, argv);



I think this patch could be a base for a slightly better
heuristic, that does not depend on the inline function
to be in a different file, but uses end-PC values from
the inlined_subroutine ranges.

I could imagine that if a is_stmt location is at the same PC
value where a subroutine range ends, and it is followed
by one or more !is_stmt location at the same PC then
the last of these should replace the is_stmt which is likely
part if the inline routine.

I believe the real fix would be to annotate the line number
program with a reference to the inlined subroutine that
the line number refers to.


Thanks
Bernd.


> I wont run the full analysis again, but I mentioned previously that I
> tried GCC 7.4.0.  The debug information generated here was not as
> sophisticated, but nor was it (I think) so wrong.  The debug range for
> the first inlined subroutine entry makes much more sense, there's no
> longer the silly 'start == end' thing going on, and the line table is
> much simpler.  Consequently the debug experience, though not perfect,
> is much better than on any of the later compiler - and this is with my
> is_stmt supporting GDB.
> 
> I've included below the is_stmt support patch as I currently have it,
> but please remember, this is a very quick hack just so anyone
> interested can test it.  I'm going to try and clean it up and repost
> it maybe tomorrow if I have time.
> 
> Thanks,
> Andrew
> 
> --
> 
> ## START: next-inline2.cc ##
> 
> /* This testcase is part of GDB, the GNU debugger.
> 
>    Copyright 2019 Free Software Foundation, Inc.
> 
>    This program is free software; you can redistribute it and/or modify
>    it under the terms of the GNU General Public License as published by
>    the Free Software Foundation; either version 3 of the License, or
>    (at your option) any later version.
> 
>    This program is distributed in the hope that it will be useful,
>    but WITHOUT ANY WARRANTY; without even the implied warranty of
>    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>    GNU General Public License for more details.
> 
>    You should have received a copy of the GNU General Public License
>    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> 
> #include <stdlib.h>
> 
> struct tree
> {
>   volatile int x;
>   volatile int z;
> };
> 
> #define TREE_TYPE(NODE) (*tree_check (NODE, 0))
> 
> inline tree *
> tree_check (tree *t, int i)
> {
>   if (t->x != i)
>     abort();
>   tree *x = t;
>   return x;
> }
> 
> int __attribute__((noinline, noclone))
> get_alias_set (tree *t)
> {
>   if (t != NULL
>       && TREE_TYPE (t).z != 1
>       && TREE_TYPE (t).z != 2
>       && TREE_TYPE (t).z != 3)
>     return 0;
>   return 1;
> }
> 
> tree xx;
> 
> int main()
> {
>   get_alias_set (&xx);
>   abort ();
> }
> 
> ## END ##
> 
> ## START: is_stmt patch ##
> 
> diff --git a/gdb/buildsym-legacy.c b/gdb/buildsym-legacy.c
> index 91f8eb869b7..1a1a856ea37 100644
> --- a/gdb/buildsym-legacy.c
> +++ b/gdb/buildsym-legacy.c
> @@ -252,7 +252,7 @@ void
>  record_line (struct subfile *subfile, int line, CORE_ADDR pc)
>  {
>    gdb_assert (buildsym_compunit != nullptr);
> -  buildsym_compunit->record_line (subfile, line, pc);
> +  buildsym_compunit->record_line (subfile, line, pc, 1);
>  }
>  
>  /* Start a new symtab for a new source file in OBJFILE.  Called, for example,
> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> index df745080817..e6b7e0ede2a 100644
> --- a/gdb/buildsym.c
> +++ b/gdb/buildsym.c
> @@ -666,7 +666,7 @@ buildsym_compunit::pop_subfile ()
>  
>  void
>  buildsym_compunit::record_line (struct subfile *subfile, int line,
> -				CORE_ADDR pc)
> +				CORE_ADDR pc, bool is_stmt)
>  {
>    struct linetable_entry *e;
>  
> @@ -687,6 +687,17 @@ buildsym_compunit::record_line (struct subfile *subfile, int line,
>        m_have_line_numbers = true;
>      }
>  
> +  /* If we have a duplicate for the previous entry then ignore the new
> +     entry, except, if the new entry is setting the is_stmt flag, then
> +     ensure the previous entry respects the new setting.  */
> +  e = subfile->line_vector->item + subfile->line_vector->nitems - 1;
> +  if (e->line == line && e->pc == pc)
> +    {
> +      if (is_stmt && !e->is_stmt)
> +	e->is_stmt = 1;
> +      return;
> +    }
> +
>    if (subfile->line_vector->nitems + 1 >= subfile->line_vector_length)
>      {
>        subfile->line_vector_length *= 2;
> @@ -722,6 +733,7 @@ buildsym_compunit::record_line (struct subfile *subfile, int line,
>  
>    e = subfile->line_vector->item + subfile->line_vector->nitems++;
>    e->line = line;
> +  e->is_stmt = is_stmt ? 1 : 0;
>    e->pc = pc;
>  }
>  
> diff --git a/gdb/buildsym.h b/gdb/buildsym.h
> index 193832fc37a..176c56dcbbb 100644
> --- a/gdb/buildsym.h
> +++ b/gdb/buildsym.h
> @@ -187,7 +187,8 @@ struct buildsym_compunit
>  
>    const char *pop_subfile ();
>  
> -  void record_line (struct subfile *subfile, int line, CORE_ADDR pc);
> +  void record_line (struct subfile *subfile, int line, CORE_ADDR pc,
> +		    bool is_stmt);
>  
>    struct compunit_symtab *get_compunit_symtab ()
>    {
> diff --git a/gdb/disasm.c b/gdb/disasm.c
> index 164939b5882..4eb1360de08 100644
> --- a/gdb/disasm.c
> +++ b/gdb/disasm.c
> @@ -376,6 +376,9 @@ do_mixed_source_and_assembly_deprecated
>        if (le[i].line == le[i + 1].line && le[i].pc == le[i + 1].pc)
>  	continue;		/* Ignore duplicates.  */
>  
> +      if (le[i].is_stmt == 0)
> +	continue;		/* Ignore non-statements.  */
> +
>        /* Skip any end-of-function markers.  */
>        if (le[i].line == 0)
>  	continue;
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 819d37c9b8e..96d9763147c 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -21267,7 +21267,7 @@ dwarf_record_line_p (struct dwarf2_cu *cu,
>  
>  static void
>  dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile,
> -		     unsigned int line, CORE_ADDR address,
> +		     unsigned int line, CORE_ADDR address, bool is_stmt,
>  		     struct dwarf2_cu *cu)
>  {
>    CORE_ADDR addr = gdbarch_addr_bits_remove (gdbarch, address);
> @@ -21281,7 +21281,7 @@ dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile,
>      }
>  
>    if (cu != nullptr)
> -    cu->get_builder ()->record_line (subfile, line, addr);
> +    cu->get_builder ()->record_line (subfile, line, addr, is_stmt);
>  }
>  
>  /* Subroutine of dwarf_decode_lines_1 to simplify it.
> @@ -21304,7 +21304,7 @@ dwarf_finish_line (struct gdbarch *gdbarch, struct subfile *subfile,
>  			  paddress (gdbarch, address));
>      }
>  
> -  dwarf_record_line_1 (gdbarch, subfile, 0, address, cu);
> +  dwarf_record_line_1 (gdbarch, subfile, 0, address, 1, cu);
>  }
>  
>  void
> @@ -21330,7 +21330,7 @@ lnp_state_machine::record_line (bool end_sequence)
>    else if (m_op_index == 0 || end_sequence)
>      {
>        fe->included_p = 1;
> -      if (m_record_lines_p && (producer_is_codewarrior (m_cu) || m_is_stmt))
> +      if (m_record_lines_p)
>  	{
>  	  if (m_last_subfile != m_cu->get_builder ()->get_current_subfile ()
>  	      || end_sequence)
> @@ -21341,6 +21341,8 @@ lnp_state_machine::record_line (bool end_sequence)
>  
>  	  if (!end_sequence)
>  	    {
> +	      bool is_stmt = producer_is_codewarrior (m_cu) || m_is_stmt;
> +
>  	      if (dwarf_record_line_p (m_cu, m_line, m_last_line,
>  				       m_line_has_non_zero_discriminator,
>  				       m_last_subfile))
> @@ -21348,7 +21350,7 @@ lnp_state_machine::record_line (bool end_sequence)
>  		  buildsym_compunit *builder = m_cu->get_builder ();
>  		  dwarf_record_line_1 (m_gdbarch,
>  				       builder->get_current_subfile (),
> -				       m_line, m_address,
> +				       m_line, m_address, is_stmt,
>  				       m_currently_recording_lines ? m_cu : nullptr);
>  		}
>  	      m_last_subfile = m_cu->get_builder ()->get_current_subfile ();
> diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
> index 3b4848b3a45..ef2661702f0 100644
> --- a/gdb/inline-frame.c
> +++ b/gdb/inline-frame.c
> @@ -464,7 +464,7 @@ void
>  _initialize_inline_frame (void)
>  {
>    add_cmd ("inline-frames", class_maintenance, maint_info_inline_frames_cmd,
> -	   _("Info about inline frames at the current location"),
> +	   _("Info about inline frames at the current location."),
>  	   &maintenanceinfolist);
>  }
>  
> diff --git a/gdb/jit.c b/gdb/jit.c
> index 2cfea4561b3..316c440e013 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -578,6 +578,7 @@ jit_symtab_line_mapping_add_impl (struct gdb_symbol_callbacks *cb,
>      {
>        stab->linetable->item[i].pc = (CORE_ADDR) map[i].pc;
>        stab->linetable->item[i].line = map[i].line;
> +      stab->linetable->item[i].is_stmt = 1;
>      }
>  }
>  
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 459d0da8402..6e45a993645 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -717,7 +717,16 @@ btrace_find_line_range (CORE_ADDR pc)
>    range = btrace_mk_line_range (symtab, 0, 0);
>    for (i = 0; i < nlines - 1; i++)
>      {
> -      if ((lines[i].pc == pc) && (lines[i].line != 0))
> +      /* The test of is_stmt here was added when the is_stmt field was
> +	 introduced to the 'struct linetable_entry' structure.  This
> +	 ensured that this loop maintained the same behaviour as before we
> +	 introduced is_stmt.  That said, it might be that we would be
> +	 better off not checking is_stmt here, this would lead to us
> +	 possibly adding more line numbers to the range.  At the time this
> +	 change was made I was unsure how to test this so chose to go with
> +	 maintaining the existing experience.  */
> +      if ((lines[i].pc == pc) && (lines[i].line != 0)
> +	  && (lines[i].is_stmt == 1))

this condition can never be true, since is_stmt is
either 0, or -1.

>  	range = btrace_line_range_add (range, lines[i].line);
>      }
>  
> diff --git a/gdb/symmisc.c b/gdb/symmisc.c
> index 69d035dc843..b173c7aaca1 100644
> --- a/gdb/symmisc.c
> +++ b/gdb/symmisc.c
> @@ -306,6 +306,8 @@ dump_symtab_1 (struct symtab *symtab, struct ui_file *outfile)
>  	{
>  	  fprintf_filtered (outfile, " line %d at ", l->item[i].line);
>  	  fputs_filtered (paddress (gdbarch, l->item[i].pc), outfile);
> +	  if (l->item[i].is_stmt)
> +	    fprintf_filtered (outfile, "\t(stmt)");
>  	  fprintf_filtered (outfile, "\n");
>  	}
>      }
> @@ -992,16 +994,17 @@ maintenance_print_one_line_table (struct symtab *symtab, void *data)
>  
>        /* Leave space for 6 digits of index and line number.  After that the
>  	 tables will just not format as well.  */
> -      printf_filtered (_("%-6s %6s %s\n"),
> -		       _("INDEX"), _("LINE"), _("ADDRESS"));
> +      printf_filtered (_("%-6s %6s %s %s\n"),
> +		       _("INDEX"), _("LINE"), _("ADDRESS"), _("IS-STMT"));
>  
>        for (i = 0; i < linetable->nitems; ++i)
>  	{
>  	  struct linetable_entry *item;
>  
>  	  item = &linetable->item [i];
> -	  printf_filtered (_("%-6d %6d %s\n"), i, item->line,
> -			   core_addr_to_string (item->pc));
> +	  printf_filtered (_("%-6d %6d %s%s\n"), i, item->line,
> +			   core_addr_to_string (item->pc),
> +			   (item->is_stmt ? " Y" : ""));
>  	}
>      }
>  
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 099e92070a8..5f90bad30f8 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -3236,6 +3236,16 @@ find_pc_sect_line (CORE_ADDR pc, struct obj_section *section, int notcurrent)
>  	  best = prev;
>  	  best_symtab = iter_s;
>  
> +	  if (!best->is_stmt)
> +	    {
> +	      struct linetable_entry *tmp = best;
> +	      while (tmp > first && (tmp - 1)->pc == tmp->pc
> +		     && (tmp - 1)->line != 0 && !tmp->is_stmt)
> +		--tmp;
> +	      if (tmp->is_stmt)
> +		best = tmp;
> +	    }
> +
>  	  /* Discard BEST_END if it's before the PC of the current BEST.  */
>  	  if (best_end <= best->pc)
>  	    best_end = 0;
> @@ -3434,7 +3444,8 @@ find_pcs_for_symtab_line (struct symtab *symtab, int line,
>  	{
>  	  struct linetable_entry *item = &SYMTAB_LINETABLE (symtab)->item[idx];
>  
> -	  if (*best_item == NULL || item->line < (*best_item)->line)
> +	  if (*best_item == NULL
> +	      || (item->line < (*best_item)->line && item->is_stmt))
>  	    *best_item = item;
>  
>  	  break;
> @@ -3545,6 +3556,10 @@ find_line_common (struct linetable *l, int lineno,
>      {
>        struct linetable_entry *item = &(l->item[i]);
>  
> +      /* Ignore non-statements.  */
> +      if (!item->is_stmt)
> +	continue;
> +
>        if (item->line == lineno)
>  	{
>  	  /* Return the first (lowest address) entry which matches.  */
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index a1682969355..89eb9ad5bcc 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -1277,7 +1277,13 @@ struct rust_vtable_symbol : public symbol
>  
>  struct linetable_entry
>  {
> +  /* The line number for this entry.  */
>    int line;
> +
> +  /* True if this PC is a good location to place a breakpoint for LINE.  */
> +  int is_stmt : 1;

I think this should be unsinged int is_stmt : 1,
or bool.

> +
> +  /* The address for this entry.  */
>    CORE_ADDR pc;
>  };
>  
> diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
> index eaa77fd4915..9028ff4cb9b 100644
> --- a/gdb/xcoffread.c
> +++ b/gdb/xcoffread.c
> @@ -431,6 +431,9 @@ arrange_linetable (struct linetable *oldLineTb)
>  
>    for (function_count = 0, ii = 0; ii < oldLineTb->nitems; ++ii)
>      {
> +      if (oldLineTb->item[ii].is_stmt == 0)
> +	continue;
> +
>        if (oldLineTb->item[ii].line == 0)
>  	{			/* Function entry found.  */
>  	  if (function_count >= fentry_size)
> @@ -441,6 +444,7 @@ arrange_linetable (struct linetable *oldLineTb)
>  			  fentry_size * sizeof (struct linetable_entry));
>  	    }
>  	  fentry[function_count].line = ii;
> +	  fentry[function_count].is_stmt = 1;
>  	  fentry[function_count].pc = oldLineTb->item[ii].pc;
>  	  ++function_count;
>  
> ## END: is_stmt patch ##
>
diff mbox

Patch

diff --git a/gdb/buildsym-legacy.c b/gdb/buildsym-legacy.c
index 91f8eb869b7..1a1a856ea37 100644
--- a/gdb/buildsym-legacy.c
+++ b/gdb/buildsym-legacy.c
@@ -252,7 +252,7 @@  void
 record_line (struct subfile *subfile, int line, CORE_ADDR pc)
 {
   gdb_assert (buildsym_compunit != nullptr);
-  buildsym_compunit->record_line (subfile, line, pc);
+  buildsym_compunit->record_line (subfile, line, pc, 1);
 }
 
 /* Start a new symtab for a new source file in OBJFILE.  Called, for example,
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index df745080817..e6b7e0ede2a 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -666,7 +666,7 @@  buildsym_compunit::pop_subfile ()
 
 void
 buildsym_compunit::record_line (struct subfile *subfile, int line,
-				CORE_ADDR pc)
+				CORE_ADDR pc, bool is_stmt)
 {
   struct linetable_entry *e;
 
@@ -687,6 +687,17 @@  buildsym_compunit::record_line (struct subfile *subfile, int line,
       m_have_line_numbers = true;
     }
 
+  /* If we have a duplicate for the previous entry then ignore the new
+     entry, except, if the new entry is setting the is_stmt flag, then
+     ensure the previous entry respects the new setting.  */
+  e = subfile->line_vector->item + subfile->line_vector->nitems - 1;
+  if (e->line == line && e->pc == pc)
+    {
+      if (is_stmt && !e->is_stmt)
+	e->is_stmt = 1;
+      return;
+    }
+
   if (subfile->line_vector->nitems + 1 >= subfile->line_vector_length)
     {
       subfile->line_vector_length *= 2;
@@ -722,6 +733,7 @@  buildsym_compunit::record_line (struct subfile *subfile, int line,
 
   e = subfile->line_vector->item + subfile->line_vector->nitems++;
   e->line = line;
+  e->is_stmt = is_stmt ? 1 : 0;
   e->pc = pc;
 }
 
diff --git a/gdb/buildsym.h b/gdb/buildsym.h
index 193832fc37a..176c56dcbbb 100644
--- a/gdb/buildsym.h
+++ b/gdb/buildsym.h
@@ -187,7 +187,8 @@  struct buildsym_compunit
 
   const char *pop_subfile ();
 
-  void record_line (struct subfile *subfile, int line, CORE_ADDR pc);
+  void record_line (struct subfile *subfile, int line, CORE_ADDR pc,
+		    bool is_stmt);
 
   struct compunit_symtab *get_compunit_symtab ()
   {
diff --git a/gdb/disasm.c b/gdb/disasm.c
index 164939b5882..4eb1360de08 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -376,6 +376,9 @@  do_mixed_source_and_assembly_deprecated
       if (le[i].line == le[i + 1].line && le[i].pc == le[i + 1].pc)
 	continue;		/* Ignore duplicates.  */
 
+      if (le[i].is_stmt == 0)
+	continue;		/* Ignore non-statements.  */
+
       /* Skip any end-of-function markers.  */
       if (le[i].line == 0)
 	continue;
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 819d37c9b8e..96d9763147c 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -21267,7 +21267,7 @@  dwarf_record_line_p (struct dwarf2_cu *cu,
 
 static void
 dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile,
-		     unsigned int line, CORE_ADDR address,
+		     unsigned int line, CORE_ADDR address, bool is_stmt,
 		     struct dwarf2_cu *cu)
 {
   CORE_ADDR addr = gdbarch_addr_bits_remove (gdbarch, address);
@@ -21281,7 +21281,7 @@  dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile,
     }
 
   if (cu != nullptr)
-    cu->get_builder ()->record_line (subfile, line, addr);
+    cu->get_builder ()->record_line (subfile, line, addr, is_stmt);
 }
 
 /* Subroutine of dwarf_decode_lines_1 to simplify it.
@@ -21304,7 +21304,7 @@  dwarf_finish_line (struct gdbarch *gdbarch, struct subfile *subfile,
 			  paddress (gdbarch, address));
     }
 
-  dwarf_record_line_1 (gdbarch, subfile, 0, address, cu);
+  dwarf_record_line_1 (gdbarch, subfile, 0, address, 1, cu);
 }
 
 void
@@ -21330,7 +21330,7 @@  lnp_state_machine::record_line (bool end_sequence)
   else if (m_op_index == 0 || end_sequence)
     {
       fe->included_p = 1;
-      if (m_record_lines_p && (producer_is_codewarrior (m_cu) || m_is_stmt))
+      if (m_record_lines_p)
 	{
 	  if (m_last_subfile != m_cu->get_builder ()->get_current_subfile ()
 	      || end_sequence)
@@ -21341,6 +21341,8 @@  lnp_state_machine::record_line (bool end_sequence)
 
 	  if (!end_sequence)
 	    {
+	      bool is_stmt = producer_is_codewarrior (m_cu) || m_is_stmt;
+
 	      if (dwarf_record_line_p (m_cu, m_line, m_last_line,
 				       m_line_has_non_zero_discriminator,
 				       m_last_subfile))
@@ -21348,7 +21350,7 @@  lnp_state_machine::record_line (bool end_sequence)
 		  buildsym_compunit *builder = m_cu->get_builder ();
 		  dwarf_record_line_1 (m_gdbarch,
 				       builder->get_current_subfile (),
-				       m_line, m_address,
+				       m_line, m_address, is_stmt,
 				       m_currently_recording_lines ? m_cu : nullptr);
 		}
 	      m_last_subfile = m_cu->get_builder ()->get_current_subfile ();
diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
index 3b4848b3a45..ef2661702f0 100644
--- a/gdb/inline-frame.c
+++ b/gdb/inline-frame.c
@@ -464,7 +464,7 @@  void
 _initialize_inline_frame (void)
 {
   add_cmd ("inline-frames", class_maintenance, maint_info_inline_frames_cmd,
-	   _("Info about inline frames at the current location"),
+	   _("Info about inline frames at the current location."),
 	   &maintenanceinfolist);
 }
 
diff --git a/gdb/jit.c b/gdb/jit.c
index 2cfea4561b3..316c440e013 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -578,6 +578,7 @@  jit_symtab_line_mapping_add_impl (struct gdb_symbol_callbacks *cb,
     {
       stab->linetable->item[i].pc = (CORE_ADDR) map[i].pc;
       stab->linetable->item[i].line = map[i].line;
+      stab->linetable->item[i].is_stmt = 1;
     }
 }
 
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 459d0da8402..6e45a993645 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -717,7 +717,16 @@  btrace_find_line_range (CORE_ADDR pc)
   range = btrace_mk_line_range (symtab, 0, 0);
   for (i = 0; i < nlines - 1; i++)
     {
-      if ((lines[i].pc == pc) && (lines[i].line != 0))
+      /* The test of is_stmt here was added when the is_stmt field was
+	 introduced to the 'struct linetable_entry' structure.  This
+	 ensured that this loop maintained the same behaviour as before we
+	 introduced is_stmt.  That said, it might be that we would be
+	 better off not checking is_stmt here, this would lead to us
+	 possibly adding more line numbers to the range.  At the time this
+	 change was made I was unsure how to test this so chose to go with
+	 maintaining the existing experience.  */
+      if ((lines[i].pc == pc) && (lines[i].line != 0)
+	  && (lines[i].is_stmt == 1))
 	range = btrace_line_range_add (range, lines[i].line);
     }
 
diff --git a/gdb/symmisc.c b/gdb/symmisc.c
index 69d035dc843..b173c7aaca1 100644
--- a/gdb/symmisc.c
+++ b/gdb/symmisc.c
@@ -306,6 +306,8 @@  dump_symtab_1 (struct symtab *symtab, struct ui_file *outfile)
 	{
 	  fprintf_filtered (outfile, " line %d at ", l->item[i].line);
 	  fputs_filtered (paddress (gdbarch, l->item[i].pc), outfile);
+	  if (l->item[i].is_stmt)
+	    fprintf_filtered (outfile, "\t(stmt)");
 	  fprintf_filtered (outfile, "\n");
 	}
     }
@@ -992,16 +994,17 @@  maintenance_print_one_line_table (struct symtab *symtab, void *data)
 
       /* Leave space for 6 digits of index and line number.  After that the
 	 tables will just not format as well.  */
-      printf_filtered (_("%-6s %6s %s\n"),
-		       _("INDEX"), _("LINE"), _("ADDRESS"));
+      printf_filtered (_("%-6s %6s %s %s\n"),
+		       _("INDEX"), _("LINE"), _("ADDRESS"), _("IS-STMT"));
 
       for (i = 0; i < linetable->nitems; ++i)
 	{
 	  struct linetable_entry *item;
 
 	  item = &linetable->item [i];
-	  printf_filtered (_("%-6d %6d %s\n"), i, item->line,
-			   core_addr_to_string (item->pc));
+	  printf_filtered (_("%-6d %6d %s%s\n"), i, item->line,
+			   core_addr_to_string (item->pc),
+			   (item->is_stmt ? " Y" : ""));
 	}
     }
 
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 099e92070a8..5f90bad30f8 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3236,6 +3236,16 @@  find_pc_sect_line (CORE_ADDR pc, struct obj_section *section, int notcurrent)
 	  best = prev;
 	  best_symtab = iter_s;
 
+	  if (!best->is_stmt)
+	    {
+	      struct linetable_entry *tmp = best;
+	      while (tmp > first && (tmp - 1)->pc == tmp->pc
+		     && (tmp - 1)->line != 0 && !tmp->is_stmt)
+		--tmp;
+	      if (tmp->is_stmt)
+		best = tmp;
+	    }
+
 	  /* Discard BEST_END if it's before the PC of the current BEST.  */
 	  if (best_end <= best->pc)
 	    best_end = 0;
@@ -3434,7 +3444,8 @@  find_pcs_for_symtab_line (struct symtab *symtab, int line,
 	{
 	  struct linetable_entry *item = &SYMTAB_LINETABLE (symtab)->item[idx];
 
-	  if (*best_item == NULL || item->line < (*best_item)->line)
+	  if (*best_item == NULL
+	      || (item->line < (*best_item)->line && item->is_stmt))
 	    *best_item = item;
 
 	  break;
@@ -3545,6 +3556,10 @@  find_line_common (struct linetable *l, int lineno,
     {
       struct linetable_entry *item = &(l->item[i]);
 
+      /* Ignore non-statements.  */
+      if (!item->is_stmt)
+	continue;
+
       if (item->line == lineno)
 	{
 	  /* Return the first (lowest address) entry which matches.  */
diff --git a/gdb/symtab.h b/gdb/symtab.h
index a1682969355..89eb9ad5bcc 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1277,7 +1277,13 @@  struct rust_vtable_symbol : public symbol
 
 struct linetable_entry
 {
+  /* The line number for this entry.  */
   int line;
+
+  /* True if this PC is a good location to place a breakpoint for LINE.  */
+  int is_stmt : 1;
+
+  /* The address for this entry.  */
   CORE_ADDR pc;
 };
 
diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index eaa77fd4915..9028ff4cb9b 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -431,6 +431,9 @@  arrange_linetable (struct linetable *oldLineTb)
 
   for (function_count = 0, ii = 0; ii < oldLineTb->nitems; ++ii)
     {
+      if (oldLineTb->item[ii].is_stmt == 0)
+	continue;
+
       if (oldLineTb->item[ii].line == 0)
 	{			/* Function entry found.  */
 	  if (function_count >= fentry_size)
@@ -441,6 +444,7 @@  arrange_linetable (struct linetable *oldLineTb)
 			  fentry_size * sizeof (struct linetable_entry));
 	    }
 	  fentry[function_count].line = ii;
+	  fentry[function_count].is_stmt = 1;
 	  fentry[function_count].pc = oldLineTb->item[ii].pc;
 	  ++function_count;