public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Bruno Larsen <blarsen@redhat.com>
To: Pedro Alves <pedro@palves.net>, Carl Love <cel@us.ibm.com>,
	Ulrich Weigand <Ulrich.Weigand@de.ibm.com>,
	"will_schmidt@vnet.ibm.com" <will_schmidt@vnet.ibm.com>,
	gdb-patches@sourceware.org
Subject: Re: [PATCH 1/2 version 3] fix for gdb.reverse/finish-precsave.exp and gdb.reverse/finish-reverse.exp
Date: Tue, 24 Jan 2023 17:04:40 +0100	[thread overview]
Message-ID: <837e302b-3ac3-67bd-5314-55194ab35421@redhat.com> (raw)
In-Reply-To: <d2960257-ff49-aa8b-c5d8-0dc0cae0fb80@palves.net>

On 24/01/2023 16:06, Pedro Alves wrote:
> On 2023-01-24 2:23 p.m., Bruno Larsen wrote:
>> On 24/01/2023 15:08, Pedro Alves wrote:
>>> On 2023-01-23 9:13 p.m., Carl Love wrote:
>>>> Pedro:
>>>>
>>>> On Mon, 2023-01-23 at 19:17 +0000, Pedro Alves wrote:
>>>>>> Currently on X86, when executing the finish command in reverse, gdb
>>>>>> does a
>>>>>> single step from the first instruction in the callee to get back to
>>>>>> the
>>>>>> caller.  GDB stops on the last instruction in the source code line
>>>>>> where
>>>>>> the call was made.  When stopped at the last instruction of the
>>>>>> source code
>>>>>> line, a reverse next or step command will stop at the first
>>>>>> instruction
>>>>>> of the same source code line thus requiring two step/next commands
>>>>>> to
>>>>>> reach the previous source code line.  It should only require one
>>>>>> step/next
>>>>>> command to reach the previous source code line.
>>>>>>
>>>>>> By contrast, a reverse next or step command from the first line in
>>>>>> a
>>>>>> function stops at the first instruction in the source code line
>>>>>> where the
>>>>>> call was made.
>>>>> I'd think this was on purpose.  Note that next/step/reverse-
>>>>> {next/step} are line-oriented
>>>>> stepping commands, they step/next until the previous/next line.
>>>>> While "finish" is described
>>>>> as undoing the _function call_.
>>>>>
>>>>> The manual says:
>>>>>
>>>>>    reverse-finish
>>>>>    Just as the finish command takes you to the point where the current
>>>>> function returns,
>>>>>    reverse-finish takes you to the point where it was called. Instead
>>>>> of ending up at the end of
>>>>>    the current function invocation, you end up at the beginning.
>>>>>
>>>>> Say you have a line with multiple statements involving multiple
>>>>> function calls.
>>>>> The simplest would be:
>>>>>
>>>>>     func1 ();  func2 ();
>>>>>
>>>>> Say you'd stopped inside 'func2'.  If you do finish there, in current
>>>>> master gdb
>>>>> stops at the call to 'func2', and you can then decide to reverse step
>>>>> into 'func1'.
>>>> I don't think you followed the issue.
>>> Totally possible!
>>>
>>>> So, if you are in func2 and do a reverse-finish, without the patch gdb
>>>> stops on the last instruction for the line that calls func2.
>>> Right.
>>>
>>>> Now if
>>>> you issue a reverse-step, you stop at the first instruction for the
>>>> call to func2, i.e. you are still on the same source code line.
>>> Wait.  That right there sounds bogus.  The source line looks like:
>>>
>>>      func1 ();  func2 ();
>>>
>>> so stepping backwards over that line should always stop at the first
>>> instruction of the line, not in the middle.  Let's simplify this.
>>>
>>> Here's the full source code of my example:
>>>
>>> (gdb) list 1
>>> 1       void func1 ()
>>> 2       {
>>> 3       }
>>> 4
>>> 5       void func2 ()
>>> 6       {
>>> 7       }
>>> 8
>>> 9       int main ()
>>> 10      {
>>> 11        func1 (); func2 ();
>>> 12      }
>>>
>> I think you are describing a different issue to what Carl is trying to solve. Using your example code, I have the following debugging session:
> No, it's the exact same.  Please re-read.
>
>> $ ./gdb -q reverse
>> Reading symbols from reverse...
>> (gdb) start
>> Temporary breakpoint 1 at 0x401118: file t.c, line 8.
>> Starting program: /home/blarsen/Documents/build/gdb/reverse
>> [Thread debugging using libthread_db enabled]
>> Using host libthread_db library "/lib64/libthread_db.so.1".
>>
>> Temporary breakpoint 1, main () at t.c:8
>> 8           func1(); func2();
>> (gdb) record
>> (gdb) n
>> 9       }
>> (gdb) rs
>> func2 () at t.c:5
>> 5       }
>> (gdb) reverse-finish
>> Run back to call of #0  func2 () at t.c:5
>> 0x0000000000401127 in main () at t.c:8
>> 8           func1(); func2();
>> (gdb) rs
>> 8           func1(); func2();
>> (gdb)
>> func1 () at t.c:2
>> 2       }
>> (gdb)
>>
>> Notice how there were two "reverse-step" commands needed after the "reverse-finish" used to exit func2.
> Yes.  And the reason you need two "reverse-step" is because there are two line ranges for line 8, and reverse-step stops
> at the beginning of the second range instead of at the beginning of the first.  It's the exact same as with my simpler
> example of just doing "next" -> "reverse-next", which I used as a simpler example to explain the problem.

