Re: [PATCH][gdb/breakpoint] Fix stepping past non-stmt line-table entries

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

On 1/14/21 2:26 PM, Tom de Vries wrote:
> Hi,
> 
> Consider the test-case small.c:
> ...
> $ cat -n small.c
>      1  __attribute__ ((noinline, noclone))
>      2  int foo (char *c)
>      3  {
>      4    asm volatile ("" : : "r" (c) : "memory");
>      5    return 1;
>      6  }
>      7
>      8  int main ()
>      9  {
>     10    char tpl1[20] = "/tmp/test.XXX";
>     11    char tpl2[20] = "/tmp/test.XXX";
>     12    int fd1 = foo (tpl1);
>     13    int fd2 = foo (tpl2);
>     14    if (fd1 == -1) {
>     15      return 1;
>     16    }
>     17
>     18    return 0;
>     19  }
> ...
> 
> Compiled with gcc-8 and optimization:
> ...
> $ gcc-8 -O2 -g small.c
> ...
> 
> We step through the calls to foo, but fail to visit line 13:
> ...
> 12	  int fd1 = foo (tpl1);
> (gdb) step
> foo (c=c@entry=0x7fffffffdea0 "/tmp/test.XXX") at small.c:5
> 5	  return 1;
> (gdb) step
> foo (c=c@entry=0x7fffffffdec0 "/tmp/test.XXX") at small.c:5
> 5	  return 1;
> (gdb) step
> main () at small.c:14
> 14	  if (fd1 == -1) {
> (gdb)
> ...
> 
> This is caused by the following.  The calls to foo are implemented by these
> insns:
> ....
>   4003df:       0f 29 04 24             movaps %xmm0,(%rsp)
>   4003e3:       0f 29 44 24 20          movaps %xmm0,0x20(%rsp)
>   4003e8:       e8 03 01 00 00          callq  4004f0 <foo>
>   4003ed:       48 8d 7c 24 20          lea    0x20(%rsp),%rdi
>   4003f2:       89 c2                   mov    %eax,%edx
>   4003f4:       e8 f7 00 00 00          callq  4004f0 <foo>
>   4003f9:       31 c0                   xor    %eax,%eax
> ...
> with corresponding line table entries:
> ...
> INDEX  LINE   ADDRESS            IS-STMT
> 8      12     0x00000000004003df Y
> 9      10     0x00000000004003df
> 10     11     0x00000000004003e3
> 11     12     0x00000000004003e8
> 12     13     0x00000000004003ed
> 13     12     0x00000000004003f2
> 14     13     0x00000000004003f4 Y
> 15     13     0x00000000004003f4
> 16     14     0x00000000004003f9 Y
> 17     14     0x00000000004003f9
> ...
> 
> Once we step out of the call to foo at 4003e8, we land at 4003ed, and gdb
> enters process_event_stop_test to figure out what to do.
> 
> That entry has is-stmt=n, so it's not the start of a line, so we don't stop
> there.  However, we do update ecs->event_thread->current_line to line 13,
> because the frame has changed (because we stepped out of the function).
> 
> Next we land at 4003f2.  Again the entry has is-stmt=n, so it's not the start
> of a line, so we don't stop there.  However, because the frame hasn't changed,
> we don't update update ecs->event_thread->current_line, so it stays 13.
> 
> Next we land at 4003f4.  Now is-stmt=y, so it's the start of a line, and we'd
> like to stop here.
> 
> But we don't stop because this test fails:
> ...
>   if ((ecs->event_thread->suspend.stop_pc == stop_pc_sal.pc)
>       && (ecs->event_thread->current_line != stop_pc_sal.line
>           || ecs->event_thread->current_symtab != stop_pc_sal.symtab))
>     {
> ...
> because ecs->event_thread->current_line == 13 and stop_pc_sal.line == 13.
> 
> Fix this by resetting ecs->event_thread->current_line to 0 if is-stmt=n and
> the frame has changed, such that we have:
> ...
> 12        int fd1 = foo (tpl1);
> (gdb) step
> foo (c=c@entry=0x7fffffffdbc0 "/tmp/test.XXX") at small.c:5
> 5         return 1;
> (gdb) step
> main () at small.c:13
> 13        int fd2 = foo (tpl2);
> (gdb)
> ...
> 
> Tested on x86_64-linux, with gcc-7 and gcc-8.
> 
> Any comments?
> 

I agree that this should be fixed.

It is interesting that the small.c example does not miss to
step at line 13 if I manually add a nop after the call foo.
In this case we are still at line 12, where the call is coming
from.

I think when we step away from the call instruction we should
use the line info from the call statement which is 12 in this
example, not the non-stmt line immediately after the call
stmt which is 13.

find_pc_line can be used to find the line immediately before
the current pc when the second parameter is non-zero.

So how about this:

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 7bbfe04..c823586 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -7048,6 +7048,19 @@ struct wait_one_event
 	  infrun_debug_printf ("stepped to a different line, but "
 			       "it's not the start of a statement");
 	}
+      else
+	{
+	  struct symtab_and_line call_line;
+
+	  /* We are returning from a call immediatley before the stop_pc.
+	     Therefore we are stepping away from the call line.  */
+	  call_line = find_pc_line (ecs->event_thread->suspend.stop_pc, 1);
+	  if (call_line.line != 0)
+	    {
+	      stop_pc_sal.line = call_line.line;
+	      stop_pc_sal.symtab = call_line.symtab;
+	    }
+	}
     }
 
   /* We aren't done stepping.
 

Bernd.