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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Luis Machado <luis.machado@linaro.org> [2020-02-06 06:00:29 -0300]:

> Hi,
> 
> I still need to check the patch itself, but i had a question about one
> particular paragraph...
> 
> On 2/5/20 8:37 AM, Andrew Burgess wrote:
> > This commit brings support for the DWARF line table is_stmt field to
> > GDB.  The is_stmt field is used by the compiler when a single source
> > line is split into multiple assembler instructions, especially if the
> > assembler instructions are interleaved with instruction from other
> > source lines.
> > 
> > The compiler will set the is_stmt flag false from some instructions
> > from the source lines, these instructions are not a good place to
> > insert a breakpoint in order to stop at the source line.
> > Instructions which are marked with the is_stmt flag true are a good
> > place to insert a breakpoint for that source line.
> > 
> > Currently GDB ignores all instructions for which is_stmt is false.
> > This is fine in a lot of cases, however, there are some cases where
> > this means the debug experience is not as good as it could be.
> > 
> > Consider stopping at a random instruction, currently this instruction
> > will be attributed to the last line table entry before this point for
> > which is_stmt was true - as these are the only line table entries that
> > GDB tracks.  This can easily be incorrect in code with even a low
> > level of optimisation.
> > 
> > With is_stmt tracking in place, when stopping at a random instruction
> > we now attribute the instruction back to the real source line, even
> > when is_stmt is false for that instruction in the line table.
> > 
> > When inserting breakpoints we still select line table entries for
> > which is_stmt is true, so the breakpoint placing behaviour should not
> > change.
> > 
> > When stepping though code (at the line level, not the instruction
> > level) we will still stop at instruction where is_stmt is true, I
> > think this is more likely to be the desired behaviour.
> 
> Considering a block of continuous instructions that map to the same source
> line, will line-stepping stop at each one of these instructions if is_stmt
> is true? As opposed to stepping over the the whole block until we see a line
> change?

No, a continuous block will be skipped as it is at the moment, even if
is_stmt is true for all entries.

>
> I'm wondering if this would help this bug
> (https://sourceware.org/bugzilla/show_bug.cgi?id=21221) in any way.

I took a look at this bug and even had a few ideas on how we might
improve GDB.  Below is one patch that I put together, though this only
fixes one of the example cases from that bug.  The problem this patch
runs into is that it regresses a few GDB tests
(gdb.base/gdb-sigterm.exp and
gdb.thread/step-bg-decr-pc-switch-thread.exp) in at least one of these
tests GDB makes the following assumption:  Single stepping on a one
line loop (e.g. 'for (;;);') will block forever.  The question then
is:  Does single step move us to the next source line to be executed
that is not the current line, or does single step move us to the next
source line to be executed, even if it is the same as the current
line?

I'm not sure what the answer is to this question...

Thanks,
Andrew

---

commit 35a72ddacf86c7e7f899c869558d1af3cfadb549
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Tue Feb 11 15:08:10 2020 +0000

    WIP: Try to better handle small loops
    
    This handles this case:
    
      int main(void)
      {
        int var = 0;
    
        for (;;)
          {
            var++;
          }
    
        return 0;
      }
    
    Compiled as 'gcc -g -o test.x test.c'.
    
    Change-Id: I6f499181eca5fb51b6edb050cbce0bda15665d38

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 3919de81f90..03878025959 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6674,11 +6674,14 @@ process_event_stop_test (struct execution_control_state *ecs)
 
       /* When stepping backward, stop at beginning of line range
 	 (unless it's the function entry point, in which case
-	 keep going back to the call point).  */
+	 keep going back to the call point).  When stepping forward we
+	 stop at the beginning of the range as this indicates we must be
+	 looping.  */
       CORE_ADDR stop_pc = ecs->event_thread->suspend.stop_pc;
       if (stop_pc == ecs->event_thread->control.step_range_start
-	  && stop_pc != ecs->stop_func_start
-	  && execution_direction == EXEC_REVERSE)
+	  && (execution_direction != EXEC_REVERSE
+	      || (stop_pc != ecs->stop_func_start
+		  && execution_direction == EXEC_REVERSE)))
 	end_stepping_range (ecs);
       else
 	keep_going (ecs);
@@ -7173,6 +7176,16 @@ process_event_stop_test (struct execution_control_state *ecs)
 	}
     }
 
+  if (ecs->event_thread->suspend.stop_pc
+      == ecs->event_thread->control.step_range_start)
+    {
+      /* Very small loops might only contain a single line information
+	 entry.  If this is the case then spot when we return to the start
+	 of the line range and stop again.  */
+      end_stepping_range (ecs);
+      return;
+    }
+
   /* We aren't done stepping.
 
      Optimize by setting the stepping range to the line.