Re: [PATCH] Fix an issue with the gdb step-over aka. "n" command

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

[-- Attachment #1: Type: text/plain, Size: 4081 bytes --]

On 12/20/19 7:13 AM, Simon Marchi wrote:
> Hi Bernd,
> 
> On 2019-12-19 5:53 p.m., Bernd Edlinger wrote:
>> Does this explanation make sense?
> 
> Yes.  Well I think so, I have to admit this is a bit over my head,
> there are a lot of pieces to put together to have a good understanding
> of the problem.  I just did a first read, I'll sleep on it and come
> back to it later.
> 
> Thanks for the small reproducer, this is extremely valuable.  I think it
> will be a good idea to integrate it as a test case in the test suite.
> 
> In your patch to dwarf2read.c, I was a bit surprised to see:
> 
>   m_last_subfile != m_cu->get_builder ()->get_current_subfile ()
> 
> So your fix only works if the inlined subroutine comes from another file?  If
> I move the tree_check function in next-inline.cc, the fix no longer applies,
> and we get the broken behavior.  From your previous email, I understand that
> this is expected.  I guess that if both are in the same file, we can't detect
> this situation using the same technique.

Yes, when the inline function is not in a header file that will not help.
But it solves 90% of the problem with a simple and obvious heuristic.

To attack the rest of the problem we would need to know the PCs where inlined
subroutines and each corresponding range infos do end, but that data is
only available long after the line info is parsed.

> 
> I also read about location views, since that's what Alexandre referred to.  It
> sounds like it's a magic solution that will allow GDB to do the right thing in
> this kind of situation.  If that's indeed the case, then it might be good to start
> exploring this path.  I'd like to have a better understanding of how this will help
> GDB make a smarter "next", and what kind of effort is needed to make GDB use it.  My
> understanding is that location views allow having an address mapped to multiple
> source locations.  For example, here's the problematic address in the next-inline
> test case I've compiled:
> 
>   ./next-inline.h:[++]
>   next-inline.h                                 28              0x1175               x
>   next-inline.h                                 30              0x1175       1       x
> 
>   ./next-inline.cc:[++]
>   next-inline.cc                                22              0x1175       2
> 

I think the main problem here, is that from the line numbers alone it is not clear which
of these location infos is in the subroutine and which is in the caller.
The only link is the program address which is ambiguous at the end of the inline
block.

So my impression is that we need a connection between the location views and the
inlined subroutine info.

> Today, when I ask GDB "which source line does this address correspond to", it gives me
> one answer.  Does this mean that GDB will now say that 0x1175 corresponds to
> 
> - next-inline.h:28
> - next-inline.h:30
> - next-inline.cc:22
> 
> all at the same time?  Is one of these source locations more important than the others?
> If execution happens to stop exactly at this address, which location do we present to
> the user?
> 

No idea. That will likely be confusing.

> And to come back the problem at hand, how does this help GDB make a smarter "next"?
> 
> Btw, I stumbled on a bug with the TUI source display.  It might be caused by this patch,
> or it might be that this patch uncovers it.
> 
> When I do these actions:
> 
> - Start GDB with the next-inline test file (from this patch)
> - Enable the TUI
> - Type "start"
> - Type "s"
> - Type "n" twice
> 
> The TUI source display wrongfully jumps to the header file, line 24.
> When I type "frame", it says I'm stopped at next-line.cc:24.  So it
> is showing the right line number of the wrong file.
> 

Ah, yes.  That is already preexistent.
Consider the attached idea for a test case.
I have no idea yet how to make a working test case
out if it.
But I can fix the tui bug, it is quite easy.
I will send a patch for that in a moment.


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: step.c --]
[-- Type: text/x-csrc; name="step.c", Size: 917 bytes --]

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

__attribute__((noinline, noclone)) static void
bar ()
{
}

#include "step.h"

int
main ()
{
  int x;

  x = 0;
  foo ();
  abort ();
  return x;
}

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: step.h --]
[-- Type: text/x-chdr; name="step.h", Size: 827 bytes --]

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

__attribute__((__always_inline__)) static inline int
foo (void)
{
  bar ();
}