[RFA] skip_prologue_sal and sal expansion

[RFA] skip_prologue_sal and sal expansion

[Jerome Guitton]

[-- Attachment #1: Type: text/plain, Size: 2355 bytes --]


A couple of cleanups in breakpoint.c. Let me give some background
first; consider the following program:

int counter = 42;

inline void
callee ()
{
  counter = 0; /* set breakpoint in an inlined function.  */
}

void
caller ()
{
  callee ();
}

int
main ()
{
  caller ();
  callee ();
  return counter;
}



When callee is inlined, we have three occurence for the line
"counter = 0;": inlined in caller, inlined in main, and not inlined.
When a breakpoint is set on this line, GDB sets a breakpoint on 3
locations.

(gdb) l p.c:6
1       int counter = 42;
2
3       inline void
4       callee ()
5       {
6         counter = 0;
7       }
8
9       void
10      caller ()
(gdb) b 6
Breakpoint 1 at 0x1800074: file p.c, line 6. (3 locations)


I have recently hit a bug in an assembler which was optimizing out the
prologue line info; it was making GDB think that the line
"counter = 0;" was a part of callee's prologue. And this pointed me to
something strange in GDB.

After having used this bogus assembler to generate my program, if I try
to set a breakpoint at line "counter = 0;", I end up with only one
occurence instead of three:

(gdb) b 6  
Breakpoint 1 at 0x1800074: file p.c, line 6.

The problem was in skip_prologue_sal defined in breakpoint.c. When it
actually skips a prologue, it does not assure that the other sal's
fields (explicit_pc and explicit_line) are left unchanged. In my case,
it was accidently changing explicit_line from 1 to 0. This change
disabled the line sal expansion, and in consequence we ended up with
the breakpoint set in only one location. I think that it's a bug in
skip_prologue_sal, this function should not change mess with these
fields.

Now, if I change skip_prologue_sal to copy explicit_line and
explicit_pc, the line expansion is done; but we should make sure that
prologue is skipped similarly, otherwise we get an assertion failure
when the address returned by resolve_sal_pc cannot be found after
line sal expansion:

(gdb) break p.c:6
../../src/gdb/breakpoint.c:5113: internal-error: expand_line_sal_maybe:
Assertion `found' failed.


Patch attached, tested on x86-linux. OK to apply?


2009-06-02  Jerome Guitton  <guitton@adacore.com>

	* breakpoint.c (expand_line_sal_maybe): When explicit_line,
	skip prologue on each sals.
	(skip_prologue_sal): Return explicit_line and explicit_pc
	unmodified.

[-- Attachment #2: inline.diff --]
[-- Type: text/x-diff, Size: 1568 bytes --]

Index: breakpoint.c
===================================================================
--- breakpoint.c	(revision 148760)
+++ breakpoint.c	(working copy)
@@ -207,6 +207,9 @@ static void disable_trace_command (char 
 
 static void trace_pass_command (char *, int);
 
+static void skip_prologue_sal (struct symtab_and_line *sal);
+
+
 /* Flag indicating that a command has proceeded the inferior past the
    current breakpoint.  */
 
@@ -5412,6 +5415,15 @@ expand_line_sal_maybe (struct symtab_and
 	    }
 	}
     }
+  else
+    {
+      for (i = 0; i < expanded.nelts; ++i)
+	{
+	  /* If this SAL corresponds to a breakpoint inserted using a
+	     line number, then skip the function prologue if necessary.  */
+	  skip_prologue_sal (&expanded.sals[i]);
+	}
+    }
 
   
   if (expanded.nelts <= 1)
@@ -5896,7 +5908,8 @@ set_breakpoint (char *address, char *con
 
 /* Adjust SAL to the first instruction past the function prologue.
    The end of the prologue is determined using the line table from
-   the debugging information.
+   the debugging information.  explicit_pc and explicit_line are
+   not modified.
 
    If SAL is already past the prologue, then do nothing.  */
 
@@ -5911,7 +5924,11 @@ skip_prologue_sal (struct symtab_and_lin
 
   start_sal = find_function_start_sal (sym, 1);
   if (sal->pc < start_sal.pc)
-    *sal = start_sal;
+    {
+      start_sal.explicit_line = sal->explicit_line;
+      start_sal.explicit_pc = sal->explicit_pc;
+      *sal = start_sal;
+    }
 }
 
 /* Helper function for break_command_1 and disassemble_command.  */

Re: [RFA] skip_prologue_sal and sal expansion

[Doug Evans]

On Tue, Jun 2, 2009 at 9:21 AM, Jerome Guitton <guitton@adacore.com> wrote:
>
> A couple of cleanups in breakpoint.c. Let me give some background
> first; consider the following program:
>
> int counter = 42;
>
> inline void
> callee ()
> {
>  counter = 0; /* set breakpoint in an inlined function.  */
> }
>
> void
> caller ()
> {
>  callee ();
> }
>
> int
> main ()
> {
>  caller ();
>  callee ();
>  return counter;
> }
>
>
>
> When callee is inlined, we have three occurence for the line
> "counter = 0;": inlined in caller, inlined in main, and not inlined.
> When a breakpoint is set on this line, GDB sets a breakpoint on 3
> locations.
>
> (gdb) l p.c:6
> 1       int counter = 42;
> 2
> 3       inline void
> 4       callee ()
> 5       {
> 6         counter = 0;
> 7       }
> 8
> 9       void
> 10      caller ()
> (gdb) b 6
> Breakpoint 1 at 0x1800074: file p.c, line 6. (3 locations)
>
>
> I have recently hit a bug in an assembler which was optimizing out the
> prologue line info; it was making GDB think that the line
> "counter = 0;" was a part of callee's prologue. And this pointed me to
> something strange in GDB.
>
> After having used this bogus assembler to generate my program, if I try
> to set a breakpoint at line "counter = 0;", I end up with only one
> occurence instead of three:
>
> (gdb) b 6
> Breakpoint 1 at 0x1800074: file p.c, line 6.
>
> The problem was in skip_prologue_sal defined in breakpoint.c. When it
> actually skips a prologue, it does not assure that the other sal's
> fields (explicit_pc and explicit_line) are left unchanged. In my case,
> it was accidently changing explicit_line from 1 to 0. This change
> disabled the line sal expansion, and in consequence we ended up with
> the breakpoint set in only one location. I think that it's a bug in
> skip_prologue_sal, this function should not change mess with these
> fields.
>
> Now, if I change skip_prologue_sal to copy explicit_line and
> explicit_pc, the line expansion is done; but we should make sure that
> prologue is skipped similarly, otherwise we get an assertion failure
> when the address returned by resolve_sal_pc cannot be found after
> line sal expansion:
>
> (gdb) break p.c:6
> ../../src/gdb/breakpoint.c:5113: internal-error: expand_line_sal_maybe:
> Assertion `found' failed.
>
>
> Patch attached, tested on x86-linux. OK to apply?
>
>
> 2009-06-02  Jerome Guitton  <guitton@adacore.com>
>
>        * breakpoint.c (expand_line_sal_maybe): When explicit_line,
>        skip prologue on each sals.
>        (skip_prologue_sal): Return explicit_line and explicit_pc
>        unmodified.
>

Sounds reasonable to me (fwiw).  Still need to wait for an official
maintainer's comments.

Re: [RFA] skip_prologue_sal and sal expansion

[Jerome Guitton]

Ping


Jerome Guitton (guitton@adacore.com):

> 
> A couple of cleanups in breakpoint.c. Let me give some background
> first; consider the following program:
> 
> int counter = 42;
> 
> inline void
> callee ()
> {
>   counter = 0; /* set breakpoint in an inlined function.  */
> }
> 
> void
> caller ()
> {
>   callee ();
> }
> 
> int
> main ()
> {
>   caller ();
>   callee ();
>   return counter;
> }
> 
> 
> 
> When callee is inlined, we have three occurence for the line
> "counter = 0;": inlined in caller, inlined in main, and not inlined.
> When a breakpoint is set on this line, GDB sets a breakpoint on 3
> locations.
> 
> (gdb) l p.c:6
> 1       int counter = 42;
> 2
> 3       inline void
> 4       callee ()
> 5       {
> 6         counter = 0;
> 7       }
> 8
> 9       void
> 10      caller ()
> (gdb) b 6
> Breakpoint 1 at 0x1800074: file p.c, line 6. (3 locations)
> 
> 
> I have recently hit a bug in an assembler which was optimizing out the
> prologue line info; it was making GDB think that the line
> "counter = 0;" was a part of callee's prologue. And this pointed me to
> something strange in GDB.
> 
> After having used this bogus assembler to generate my program, if I try
> to set a breakpoint at line "counter = 0;", I end up with only one
> occurence instead of three:
> 
> (gdb) b 6  
> Breakpoint 1 at 0x1800074: file p.c, line 6.
> 
> The problem was in skip_prologue_sal defined in breakpoint.c. When it
> actually skips a prologue, it does not assure that the other sal's
> fields (explicit_pc and explicit_line) are left unchanged. In my case,
> it was accidently changing explicit_line from 1 to 0. This change
> disabled the line sal expansion, and in consequence we ended up with
> the breakpoint set in only one location. I think that it's a bug in
> skip_prologue_sal, this function should not change mess with these
> fields.
> 
> Now, if I change skip_prologue_sal to copy explicit_line and
> explicit_pc, the line expansion is done; but we should make sure that
> prologue is skipped similarly, otherwise we get an assertion failure
> when the address returned by resolve_sal_pc cannot be found after
> line sal expansion:
> 
> (gdb) break p.c:6
> ../../src/gdb/breakpoint.c:5113: internal-error: expand_line_sal_maybe:
> Assertion `found' failed.
> 
> 
> Patch attached, tested on x86-linux. OK to apply?
> 
> 
> 2009-06-02  Jerome Guitton  <guitton@adacore.com>
> 
> 	* breakpoint.c (expand_line_sal_maybe): When explicit_line,
> 	skip prologue on each sals.
> 	(skip_prologue_sal): Return explicit_line and explicit_pc
> 	unmodified.

> Index: breakpoint.c
> ===================================================================
> --- breakpoint.c	(revision 148760)
> +++ breakpoint.c	(working copy)
> @@ -207,6 +207,9 @@ static void disable_trace_command (char 
>  
>  static void trace_pass_command (char *, int);
>  
> +static void skip_prologue_sal (struct symtab_and_line *sal);
> +
> +
>  /* Flag indicating that a command has proceeded the inferior past the
>     current breakpoint.  */
>  
> @@ -5412,6 +5415,15 @@ expand_line_sal_maybe (struct symtab_and
>  	    }
>  	}
>      }
> +  else
> +    {
> +      for (i = 0; i < expanded.nelts; ++i)
> +	{
> +	  /* If this SAL corresponds to a breakpoint inserted using a
> +	     line number, then skip the function prologue if necessary.  */
> +	  skip_prologue_sal (&expanded.sals[i]);
> +	}
> +    }
>  
>    
>    if (expanded.nelts <= 1)
> @@ -5896,7 +5908,8 @@ set_breakpoint (char *address, char *con
>  
>  /* Adjust SAL to the first instruction past the function prologue.
>     The end of the prologue is determined using the line table from
> -   the debugging information.
> +   the debugging information.  explicit_pc and explicit_line are
> +   not modified.
>  
>     If SAL is already past the prologue, then do nothing.  */
>  
> @@ -5911,7 +5924,11 @@ skip_prologue_sal (struct symtab_and_lin
>  
>    start_sal = find_function_start_sal (sym, 1);
>    if (sal->pc < start_sal.pc)
> -    *sal = start_sal;
> +    {
> +      start_sal.explicit_line = sal->explicit_line;
> +      start_sal.explicit_pc = sal->explicit_pc;
> +      *sal = start_sal;
> +    }
>  }
>  
>  /* Helper function for break_command_1 and disassemble_command.  */

Re: [RFA] skip_prologue_sal and sal expansion

[Joel Brobecker]

> 2009-06-02  Jerome Guitton  <guitton@adacore.com>
> 
> 	* breakpoint.c (expand_line_sal_maybe): When explicit_line,
> 	skip prologue on each sals.
> 	(skip_prologue_sal): Return explicit_line and explicit_pc
> 	unmodified.

This makes sense to me. Please go ahead and check it in.

-- 
Joel

Re: [RFA] skip_prologue_sal and sal expansion

[Jerome Guitton]

Joel Brobecker (brobecker@adacore.com):

> > 2009-06-02  Jerome Guitton  <guitton@adacore.com>
> > 
> > 	* breakpoint.c (expand_line_sal_maybe): When explicit_line,
> > 	skip prologue on each sals.
> > 	(skip_prologue_sal): Return explicit_line and explicit_pc
> > 	unmodified.
> 
> This makes sense to me. Please go ahead and check it in.

Thank you. It has been checked in.