diff mbox

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

Message ID AM6PR03MB517018CA7EE81453672EFDEFE4E20@AM6PR03MB5170.eurprd03.prod.outlook.com
State New
Headers show

Commit Message

Bernd Edlinger March 5, 2020, 6:01 p.m. UTC
On 2/14/20 9:05 PM, Bernd Edlinger wrote:
> 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.
> 

Hmm, I think this got stuck, so I just worked a follow-up patch out for you.

I also noticed that the test case cannot work with gcc before gcc-8,
since the is_stmt feature is not implemented earlier, at least with
gcc-4.8 and gcc-5.4 the test case fails.

I thought it would be good to detect that by adding the -gstatement-frontiers
option, so that the gcc driver complains when it is not able to generate
sufficient debug info for that test.

Thoughts?


Bernd.

Comments

Andrew Burgess March 6, 2020, 10:42 p.m. UTC | #1
* Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-03-05 18:01:45 +0000]:

> On 2/14/20 9:05 PM, Bernd Edlinger wrote:
> > 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.
> > 
> 
> Hmm, I think this got stuck, so I just worked a follow-up patch out
> for you.

Thank for this.  Just wanted to let you know that I saw this mail and
am currently integrating your fix into the original patch.  I have a
second fix I need to merge in and write a test for, but I hope to have
something together soon for you to take a look at.

Thanks,
Andrew



> 
> I also noticed that the test case cannot work with gcc before gcc-8,
> since the is_stmt feature is not implemented earlier, at least with
> gcc-4.8 and gcc-5.4 the test case fails.
> 
> I thought it would be good to detect that by adding the -gstatement-frontiers
> option, so that the gcc driver complains when it is not able to generate
> sufficient debug info for that test.
> 
> Thoughts?
> 
> 
> Bernd.

