Re: [PATCH 2/2] gdb: Add support for tracking the DWARF line table is-stmt field

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

On 2/11/20 2:57 PM, Andrew Burgess wrote:
> * Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-02-05 17:55:37 +0000]:
> 
>> On 2/5/20 12:37 PM, Andrew Burgess wrote:
>>
>> did you mean, when we don't land in the middle of a line ?
> 
> No, in this case I did write the correct thing.
> 
> So GDB had/has some code designed to "improve" the handling of looping
> constructs.  I think the idea is to handle cases like this:
> 
>   0x100      LN=5
>   0x104 <-\
>   0x108   |
>   0x10c   |  LN=6
>   0x110   |
>   0x114 --/
> 
> So when line 6 branches back to the middle of line 5, we don't stop
> (because we are in the middle of line 5), but we do want to stop at
> the start of line 6, so we "reset" the starting point back to line 5.
> 
> This is done by calling set_step_info in process_event_stop_test, in
> infrun.c.
> 
> What we actually did though was whenever we were at an address that
> didn't exactly match a SAL (in the example above, marked LN=5, LN=6)
> we called set_step_info.  This worked fine, at address 0x104 we reset
> back to LN=5, same at address 0x108.
> 
> However, in the new world things can look different, for example, like
> this:
> 
>   0x100      LN=5,STMT=1
>   0x104
>   0x108      LN=6,STMT=0
>   0x10c      LN=6,STMT=1
>   0x110
>   0x114
> 
> In this world, when we move forward to 0x100 we don't have a matching
> SAL, so we get the SAL for address 0x100.  We can then safely call
> set_step_info like we did before, that's fine.  But when we step to
> 0x108 we do now have a matching SAL, but we don't want to stop yet as
> this address is 'STMT=0'.  However, if we call set_step_info then GDB
> is going to think we are stepping away from LN=6, and so will refuse
> to stop at address 0x10c.
> 
> The proposal in this commit is that if we land at an address which
> doesn't specifically match a SAL, 0x104, 0x110, 0x114 in the above
> example, then we do call set_step_info, but otherwise we don't.
> 
> There are going to be situations where the behaviour of GDB changes
> after this patch, but I don't think we can avoid that.  What we had
> previously was just a heuristic.  I think enabling this new
> information in GDB will allow us to do better overall, so I think we
> should make this change and fix any issues as they show up.
> 

I agree with that, thanks for this explanation.

However, I think I found a small problem in this patch.
You can see it with the next-inline test case.
When you use just step the execution does not stop
between the inline functions, because not calling 

current behaviour is this:

Breakpoint 1, main () at next-inline.cc:63
63	  get_alias_set (&xx);
(gdb) s
get_alias_set (t=0x404040 <xx>) at next-inline.cc:50
50	  if (t != NULL
(gdb) s
51	      && TREE_TYPE (t).z != 1
(gdb) s
0x0000000000401169 in tree_check (i=0, t=0x404040 <xx>) at next-inline.h:34
34	  if (t->x != i)
(gdb) s
0x000000000040117d in tree_check (i=0, t=0x404040 <xx>) at next-inline.h:34
34	  if (t->x != i)
(gdb) s
0x000000000040118f in tree_check (i=0, t=0x404040 <xx>) at next-inline.h:34
34	  if (t->x != i)
(gdb) s
main () at next-inline.cc:64
64	  return 0;
(gdb) 

I debugged a bit because I was not sure why that happens, and it looks
like set_step_info does also set the current inline frame, but you
only want to suppress the line number not the currently executing
inline function.
With this small patch the step works as expected.

commit 193d81765d88b11e68111a8856a84cdf084d0b22
Author: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date:   Fri Feb 14 20:41:51 2020 +0100

    Fix next-inline test case with step
    
    Should stop between inline function invocations.

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 3919de8..89e6654 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -7190,6 +7190,8 @@ process_event_stop_test (struct execution_control_state *e
   ecs->event_thread->control.step_range_start = stop_pc_sal.pc;
   ecs->event_thread->control.step_range_end = stop_pc_sal.end;
   ecs->event_thread->control.may_range_step = 1;
+  ecs->event_thread->control.step_frame_id = get_frame_id (frame);
+  ecs->event_thread->control.step_stack_frame_id = get_stack_frame_id (frame);
   if (refresh_step_info)
     set_step_info (frame, stop_pc_sal);
 

I'm not sure if that is the cleanest solution, but it works.
With that adjustment, the stepping out of the inline and back into
makes the step call stop, which is the correct behavior:

Breakpoint 1, main () at next-inline.cc:63
63	  get_alias_set (&xx);
(gdb) s
get_alias_set (t=0x404040 <xx>) at next-inline.cc:50
50	  if (t != NULL
(gdb) s
51	      && TREE_TYPE (t).z != 1
(gdb) s
0x0000000000401169 in tree_check (i=0, t=0x404040 <xx>) at next-inline.h:34
34	  if (t->x != i)
(gdb) s
52	      && TREE_TYPE (t).z != 2
(gdb) s
tree_check (i=0, t=0x404040 <xx>) at next-inline.h:34
34	  if (t->x != i)
(gdb) s
53	      && TREE_TYPE (t).z != 3)
(gdb) s
tree_check (i=0, t=0x404040 <xx>) at next-inline.h:34
34	  if (t->x != i)
(gdb) s
main () at next-inline.cc:64
64	  return 0;
(gdb) 

If you like you can integrate that into your patch.
You will probably want to add that to the test case.

>>
>>> --- a/gdb/stack.c
>>> +++ b/gdb/stack.c
>>> @@ -330,7 +330,7 @@ frame_show_address (struct frame_info *frame,
>>>        return false;
>>>      }
>>>  
>>> -  return get_frame_pc (frame) != sal.pc;
>>> +  return get_frame_pc (frame) != sal.pc || !sal.is_stmt;
>>>  }
>>>  
>>>  /* See frame.h.  */
>>> @@ -1036,6 +1036,7 @@ print_frame_info (const frame_print_options &fp_opts,
>>>    int location_print;
>>>    struct ui_out *uiout = current_uiout;
>>>  
>>> +
>>>    if (!current_uiout->is_mi_like_p ()
>>>        && fp_opts.print_frame_info != print_frame_info_auto)
>>>      {
>>
>> Is this white space change intentional?
> 
> Ooops.  Fixed.
> 
> I also fixed the space before tab issue you pointed out in another
> mail.
> 
> I see you have a patch that depends on this one, but I'd like to leave
> this on the mailing list for a little longer before I merge it to give
> others a chance to comment.
> 

Thanks, regarding my other patch...

I used gprof to get performance numbers, because I was not sure if that
simple approach adds too much execution time, and it turns out that it
spent 30% of the complete execution time in the record_inline_range_end
function, when I was debugging GCC's cc1 executable.
So I decided that this needs to be optimized, I will send a new
version that does the line table patching with a binary search,
after the weekend.

Thanks
Bernd.


> I'll probably merge this over the weekend if there's no other
> feedback.
> 
> Thanks,
> Andrew
>