Re: [PATCH 3/8] Break at each iteration for breakpoints placed on a while statement

[Kevin Buettner <kevinb@redhat.com>]

Hi Pedro,

Thanks again for the review.

On Wed, 30 Sep 2015 13:16:55 +0100
Pedro Alves <palves@redhat.com> wrote:

> >     The pratical effect of this is that a breakpoint placed on a while
> 
> "practical"

> > +	       The pratical effect of this is that a breakpoint placed
> 
> "practical".

Both fixed in new patch.

> > +	    /* Only use branch destination if it's in the same block and
> > +	       is also within one of the sals from the initial list.  */
> > +	    if (is_branch && blocks[i]->startaddr <= branch_dest
> > +	        && branch_dest < blocks[i]->endaddr
> > +		&& addr_in_sals (branch_dest, intermediate_results.nelts,
> > +		                 intermediate_results.sals))
> 
> I believe intermediate_results can contain sals of different
> pspaces / inferiors.  Shouldn't we take that into account somehow?

Good catch.  I had not considered this.

In the new patch, I've added a `pspace' paramater to addr_in_sals()
and have added the necessary code to that function.  The end result is
that only sals in the program space of the sal that we might
potentially fix up will be considered.  (I hope that makes sense.)

> 
> > +	      {
> > +		intermediate_results.sals[i].pc = branch_dest;
> > +	      }
> 
> Unnecessary braces.

I took them out.

Here's the revised patch:

commit 6018c9ae5ea2bc5fcf723ce9a9beeb315d3fb3c0
Author: Kevin Buettner <kevinb@redhat.com>
Date:   Tue Aug 18 22:06:40 2015 -0700

    Break at each iteration for breakpoints placed on a while statement.
    
    This patch changes create_sals_line_offset() in linespec.c so that, for
    a given SAL, if that SAL's address (pc) refers to an unconditional
    branch instruction whose branch target also refers to the same SAL, then
    the branch target is used for the SAL instead.
    
    The practical effect of this is that a breakpoint placed on a while
    loop will break at the evaluation of the condition instead of at the
    unconditional branch which transfers control to the starting address
    for the evaluation of the condition.
    
    Consider the following code snippet (which is taken from one of the
    new tests for this patch set):
    
        9         while (v < 3)                         /* Loop 1 condition */
        10          {
        11            v++;                              /* Loop 1 increment */
        12          }
    
    This is compiled as the following x86_64 code:
    
        0x000000000040059e <loop_test+14>:   jmp    0x4005af <loop_test+31>
        0x00000000004005a0 <loop_test+16>:   mov    0x200a8a(%rip),%eax # 0x601030 <v>
        0x00000000004005a6 <loop_test+22>:   add    $0x1,%eax
        0x00000000004005a9 <loop_test+25>:   mov    %eax,0x200a81(%rip) # 0x601030 <v>
        0x00000000004005af <loop_test+31>:   mov    0x200a7b(%rip),%eax # 0x601030 <v>
        0x00000000004005b5 <loop_test+37>:   cmp    $0x2,%eax
        0x00000000004005b8 <loop_test+40>:   jle    0x4005a0 <loop_test+16>
    
    If a breakpoint is placed on line 9, which begins at loop_test+14, this
    change/patch causes the breakpoint to be placed on loop_test+31, which is
    the starting address for the evaluation of the condition.
    
    In order for this to work, an architecture specific method,
    unconditional_branch_destination, was introduced in an earlier patch in
    the set.  I've implemented this method for x86_64, i386, arm, thumb,
    powerpc, rx, and rl78.
    
    I've tested on each of these architectures and see no regressions.
    
    gdb/ChangeLog:
    	* linespec.c (addr_in_sals): New function.
    	(create_sals_line_offset): Adjust SAL whose pc refers to an
    	unconditional branch whose destination is the same line.
---
 gdb/linespec.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/gdb/linespec.c b/gdb/linespec.c
