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

[Tom de Vries <tdevries@suse.de>]

On 1/23/21 9:07 AM, Bernd Edlinger wrote:
> 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.
> 

Hi Bernd,

thanks for the review.

> 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 can reproduce that like so:
...
         call    foo
+        nop
 .LVL1:
         .loc 1 13 13 view .LVU15
         leaq    32(%rsp), %rdi
         .loc 1 12 13 view .LVU16
         movl    %eax, %edx
 .LVL2:
         .loc 1 13 3 is_stmt 1 view .LVU17
         .loc 1 13 13 is_stmt 0 view .LVU18
         call    foo
...
resulting in:
...
  4003d7:       0f 29 04 24             movaps %xmm0,(%rsp)
  4003db:       0f 29 44 24 20          movaps %xmm0,0x20(%rsp)
  4003e0:       c7 44 24 30 00 00 00    movl   $0x0,0x30(%rsp)
  4003e7:       00
  4003e8:       e8 03 01 00 00          callq  4004f0 <foo>
  4003ed:       90                      nop
  4003ee:       48 8d 7c 24 20          lea    0x20(%rsp),%rdi
  4003f3:       89 c2                   mov    %eax,%edx
  4003f5:       e8 f6 00 00 00          callq  4004f0 <foo>
  4003fa:       31 c0                   xor    %eax,%eax
...

So, once we step out of the call to foo at 4003e8, we land at the nop at
4003ed, and gdb enters process_event_stop_test to figure out what to do.

Since there's no line number entry at that address, we get:
...
(gdb) p stop_pc_sal.line
$7 = 12
...
instead of line 13, which causes the stepping to stop at the subsequent
is-stmt=y entry at line 13.

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

I think we should use either the line number indicated by the line
table, or 0.  I don't see a need to make things more complicated than that.

The preferred solution would be to use the one from the line table, but
I think that will require more changes to the stepping mechanism, so
setting the line to 0 is a nice, localized solution.

I've added a debug print to note the case:
...
+         infrun_debug_printf ("stepped to a different frame, but "
+                              "it's not the start of a statement");
+
...
and I'll commit shortly.

Thanks,
- Tom