public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Pedro Alves <pedro@palves.net>
To: Bruno Larsen <blarsen@redhat.com>, 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 19:12:01 +0000	[thread overview]
Message-ID: <f05ba1f3-0aed-d6f9-d2a1-02d0c400a967@palves.net> (raw)
In-Reply-To: <837e302b-3ac3-67bd-5314-55194ab35421@redhat.com>


On 2023-01-24 4:04 p.m., Bruno Larsen wrote:
> On 24/01/2023 16:06, Pedro Alves wrote:

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

Correct, but it's the same root cause.  Forget at reverse-finish, that is doing what it says it should do, that is,
to stop at the call to the function.  You can just put a breakpoint at that same instruction, and once there, do a
reverse-step.  GDB should then reverse step until it reaches a different line.  But it does not.  _That_ is the bug.

Vis (with gcc and column info):

 ...
 (gdb) reverse-finish 
 Run back to call of #0  func2 (i=1) at reverse.c:7
 0x0000555555555167 in main (argc=1, argv=0x7fffffffdc58) at reverse.c:11
 ^^^^^^^^^^^^^^^^^^
 11        func1 (argc); func2 (argc);
 (gdb) start
 The program being debugged has been started already.
 Start it from the beginning? (y or n) y
 Temporary breakpoint 2 at 0x555555555158: file reverse.c, line 11.
 Starting program: /home/pedro/reverse 
 [Thread debugging using libthread_db enabled]
 Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
 
 Temporary breakpoint 2, main (argc=1, argv=0x7fffffffdc58) at reverse.c:11
 11        func1 (argc); func2 (argc);
 (gdb) record
 (gdb) b *0x0000555555555167              << manually set the breakpoint where the reverse-finish would stop
 Breakpoint 4 at 0x555555555167: file reverse.c, line 11.
 (gdb) c
 Continuing.

 Breakpoint 4, 0x0000555555555167 in main (argc=1, argv=0x7fffffffdc58) at reverse.c:11
 11        func1 (argc); func2 (argc);    << started at line 11
 (gdb) reverse-step
 11        func1 (argc); func2 (argc);    << stopped at the same line, bug!

See, reverse-step stopped in the same line you started with.  That is a bug, it should never
happen.  Just like forward "step" never steps in the same line.  Because that's how the
commands are supposed to work -- step until the line number changes!

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

What is there are more instructions in between the calls?  How do you know where to stop?

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

Seeing different inferior states is expected, as finish and reverse-finish are not the exact mirror of one another, like step
and reverse-step are.  The exact reversal of finish would be the equivalent of being stopped at a function call return insn after
a "finish" (and the command could only be used while stopped there), and stepping back into the function up until the point
where you had typed "finish" and stopping there.  Obviously impossible to implement.  So we made "reverse-finish" do something
sensible, such as stepping backwards up until the call site.

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

I can't tell what is going on in that bug without debugging it myself, but seeing SIGTRAP makes me suspect that
one thing that could be going on is that GDB didn't apply the decr-pc-after-break adjustment properly, so
then the PC is pointing into the middle of an instruction and then all hell breaks loose.

  reply	other threads:[~2023-01-24 19:12 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
2023-01-24 19:12                                                                       ` Pedro Alves [this message]
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=f05ba1f3-0aed-d6f9-d2a1-02d0c400a967@palves.net \
    --to=pedro@palves.net \
    --cc=Ulrich.Weigand@de.ibm.com \
    --cc=blarsen@redhat.com \
    --cc=cel@us.ibm.com \
    --cc=gdb-patches@sourceware.org \
    --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).