Broken prologue skipping with non-returning function

Broken prologue skipping with non-returning function

[Jonathan Larmour]

It seems the prologue analysis on ARM, but very probably more generically,
has problems if GCC is able to optimise away the prologue. I have an
example of this using GCC 4.3.x at -O1 and above.

Here's a testcase:
#include <stdio.h>
const char *args;
const char *saved1, *saved2;

void foo(void)
{
    if (args)
    {
        saved1=saved2=args;
        args=NULL;
    }
    for (;;) /* NOTHING */ ;
}
int main(int argc, char *argv[])
{
    args = argv[0];
    foo();
    return 0;
}
Compile with e.g.:
arm-none-eabi-gcc --save-temps -g -O1 -c foo.c
(and linked as per my OS runtime)

The foo.s contains:
foo:
.LFB10:
        .file 1 "foo.c"
        .loc 1 6 0
        @ Function supports interworking.
        @ args = 0, pretend = 0, frame = 0
        @ frame_needed = 0, uses_anonymous_args = 0
        @ link register save eliminated.
        .loc 1 7 0
        ldr     r3, .L5
        ldr     r2, [r3, #0]
        cmp     r2, #0
        beq     .L2
        .loc 1 9 0
        ldr     r3, .L5+4
[snip]

We end up with a .loc for both lines 6 and 7 with no intervening
instructions. gdb's symtab.c:find_pc_sect_line() looks for when the pc
changes to something different and thus ends up returning a symtab_and_line
indicating that the line at that pc is at the 'if' and runs from the start
of the function to the ldr after the .loc 1 9 0.

Thus the outcome as a user if you set a breakpoint at foo, is that it ends
up getting set *after* the conditional branch. Which means setting a
breakpoint at foo is unreliable as you may never hit it. Bad GDB, no biscuit.

But what is the fix? For a start, I envisage this will affect more than
just ARM, so a fix in generic code should by rights be best, although OTOH
some architectures can have calling conventions that guarantee the prologue
is never empty.

How about a patch like the following to the generic symtab.c? That way if
there are two locs in succession for different lines at the same pc, we
stop at the current loc, rather than a later one (after the pc actually
changes). Note that a line of 0 seems to need ignoring as that seems do
indicate the start of the function.

--- symtab.c~   2008-06-11 23:03:49.000000000 +0100
+++ symtab.c    2008-09-19 15:15:33.000000000 +0100
@@ -2264,7 +2264,7 @@
        {
          /* Leave prev pointing to the linetable entry for the last line
             that started at or before PC.  */
-         if (item->pc > pc)
+         if (item->pc > pc || (prev && item->pc == pc && prev->pc == pc &&
prev->line > 0 && item->line > prev->line))
            break;

          prev = item;


I can't help but feel uneasy about the above change though, simply because
this is quite an essential part of GDB and I'm concerned about unintended
consequences. What do people think?

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine
    >>>> Visit us on stand 905 at the Embedded Systems Show 2008 <<<<
    >>>> Oct 1-2, NEC, Birmingham, UK http://www.embedded.co.uk  <<<<

Re: Broken prologue skipping with non-returning function

[Daniel Jacobowitz]

On Fri, Sep 19, 2008 at 03:32:59PM +0100, Jonathan Larmour wrote:
> We end up with a .loc for both lines 6 and 7 with no intervening
> instructions. gdb's symtab.c:find_pc_sect_line() looks for when the pc
> changes to something different and thus ends up returning a symtab_and_line
> indicating that the line at that pc is at the 'if' and runs from the start
> of the function to the ldr after the .loc 1 9 0.

skip_prologue_using_sal is supposed to detect this.  We have a
patch to improve it in our internal tree that we haven't gotten round
to yet.  Here it is; I do not remember what the language_asm check was
really about, except that I'm sure it came up running the gdb
testsuite, so removing it and running asm-source.exp would probably
explain it.

-- 
Daniel Jacobowitz
CodeSourcery

--- symtab.c	2008-09-05 10:11:13.000000000 -0400
+++ symtab.c	2008-09-19 10:46:03.000000000 -0400
@@ -4198,6 +4235,7 @@ skip_prologue_using_sal (CORE_ADDR func_
   struct symtab_and_line prologue_sal;
   CORE_ADDR start_pc;
   CORE_ADDR end_pc;
+  struct block *bl;
 
   /* Get an initial range for the function.  */
   find_pc_partial_function (func_addr, NULL, &start_pc, &end_pc);
@@ -4206,11 +4244,35 @@ skip_prologue_using_sal (CORE_ADDR func_
   prologue_sal = find_pc_line (start_pc, 0);
   if (prologue_sal.line != 0)
     {
+      /* For langauges other than assembly, treat two consecutive line
+	 entries at the same address as a zero-instruction prologue.
+	 The GNU assembler emits separate line notes for each instruction
+	 in a multi-instruction macro, but compilers generally will not
+	 do this.  */
+      if (prologue_sal.symtab->language != language_asm)
+	{
+	  struct linetable *linetable = LINETABLE (prologue_sal.symtab);
+	  int exact;
+	  int idx = 0;
+
+	  /* Skip any earlier lines, and any end-of-sequence marker
+	     from a previous function.  */
+	  while (linetable->item[idx].pc != prologue_sal.pc
+		 || linetable->item[idx].line == 0)
+	    idx++;
+
+	  if (idx+1 < linetable->nitems
+	      && linetable->item[idx+1].line != 0
+	      && linetable->item[idx+1].pc == start_pc)
+	    return start_pc;
+	}
+
       /* If there is only one sal that covers the entire function,
 	 then it is probably a single line function, like
 	 "foo(){}". */
       if (prologue_sal.end >= end_pc)
 	return 0;
+
       while (prologue_sal.end < end_pc)
 	{
 	  struct symtab_and_line sal;
@@ -4232,7 +4313,14 @@ skip_prologue_using_sal (CORE_ADDR func_
 	  prologue_sal = sal;
 	}
     }
-  return prologue_sal.end;
+
+  if (prologue_sal.end < end_pc)
+    /* Return the end of this line, or zero if we could not find a
+       line.  */
+    return prologue_sal.end;
+  else
+    /* Don't return END_PC, which is past the end of the function.  */
+    return prologue_sal.pc;
 }
 \f
 struct symtabs_and_lines

Re: Broken prologue skipping with non-returning function

[Jonathan Larmour]

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

Daniel Jacobowitz wrote:
> On Fri, Sep 19, 2008 at 03:32:59PM +0100, Jonathan Larmour wrote:
>> We end up with a .loc for both lines 6 and 7 with no intervening
>> instructions. gdb's symtab.c:find_pc_sect_line() looks for when the pc
>> changes to something different and thus ends up returning a symtab_and_line
>> indicating that the line at that pc is at the 'if' and runs from the start
>> of the function to the ldr after the .loc 1 9 0.
> 
> skip_prologue_using_sal is supposed to detect this.  We have a
> patch to improve it in our internal tree that we haven't gotten round
> to yet.  Here it is; I do not remember what the language_asm check was
> really about, except that I'm sure it came up running the gdb
> testsuite, so removing it and running asm-source.exp would probably
> explain it.

Thanks! The current arm-tdep.c doesn't presently use
skip_prologue_using_sal() however. At a guess that's also lurking in your
internal tree, but nevermind, I'm attaching a patch assuming that's useful.

With both of these (and my tentative patch reverted) I can confirm it works
as expected.

If it helps, I have write after approval perms, and a valid current FSF
copyright assignment, including disclaimer with my current employer. I
noticed I need to update my email address in the MAINTAINERS file which I
can do too.

I can check in your change too. If so, presumably you already have a
ChangeLog entry you'd like me to use to ease your merges?

Jifl

2008-09-19  Jonathan Larmour  <jifl@eCosCentric.com>

	* arm-tdep.c (arm_skip_prologue): Call skip_prologue_using_sal
	instead of determining symbol and line info directly.

-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine
    >>>> Visit us on stand 905 at the Embedded Systems Show 2008 <<<<
    >>>> Oct 1-2, NEC, Birmingham, UK http://www.embedded.co.uk  <<<<

[-- Attachment #2: arm.skip.prologue.using.sal.patch --]
[-- Type: text/x-patch, Size: 2441 bytes --]

--- arm-tdep.c.old	2008-09-19 17:01:32.000000000 +0100
+++ arm-tdep.c	2008-09-19 17:23:42.000000000 +0100
@@ -519,43 +519,40 @@ arm_skip_prologue (struct gdbarch *gdbar
 {
   unsigned long inst;
   CORE_ADDR skip_pc;
-  CORE_ADDR func_addr, func_end = 0;
-  char *func_name;
+  CORE_ADDR func_addr, limit_pc;
   struct symtab_and_line sal;
 
   /* If we're in a dummy frame, don't even try to skip the prologue.  */
   if (deprecated_pc_in_call_dummy (pc))
     return pc;
 
-  /* See what the symbol table says.  */
-
-  if (find_pc_partial_function (pc, &func_name, &func_addr, &func_end))
-    {
-      struct symbol *sym;
-
-      /* Found a function.  */
-      sym = lookup_symbol (func_name, NULL, VAR_DOMAIN, NULL);
-      if (sym && SYMBOL_LANGUAGE (sym) != language_asm)
-        {
-	  /* Don't use this trick for assembly source files.  */
-	  sal = find_pc_line (func_addr, 0);
-	  if ((sal.line != 0) && (sal.end < func_end))
-	    return sal.end;
-        }
-    }
-
-  /* Can't find the prologue end in the symbol table, try it the hard way
-     by disassembling the instructions.  */
-
+  /* See if we can determine the end of the prologue via the symbol table.
+     If so, then return either PC, or the PC after the prologue, whichever
+     is greater.  */
+  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
+    {
+      CORE_ADDR post_prologue_pc = skip_prologue_using_sal (func_addr);
+      if (post_prologue_pc != 0)
+	return max (pc, post_prologue_pc);
+    }
+
+  /* Can't determine prologue from the symbol table, need to examine
+     instructions.  */
+
+  /* Find an upper limit on the function prologue using the debug
+     information.  If the debug information could not be used to provide
+     that bound, then use an arbitrary large number as the upper bound.  */
   /* Like arm_scan_prologue, stop no later than pc + 64. */
-  if (func_end == 0 || func_end > pc + 64)
-    func_end = pc + 64;
+  limit_pc = skip_prologue_using_sal (pc);
+  if (limit_pc == 0)
+    limit_pc = pc + 64;          /* Magic.  */
+
 
   /* Check if this is Thumb code.  */
   if (arm_pc_is_thumb (pc))
-    return thumb_analyze_prologue (gdbarch, pc, func_end, NULL);
+    return thumb_analyze_prologue (gdbarch, pc, limit_pc, NULL);
 
-  for (skip_pc = pc; skip_pc < func_end; skip_pc += 4)
+  for (skip_pc = pc; skip_pc < limit_pc; skip_pc += 4)
     {
       inst = read_memory_unsigned_integer (skip_pc, 4);
 

Re: Broken prologue skipping with non-returning function

[Daniel Jacobowitz]

On Fri, Sep 19, 2008 at 08:00:37PM +0100, Jonathan Larmour wrote:
> Thanks! The current arm-tdep.c doesn't presently use
> skip_prologue_using_sal() however. At a guess that's also lurking in your
> internal tree, but nevermind, I'm attaching a patch assuming that's useful.

It is indeed lurking... but it's got some warts in it you might not
believe.  I'll try to make some time to get at least the non-warty
bits out.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Broken prologue skipping with non-returning function

[Daniel Jacobowitz]

On Fri, Sep 19, 2008 at 08:00:37PM +0100, Jonathan Larmour wrote:
> I can check in your change too. If so, presumably you already have a
> ChangeLog entry you'd like me to use to ease your merges?
> 
> Jifl
> 
> 2008-09-19  Jonathan Larmour  <jifl@eCosCentric.com>
> 
> 	* arm-tdep.c (arm_skip_prologue): Call skip_prologue_using_sal
> 	instead of determining symbol and line info directly.

This patch is OK.  Here's mine again, with changelog; I committed it.

Both patches, in essentially the same form, have been in
CodeSourcery's tree for two years.  So I'm quite confident in them,
as far as GCC goes, and it's clearly an improvement from the ad-hoc
line check in arm-tdep.c.  skip_prologue_using_sal does have some
limitations; it's risky with incorrect debug info, and it's a problem
for non-GCC compilers that do not emit the duplicate line note.
More on that, some other time.

-- 
Daniel Jacobowitz
CodeSourcery

2008-09-22  Daniel Jacobowitz  <dan@codesourcery.com>

	* symtab.c (skip_prologue_using_sal): Treat two consecutive lines
	at the same address as a prologue marker.  Do not skip an entire
	function.

--- symtab.c	2008-09-05 10:11:13.000000000 -0400
+++ symtab.c	2008-09-19 10:46:03.000000000 -0400
@@ -4198,6 +4235,7 @@ skip_prologue_using_sal (CORE_ADDR func_
   struct symtab_and_line prologue_sal;
   CORE_ADDR start_pc;
   CORE_ADDR end_pc;
+  struct block *bl;
 
   /* Get an initial range for the function.  */
   find_pc_partial_function (func_addr, NULL, &start_pc, &end_pc);
@@ -4206,11 +4244,35 @@ skip_prologue_using_sal (CORE_ADDR func_
   prologue_sal = find_pc_line (start_pc, 0);
   if (prologue_sal.line != 0)
     {
+      /* For langauges other than assembly, treat two consecutive line
+	 entries at the same address as a zero-instruction prologue.
+	 The GNU assembler emits separate line notes for each instruction
+	 in a multi-instruction macro, but compilers generally will not
+	 do this.  */
+      if (prologue_sal.symtab->language != language_asm)
+	{
+	  struct linetable *linetable = LINETABLE (prologue_sal.symtab);
+	  int exact;
+	  int idx = 0;
+
+	  /* Skip any earlier lines, and any end-of-sequence marker
+	     from a previous function.  */
+	  while (linetable->item[idx].pc != prologue_sal.pc
+		 || linetable->item[idx].line == 0)
+	    idx++;
+
+	  if (idx+1 < linetable->nitems
+	      && linetable->item[idx+1].line != 0
+	      && linetable->item[idx+1].pc == start_pc)
+	    return start_pc;
+	}
+
       /* If there is only one sal that covers the entire function,
 	 then it is probably a single line function, like
 	 "foo(){}". */
       if (prologue_sal.end >= end_pc)
 	return 0;
+
       while (prologue_sal.end < end_pc)
 	{
 	  struct symtab_and_line sal;
@@ -4232,7 +4313,14 @@ skip_prologue_using_sal (CORE_ADDR func_
 	  prologue_sal = sal;
 	}
     }
-  return prologue_sal.end;
+
+  if (prologue_sal.end < end_pc)
+    /* Return the end of this line, or zero if we could not find a
+       line.  */
+    return prologue_sal.end;
+  else
+    /* Don't return END_PC, which is past the end of the function.  */
+    return prologue_sal.pc;
 }
 \f
 struct symtabs_and_lines

Re: Broken prologue skipping with non-returning function

[Jonathan Larmour]

Daniel Jacobowitz wrote:
> On Fri, Sep 19, 2008 at 08:00:37PM +0100, Jonathan Larmour wrote:
>>
>> 2008-09-19  Jonathan Larmour  <jifl@eCosCentric.com>
>>
>> 	* arm-tdep.c (arm_skip_prologue): Call skip_prologue_using_sal
>> 	instead of determining symbol and line info directly.
> 
> This patch is OK.

Thanks, committed, including fixing my email address in MAINTAINERS.

Jifl