The simplified example is not the exact same use case. Looking at how 
the program executes forward, if we next over this line we skip 
everything, but if we step into func1 and use finish we will still stop 
in line 11 and the user can decide to step into func2 or not. That is 
why I assumed you were thinking of a different issue when I saw you 
using next and reverse-next as examples.

Looking at where GDB lands when finishing from func1 we get (code 
compiled with gcc -gno-column-info):

(gdb) start
Temporary breakpoint 1 at 0x401118: file t.c, line 8.
Starting program: /home/blarsen/Documents/downstream_build/gdb/reverse
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

Temporary breakpoint 1, main () at t.c:8
8           f1(); f2();
(gdb) s
f1 () at t.c:2
2       }
(gdb) finish
Run till exit from #0  f1 () at t.c:2
0x0000000000401122 in main () at t.c:8
8           f1(); f2();
(gdb) disas /s
Dump of assembler code for function main:
t.c:
7       int main(){
    0x0000000000401114 <+0>:     push   %rbp
    0x0000000000401115 <+1>:     mov    %rsp,%rbp

8           f1(); f2();
    0x0000000000401118 <+4>:     mov    $0x0,%eax
    0x000000000040111d <+9>:     call   0x401106 <f1>
=> 0x0000000000401122 <+14>:    mov    $0x0,%eax
    0x0000000000401127 <+19>:    call   0x40110d <f2>
    0x000000000040112c <+24>:    mov    $0x0,%eax

9       }
    0x0000000000401131 <+29>:    pop    %rbp
    0x0000000000401132 <+30>:    ret
End of assembler dump.

The behavior that we should emulate when going backwards is that 
finishing from func2 stops us on the 'mov' instruction between the 
calls. Looking at the clang session that I cut off from the previous email:

Alright, here's the gdb session, with clang, no gdb patch:

  Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdc48) at reverse.c:11
  11        func1 (argc); func2 (argc);
  (gdb) disassemble /s
  Dump of assembler code for function main:
  reverse.c:
  10      {
     0x0000555555555150 <+0>:     push   %rbp
     0x0000555555555151 <+1>:     mov    %rsp,%rbp
     0x0000555555555154 <+4>:     sub    $0x10,%rsp
     0x0000555555555158 <+8>:     mov    %edi,-0x4(%rbp)
     0x000055555555515b <+11>:    mov    %rsi,-0x10(%rbp)
  
  11        func1 (argc); func2 (argc);
  => 0x000055555555515f <+15>:    mov    -0x4(%rbp),%edi
     0x0000555555555162 <+18>:    call   0x555555555130 <func1>
     0x0000555555555167 <+23>:    mov    -0x4(%rbp),%edi
     0x000055555555516a <+26>:    call   0x555555555140 <func2>
  
  12      }
     0x000055555555516f <+31>:    xor    %eax,%eax
     0x0000555555555171 <+33>:    add    $0x10,%rsp
     0x0000555555555175 <+37>:    pop    %rbp
     0x0000555555555176 <+38>:    ret
  End of assembler dump.
  (gdb) record
  (gdb) b func2
  Breakpoint 2 at 0x555555555147: file reverse.c, line 7.
  (gdb) c
  Continuing.
  
  Breakpoint 2, func2 (i=1) at reverse.c:7
  7       }
  (gdb) reverse-finish
  Run back to call of #0  func2 (i=1) at reverse.c:7
  0x000055555555516a in main (argc=1, argv=0x7fffffffdc48) at reverse.c:11
  11        func1 (argc); func2 (argc);
  (gdb) disassemble /s
  Dump of assembler code for function main:
  reverse.c:
  10      {
     0x0000555555555150 <+0>:     push   %rbp
     0x0000555555555151 <+1>:     mov    %rsp,%rbp
     0x0000555555555154 <+4>:     sub    $0x10,%rsp
     0x0000555555555158 <+8>:     mov    %edi,-0x4(%rbp)
     0x000055555555515b <+11>:    mov    %rsi,-0x10(%rbp)
  
  11        func1 (argc); func2 (argc);
     0x000055555555515f <+15>:    mov    -0x4(%rbp),%edi
     0x0000555555555162 <+18>:    call   0x555555555130 <func1>
     0x0000555555555167 <+23>:    mov    -0x4(%rbp),%edi
  => 0x000055555555516a <+26>:    call   0x555555555140 <func2>
  
  12      }
     0x000055555555516f <+31>:    xor    %eax,%eax
     0x0000555555555171 <+33>:    add    $0x10,%rsp
     0x0000555555555175 <+37>:    pop    %rbp
     0x0000555555555176 <+38>:    ret
  End of assembler dump.
  (gdb) reverse-step
  func1 (i=1) at reverse.c:3
  3       }
  (gdb)

We can see that GDB stopped on the call instruction instead. So a user 
that finished from func1 or reverse-finished from func2 may see 
different inferior states.