> From 3f39d068e3386ba204a598fa3d2946d357b1d7b2 Mon Sep 17 00:00:00 2001
> From: Bernd Edlinger <bernd.edlinger@hotmail.de>
> Date: Fri, 14 Feb 2020 20:41:51 +0100
> Subject: [PATCH] Fix next-inline test case with step
> 
> Should stop between inline function invocations.
> 
> Add -gstatement-frontiers compile option to avoid
> running test with gcc version that don't support
> this feature, which would fail the test unnecessarily.
> Instead make the compile step fail which is less noisy.
> 
> Add a prefix use_header / no_header to all test cases.
> 
> gdb:
> 2020-03-05  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* infrun.c (process_event_stop_test): Set step_frame_id.
> 
> gdb/testsuite:
> 2020-03-05  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* gdb.cp/next-inline.exp: Extend test case.
> ---
>  gdb/infrun.c                         |  2 ++
>  gdb/testsuite/gdb.cp/next-inline.exp | 45 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 6c35805..e8221ba 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -7219,6 +7219,8 @@ process_event_stop_test (struct execution_control_state *ecs)
>    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);
>  
> diff --git a/gdb/testsuite/gdb.cp/next-inline.exp b/gdb/testsuite/gdb.cp/next-inline.exp
> index 0b2b22d..ec04694 100644
> --- a/gdb/testsuite/gdb.cp/next-inline.exp
> +++ b/gdb/testsuite/gdb.cp/next-inline.exp
> @@ -20,12 +20,16 @@ standard_testfile .cc
>  proc do_test { use_header } {
>      global srcfile testfile
>  
> -    set options {c++ debug nowarnings optimize=-O2}
> +    set options {c++ debug nowarnings optimize=-O2\ -gstatement-frontiers}
>      if { $use_header } {
>  	lappend options additional_flags=-DUSE_NEXT_INLINE_H
>  	set executable "$testfile-with-header"
> +	set hdrfile "next-inline.h"
> +	set prefix "use_header"
>      } else {
>  	set executable "$testfile-no-header"
> +	set hdrfile "$srcfile"
> +	set prefix "no_header"
>      }
>  
>      if { [prepare_for_testing "failed to prepare" $executable \
> @@ -33,6 +37,8 @@ proc do_test { use_header } {
>  	return -1
>      }
>  
> +    with_test_prefix $prefix {
> +
>      if ![runto_main] {
>  	fail "can't run to main"
>  	return
> @@ -64,6 +70,43 @@ proc do_test { use_header } {
>      gdb_test "bt" \
>  	"\\s*\\#0\\s+(main|get_alias_set)\[^\r\]*${srcfile}:.*" \
>  	"not in inline 5"
> +
> +    if {!$use_header} {
> +        return #until that is fixed
> +    }
> +
> +    if ![runto_main] {
> +	fail "can't run to main pass 2"
> +	return
> +    }
> +
> +    gdb_test "bt" "\\s*\\#0\\s+main.*" "in main pass 2"
> +    gdb_test "step" ".*" "step into get_alias_set pass 2"
> +    gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
> +	"in get_alias_set pass 2"
> +    gdb_test "step" ".*TREE_TYPE.*" "step 1"
> +    gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
> +	"not in inline 1 pass 2"
> +    gdb_test "step" ".*if \\(t->x != i\\).*" "step 2"
> +    gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*tree_check\[^\r\]*${hdrfile}:.*" \
> +	"in inline 1 pass 2"
> +    gdb_test "step" ".*TREE_TYPE.*" "step 3"
> +    gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
> +	"not in inline 2 pass 2"
> +    gdb_test "step" ".*if \\(t->x != i\\).*" "step 4"
> +    gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*tree_check\[^\r\]*${hdrfile}:.*" \
> +	"in inline 2 pass 2"
> +    gdb_test "step" ".*TREE_TYPE.*" "step 5"
> +    gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
> +	"not in inline 3 pass 2"
> +    gdb_test "step" ".*if \\(t->x != i\\).*" "step 6"
> +    gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*tree_check\[^\r\]*${hdrfile}:.*" \
> +	"in inline 3 pass 2"
> +    gdb_test "step" "return 0.*" "step 7"
> +    gdb_test "bt" \
> +	"\\s*\\#0\\s+(main|get_alias_set)\[^\r\]*${srcfile}:.*" \
> +	"not in inline 4 pass 2"
> +    }
>  }
>  
>  do_test 0
> -- 
> 1.9.1
>
diff mbox

Patch

From 3f39d068e3386ba204a598fa3d2946d357b1d7b2 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Fri, 14 Feb 2020 20:41:51 +0100
Subject: [PATCH] Fix next-inline test case with step

Should stop between inline function invocations.

Add -gstatement-frontiers compile option to avoid
running test with gcc version that don't support
this feature, which would fail the test unnecessarily.
Instead make the compile step fail which is less noisy.

Add a prefix use_header / no_header to all test cases.

gdb:
2020-03-05  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* infrun.c (process_event_stop_test): Set step_frame_id.

gdb/testsuite:
2020-03-05  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* gdb.cp/next-inline.exp: Extend test case.
---
 gdb/infrun.c                         |  2 ++
 gdb/testsuite/gdb.cp/next-inline.exp | 45 +++++++++++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 6c35805..e8221ba 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -7219,6 +7219,8 @@  process_event_stop_test (struct execution_control_state *ecs)
   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);
 
diff --git a/gdb/testsuite/gdb.cp/next-inline.exp b/gdb/testsuite/gdb.cp/next-inline.exp
index 0b2b22d..ec04694 100644
--- a/gdb/testsuite/gdb.cp/next-inline.exp
+++ b/gdb/testsuite/gdb.cp/next-inline.exp
@@ -20,12 +20,16 @@  standard_testfile .cc
 proc do_test { use_header } {
     global srcfile testfile
 
-    set options {c++ debug nowarnings optimize=-O2}
+    set options {c++ debug nowarnings optimize=-O2\ -gstatement-frontiers}
     if { $use_header } {
 	lappend options additional_flags=-DUSE_NEXT_INLINE_H
 	set executable "$testfile-with-header"
+	set hdrfile "next-inline.h"
+	set prefix "use_header"
     } else {
 	set executable "$testfile-no-header"
+	set hdrfile "$srcfile"
+	set prefix "no_header"
     }
 
     if { [prepare_for_testing "failed to prepare" $executable \
@@ -33,6 +37,8 @@  proc do_test { use_header } {
 	return -1
     }
 
+    with_test_prefix $prefix {
+
     if ![runto_main] {
 	fail "can't run to main"
 	return
@@ -64,6 +70,43 @@  proc do_test { use_header } {
     gdb_test "bt" \
 	"\\s*\\#0\\s+(main|get_alias_set)\[^\r\]*${srcfile}:.*" \
 	"not in inline 5"
+
+    if {!$use_header} {
+        return #until that is fixed
+    }
+
+    if ![runto_main] {
+	fail "can't run to main pass 2"
+	return
+    }
+
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "in main pass 2"
+    gdb_test "step" ".*" "step into get_alias_set pass 2"
+    gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
+	"in get_alias_set pass 2"
+    gdb_test "step" ".*TREE_TYPE.*" "step 1"
+    gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
+	"not in inline 1 pass 2"
+    gdb_test "step" ".*if \\(t->x != i\\).*" "step 2"
+    gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*tree_check\[^\r\]*${hdrfile}:.*" \
+	"in inline 1 pass 2"
+    gdb_test "step" ".*TREE_TYPE.*" "step 3"
+    gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
+	"not in inline 2 pass 2"
+    gdb_test "step" ".*if \\(t->x != i\\).*" "step 4"
+    gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*tree_check\[^\r\]*${hdrfile}:.*" \
+	"in inline 2 pass 2"
+    gdb_test "step" ".*TREE_TYPE.*" "step 5"
+    gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
+	"not in inline 3 pass 2"
+    gdb_test "step" ".*if \\(t->x != i\\).*" "step 6"
+    gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*tree_check\[^\r\]*${hdrfile}:.*" \
+	"in inline 3 pass 2"
+    gdb_test "step" "return 0.*" "step 7"
+    gdb_test "bt" \
+	"\\s*\\#0\\s+(main|get_alias_set)\[^\r\]*${srcfile}:.*" \
+	"not in inline 4 pass 2"
+    }
 }
 
 do_test 0
-- 
1.9.1