index b2233b9..1ba0e67 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -1809,6 +1809,37 @@ canonicalize_linespec (struct linespec_state *state, const linespec_p ls)
     }
 }
 
+/* Return 1 if one of the SALS between 0 and NELTS contains ADDR. 
+   Return 0 otherwise.  */
+
+static int
+addr_in_sals (CORE_ADDR addr, struct program_space *pspace, int nelts,
+              struct symtab_and_line *sals)
+{
+  int i;
+
+  /* Make sure we're in the correct program space for a potential
+     call to find_pc_sect_line, below.  */
+  set_current_program_space (pspace);
+
+  for (i = 0; i < nelts; i++)
+    {
+      struct symtab_and_line sal;
+
+      if (pspace != sals[i].pspace)
+	continue;
+
+      if (sals[i].end == 0)
+        sal = find_pc_sect_line (sals[i].pc, sals[i].section, 0);
+      else
+        sal = sals[i];
+
+      if (sal.pc <= addr && addr < sal.end)
+	return 1;
+    }
+  return 0;
+}
+
 /* Given a line offset in LS, construct the relevant SALs.  */
 
 static struct symtabs_and_lines
@@ -1934,6 +1965,60 @@ create_sals_line_offset (struct linespec_state *self,
 	    struct symbol *sym = (blocks[i]
 				  ? block_containing_function (blocks[i])
 				  : NULL);
+	    int is_branch;
+	    CORE_ADDR branch_dest;
+	    
+	    /* This next bit of code examines the instruction at the
+	       SAL's address (pc).  If the instruction is an
+	       unconditional branch whose branch destination also
+	       refers to the same SAL, then the branch destination is
+	       used for the SAL's pc value instead.
+
+	       The practical effect of this is that a breakpoint placed
+	       on a while loop will break at the evaluation of the
+	       condition instead of at an unconditional branch which
+	       transfers control to the starting address for the
+	       evaluation of the condition.
+
+	       Consider the following code snippet (which is taken
+	       from the test case for gdb.base/loop-break.exp.)
+
+		 while (v < 3)
+                  {
+                    v++; 
+                  }
+
+	       On x86_64, this might be compiled as follows:
+
+		 0x40059e <loop_test+14>:   jmp    0x4005af <loop_test+31>
+		 0x4005a0 <loop_test+16>:   mov    0x200a8a(%rip),%eax
+		 0x4005a6 <loop_test+22>:   add    $0x1,%eax
+		 0x4005a9 <loop_test+25>:   mov    %eax,0x200a81(%rip)
+		 0x4005af <loop_test+31>:   mov    0x200a7b(%rip),%eax
+		 0x4005b5 <loop_test+37>:   cmp    $0x2,%eax
+		 0x4005b8 <loop_test+40>:   jle    0x4005a0 <loop_test+16>
+
+	       If a breakpoint is placed on the while statement line
+	       which begins at loop_test+14, the following code causes
+	       the breakpoint to be (instead) placed on loop_test+31, which
+	       is the starting address for the evaluation of the condition.  */
+
+	    is_branch = gdbarch_unconditional_branch_destination
+	      (get_objfile_arch
+	        (SYMTAB_OBJFILE (intermediate_results.sals[i].symtab)),
+	       intermediate_results.sals[i].pc,
+	       &branch_dest);
+
+	    /* Only use branch destination if it's in the same block and
+	       is also within one of the sals (within the same program
+	       space) from the initial list.  */
+	    if (is_branch && blocks[i]->startaddr <= branch_dest
+	        && branch_dest < blocks[i]->endaddr
+		&& addr_in_sals (branch_dest,
+		                 intermediate_results.sals[i].pspace,
+		                 intermediate_results.nelts,
+		                 intermediate_results.sals))
+	      intermediate_results.sals[i].pc = branch_dest;
 
 	    if (self->funfirstline)
 	      skip_prologue_sal (&intermediate_results.sals[i]);