>
>> What Carl is proposing we do is make it so GDB only needs one command.
> I understand.  However, I am saying that that is papering over the actual problem, _and_ it only works in the situation
> where you ended up with two line entries with is-stmt for the same line.  Note how the patch breaks clang, and gcc with
> -gno-column-info...
>
>> If I compare the $pc of where GDB is stopped and the linetable, we get:
>>
>> (gdb) print $pc
>> $2 = (void (*)()) 0x401127 <main+19>
>> (gdb) maint info line-table
>> objfile: /home/blarsen/Documents/downstream_build/gdb/reverse ((struct objfile *) 0x10f7750)
>> compunit_symtab: t.c ((struct compunit_symtab *) 0x116c330)
>> symtab: /home/blarsen/Documents/downstream_build/gdb/t.c ((struct symtab *) 0x116c3b0)
>> linetable: ((struct linetable *) 0x11aec80):
>> INDEX  LINE   ADDRESS            IS-STMT PROLOGUE-END
>> 0      1      0x0000000000401106 Y
>> 1      2      0x000000000040110a Y
>> 2      4      0x000000000040110d Y
>> 3      5      0x0000000000401111 Y
>> 4      7      0x0000000000401114 Y
>> 5      8      0x0000000000401118 Y
>> 6      8      0x0000000000401122 Y
>> 7      9      0x0000000000401131 Y
>> 8      END    0x0000000000401133 Y
>>
>> We can see that GDB shouldn't even be able to stop at that $pc, it isn't an is_stmt.
> reverse-finish is not supposed to step backwards until it reaches is_stmt.  Doing so makes it
> step backwards too much, as I've shown in my previous example.
>
>> We should have stopped at 0x401122, which is where the first reverse-step stops:
> No...
>
>> (gdb) rs
>> 8           f1(); f2();
>> (gdb) p $pc
>> $4 = (void (*)()) 0x401122 <main+14>
> ... no because in the case where you don't have column debug info (or with clang), there won't be
> an is-stmt entry for the f2 call/column, there will only be one single line entry for the whole of
> line 8, so gdb would step back too much.
>
>> So not only are we needing an extra command to re-sync with user expectations, we are in an instruction where the inferior state might be all over the place.
>>
> What does that mean, the state might be all over the place?  The DWARF should describe the locations of all variables accurately at all instructions.
>
Unrelated to this specific issue, but I was looking at record/30025 
(https://sourceware.org/bugzilla/show_bug.cgi?id=30025) and it confused 
me because because GDB sometimes reported the incorrect parameter, but 
if we continue single-stepping through instructions, GDB eventually 
finds the correct value when we stop at an is_stmt instruction.

If I misunderstood something about 30025 and is_stmt, please let me 
know! but as far as I can see, this is what happened.

-- 
Cheers,
Bruno


  reply	other threads:[~2023-01-24 16:06 UTC|newest]

Thread overview: 105+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <f594ec0070a6c585e83a6d6c8b29481a86778c0f.camel@us.ibm.com>
     [not found] ` <bc6bb459f153c0c5850d4a3e5d80bbf957ec36cc.camel@de.ibm.com>
     [not found]   ` <8bce850fa1e03e798506dc170d9b57f52034a18a.camel@us.ibm.com>
     [not found]     ` <cb5875db4e1ac60475877c685e5f172770314523.camel@de.ibm.com>
     [not found]       ` <adeeeae47c4ca79b32d79aea632ff8b2a24dd93d.camel@us.ibm.com>
     [not found]         ` <86c5e9c47945894f21b1d8bf6089c730a9f0e1a5.camel@de.ibm.com>
     [not found]           ` <b1d7ea600d6bb7af487968d938566fae9d5e1745.camel@us.ibm.com>
     [not found]             ` <5f9047b9582403561d7cce998cab9184167366a1.camel@de.ibm.com>
     [not found]               ` <e7c8093c350ad475277154014a4f0bb9b472b7af.camel@us.ibm.com>
     [not found]                 ` <f8d6379aff7af076d9edcee7d2981d052b2161ee.camel@de.ibm.com>
     [not found]                   ` <5b50668cbe882c57b8c0e9dcf5be0a253713c4c6.camel@us.ibm.com>
     [not found]                     ` <51c4bfc82ac72e475e10577dc60e4d75fa48767e.camel@de.ibm.com>
     [not found]                       ` <3ea97a8aa9cccb39299adde682f92055d1986ab3.camel@us.ibm.com>
     [not found]                         ` <f5ea8da12631f2496ba0e2263e65a0adc7ac56ca.camel@de.ibm.com>
     [not found]                           ` <53878e37c6e57de1d04d9c9960c5d0a74324ee6e.camel@us.ibm.com>
     [not found]                             ` <a5300b64533fdc753c1d50fa0e6efc21b5457547.camel@de.ibm.com>
     [not found]                               ` <50474aa92ba82eff05cdc8f49001eae56be29670.camel@us.ibm.com>
     [not found]                                 ` <f3ef4486c4ba051024602928acdfe5ddf6942b82.camel@de.ibm.com>
     [not found]                                   ` <dae6c9790b23a90d5f1494f5b6798346444f257e.camel@us.ibm.com>
     [not found]                                     ` <89331c26795e3f7743e1e068dce43b3c2dd53008.camel@us.ibm.com>
     [not found]                                       ` <c10a008e441666e4edb0916842d8eefe83f5b2f9.camel@de.ibm.com>
     [not found]                                         ` <071f24ecf9b3a2bbbe8fee7db77492eb55c5f3ff.camel@us.ibm.com>
     [not found]                                           ` <1d9b21914354bef6a290ac30673741e722e11757.camel@de.ibm.com>
2023-01-11 18:27                                             ` [PATCH 0/2] " Carl Love
2023-01-11 18:27                                             ` [PATCH 1/2] " Carl Love
2023-01-12 16:56                                               ` Tom de Vries
2023-01-12 18:54                                                 ` Carl Love
2023-01-13 13:33                                               ` Bruno Larsen
2023-01-13 16:43                                                 ` Carl Love
2023-01-13 17:04                                                   ` Bruno Larsen
2023-01-13 19:10                                                     ` Carl Love
2023-01-14 18:08                                                 ` Carl Love
2023-01-16 12:31                                                   ` Bruno Larsen
2023-01-16 16:37                                                     ` [PATCH 0/2 version 2] " Carl Love
2023-01-16 16:37                                                     ` [PATCH 1/2 " Carl Love
2023-01-17 12:35                                                       ` Bruno Larsen
2023-01-20  0:03                                                         ` [PATCH 1/2 version 3] " Carl Love
2023-01-23 19:17                                                           ` Pedro Alves
2023-01-23 21:13                                                             ` Carl Love
2023-01-24 14:08                                                               ` Pedro Alves
2023-01-24 14:23                                                                 ` Bruno Larsen
2023-01-24 15:06                                                                   ` Pedro Alves
2023-01-24 16:04                                                                     ` Bruno Larsen [this message]
2023-01-24 19:12                                                                       ` Pedro Alves
2023-01-25  9:49                                                                         ` Bruno Larsen
2023-01-25 14:11                                                                         ` Ulrich Weigand
2023-01-25 16:42                                                                           ` Pedro Alves
2023-01-25 17:13                                                                             ` Ulrich Weigand
2023-01-25 17:24                                                                               ` Pedro Alves
2023-01-25 19:38                                                                                 ` Carl Love
2023-01-24 15:51                                                                 ` Carl Love
2023-01-24 18:37                                                                   ` Pedro Alves
2023-01-24 18:25                                                                 ` Carl Love
2023-01-24 19:21                                                                   ` Pedro Alves
2023-01-24 19:26                                                                     ` Pedro Alves
2023-01-31  0:17                                                                 ` Reverse-next bug test case Carl Love
2023-02-01 14:37                                                                   ` Pedro Alves
2023-02-01 18:40                                                                     ` Carl Love
2023-01-24 15:53                                                             ` [PATCH 1/2 version 3] fix for gdb.reverse/finish-precsave.exp and gdb.reverse/finish-reverse.exp Tom de Vries
2023-01-24 18:48                                                               ` Pedro Alves
2023-01-16 16:37                                                     ` [PATCH 2/2 version 2] " Carl Love
2023-01-17 14:29                                                       ` Bruno Larsen
2023-01-17 16:36                                                         ` Carl Love
2023-01-17 16:55                                                           ` Tom de Vries
2023-01-17 17:03                                                             ` Carl Love
2023-01-17 17:14                                                               ` Tom de Vries
2023-01-17 19:31                                                                 ` Carl Love
2023-01-18 10:55                                                                   ` Tom de Vries
2023-01-18 16:16                                                                     ` Carl Love
2023-01-18 22:26                                                                     ` Carl Love
2023-01-19  8:04                                                                       ` Bruno Larsen
2023-01-19 16:56                                                                         ` Carl Love
2023-01-19 23:57                                                                           ` Carl Love
2023-01-20 20:04                                                                             ` Carl Love
2023-01-23 16:42                                                                               ` [PATCH 1/2 version 3] " Carl Love
2023-01-23 16:42                                                                               ` [PATCH 2/2 " Carl Love
2023-02-10 20:55                                                                               ` [PATCH ] PowerPC: " Carl Love
2023-02-17 12:24                                                                                 ` Ulrich Weigand
2023-02-20 16:34                                                                                   ` Carl Love
2023-02-20 16:48                                                                                     ` Bruno Larsen
2023-02-20 20:24                                                                                   ` Carl Love
2023-02-27 16:09                                                                                     ` [PING] " Carl Love
2023-02-28 13:39                                                                                     ` Bruno Larsen
2023-02-28 16:19                                                                                       ` Carl Love
2023-03-01 13:43                                                                                     ` Tom de Vries
2023-03-01 16:26                                                                                       ` Carl Love
2023-03-01 14:03                                                                                     ` Tom de Vries
2023-03-01 16:43                                                                                       ` Carl Love
2023-03-01 14:34                                                                                     ` Tom de Vries
2023-03-01 20:39                                                                                       ` Carl Love
2023-03-01 20:59                                                                                         ` [PATCH 0/2 " Carl Love
2023-03-01 20:59                                                                                         ` [PATCH 1/2] " Carl Love
2023-03-03 11:56                                                                                           ` Bruno Larsen
2023-03-08 16:19                                                                                             ` [PING] " Carl Love
2023-03-09 16:09                                                                                               ` Carl Love
2023-03-09 19:03                                                                                           ` Tom Tromey
2023-03-09 21:42                                                                                             ` Carl Love
2023-03-09 21:54                                                                                             ` [PATCH 1/2 ver 2] " Carl Love
2023-03-10  3:53                                                                                               ` Tom Tromey
2023-03-01 20:59                                                                                         ` [PATCH 2/2 ] " Carl Love
2023-03-08 16:19                                                                                           ` [PING] " Carl Love
2023-03-09 16:09                                                                                             ` Carl Love
2023-03-13 14:16                                                                                           ` Ulrich Weigand
2023-03-13 17:31                                                                                             ` Carl Love
2023-03-13 17:38                                                                                             ` [PATCH 2/2 ver2] " Carl Love
2023-03-17 17:19                                                                                               ` Ulrich Weigand
2023-03-17 23:05                                                                                                 ` Tom de Vries
2023-03-20 15:04                                                                                                   ` Carl Love
2023-03-20 23:21                                                                                                   ` Carl Love
2023-03-21  3:17                                                                                                     ` Carl Love
2023-03-21  6:52                                                                                                       ` Ulrich Weigand
2023-03-24 17:23                                                                                           ` [PATCH 2/2 ] " Simon Marchi
2023-03-24 22:16                                                                                             ` Carl Love
2023-03-25 12:39                                                                                               ` Simon Marchi
2023-03-27 23:59                                                                                                 ` Carl Love
2023-03-28  1:19                                                                                                   ` Simon Marchi
2023-03-28 15:17                                                                                                     ` Carl Love
2023-03-28 15:38                                                                                                       ` Simon Marchi
2023-07-20 12:01                                                                                                         ` Bruno Larsen
2023-07-20 14:45                                                                                                           ` Carl Love
2023-07-21  7:24                                                                                                             ` Bruno Larsen
2023-07-31 22:59                                                                                                               ` Carl Love
2023-08-02  9:29                                                                                                                 ` Bruno Larsen
2023-08-02 15:11                                                                                                                   ` Carl Love
2023-01-13 15:42                                               ` [PATCH 1/2] " Bruno Larsen
2023-01-11 18:27                                             ` [PATCH 2/2] " Carl Love
2023-01-13 15:55                                               ` Bruno Larsen
2023-01-14 18:08                                                 ` Carl Love

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=837e302b-3ac3-67bd-5314-55194ab35421@redhat.com \
    --to=blarsen@redhat.com \
    --cc=Ulrich.Weigand@de.ibm.com \
    --cc=cel@us.ibm.com \
    --cc=gdb-patches@sourceware.org \
    --cc=pedro@palves.net \
    --cc=will_schmidt@vnet.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).