Re: Re: [PATCH] Fix for incorect breakpoint set in case of clang compiled binary

[KARTHIKVENKATESH BHAT <kv dot bhat at samsung dot com>]

Hi Mark,
Thanks for the review.
I have tested most of the gdb testuites which require proper line info to be set (with clang compiled binary on x86) and they are working fine after this fix.

> If your immediate goal is to fix things for clang, I recommend you
> resubmit your change addressing only clang and worry about GCC later

Yes currently I'm intrested in fixing this for clang binaries. I have updated the patch to handle only clang compiled binary. For other producers the previous code flow will apply.

> You probably want to add similar code to the prologue skipping code in
> amd64-tdep.c.

Yes. I have updated the file as well.

Please find the Changes after comments -

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.14860
diff -u -p -r1.14860 ChangeLog
--- ChangeLog	27 Nov 2012 08:11:58 -0000	1.14860
+++ ChangeLog	27 Nov 2012 13:05:10 -0000
@@ -1,3 +1,10 @@
+2012-11-27  Karthik Bhat  <kv.bhat@samsung.com>
+
+	* i386-tdep.c (i386_skip_prologue): Using symbol table
+	to find the end of prologue for clang compiled binaries.
+	* amd64-tdep.c (amd64_skip_prologue):Using symbol table
+	to find the end of prologue for clang compiled binaries.
+
 2012-11-27  Daniel Jacobowitz  <dan@codesourcery.com>
 	    Kazu Hirata  <kazu@codesourcery.com>
 	    Yao Qi  <yao@codesourcery.com>
Index: amd64-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/amd64-tdep.c,v
retrieving revision 1.115
diff -u -p -r1.115 amd64-tdep.c
--- amd64-tdep.c	26 Oct 2012 19:34:09 -0000	1.115
+++ amd64-tdep.c	27 Nov 2012 13:05:11 -0000
@@ -2252,6 +2252,22 @@ amd64_skip_prologue (struct gdbarch *gdb
 {
   struct amd64_frame_cache cache;
   CORE_ADDR pc;
+  CORE_ADDR func_addr;
+
+  if (find_pc_partial_function (start_pc, NULL, &func_addr, NULL))
+    {
+      CORE_ADDR post_prologue_pc
+	= skip_prologue_using_sal (gdbarch, func_addr);
+      struct symtab *s = find_pc_symtab (func_addr);
+
+      /* Clang always emits a line note before the prologue and another
+	 one after.We trust clang to emit usable line notes  */
+      if (post_prologue_pc
+	  && (s != NULL
+	      && s->producer != NULL
+	      && strncmp (s->producer, "clang ", sizeof ("clang ") - 1) == 0))
+        return max(start_pc,post_prologue_pc);
+    }
 
   amd64_init_frame_cache (&cache);
   pc = amd64_analyze_prologue (gdbarch, start_pc, 0xffffffffffffffffLL,
Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.364
diff -u -p -r1.364 i386-tdep.c
--- i386-tdep.c	21 Nov 2012 14:09:10 -0000	1.364
+++ i386-tdep.c	27 Nov 2012 13:05:12 -0000
@@ -1582,7 +1582,23 @@ i386_skip_prologue (struct gdbarch *gdba
   CORE_ADDR pc;
   gdb_byte op;
   int i;
+  CORE_ADDR func_addr;
 
+  if (find_pc_partial_function (start_pc, NULL, &func_addr, NULL))
+    {
+      CORE_ADDR post_prologue_pc
+	= skip_prologue_using_sal (gdbarch, func_addr);
+      struct symtab *s = find_pc_symtab (func_addr);
+
+      /* Clang always emits a line note before the prologue and another
+	 one after.We trust clang to emit usable line notes  */
+      if (post_prologue_pc
+	  && (s != NULL
+	      && s->producer != NULL
+	      && strncmp (s->producer, "clang ", sizeof ("clang ") - 1) == 0))
+        return max(start_pc,post_prologue_pc);
+    }
+ 
   cache.locals = -1;
   pc = i386_analyze_prologue (gdbarch, start_pc, 0xffffffff, &cache);
   if (cache.locals < 0)

Please let me know if it is ok.

Thanks


------- Original Message -------
Sender : Mark Kettenis<mark.kettenis@xs4all.nl>
Date : Nov 27, 2012 20:14 (GMT+09:00)
Title : Re: [PATCH] Fix for incorect breakpoint set in case of clang compiled binary

> Date: Wed, 21 Nov 2012 15:39:22 +0000 (GMT)
> From: KARTHIKVENKATESH BHAT 
> 
> Thanks Pedro/Mark. Appologies for the build break. I'm a bit new to GDB community will take care of it from next time.
> I have fixed the warning resulting in error and modified the indentation -
> Let me also try to explain the fix a bit more. 
> 
> Index: ChangeLog
> ===================================================================
> RCS file: /cvs/src/src/gdb/ChangeLog,v
> retrieving revision 1.14849
> diff -u -p -r1.14849 ChangeLog
> --- ChangeLog 21 Nov 2012 14:09:03 -0000 1.14849
> +++ ChangeLog 21 Nov 2012 15:11:47 -0000
> @@ -1,3 +1,9 @@
> +2012-11-20  Karthik Bhat  
> +
> + * i386-tdep.c (i386_skip_prologue): See if we
> + can determine the end of the prologue via the symbol table.
> + If so use the same instead of going through prologue instructions.
> +
>  2012-11-21  Yao Qi  
>  
>   PR tdep/7438
> Index: i386-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/i386-tdep.c,v
> retrieving revision 1.364
> diff -u -p -r1.364 i386-tdep.c
> --- i386-tdep.c 21 Nov 2012 14:09:10 -0000 1.364
> +++ i386-tdep.c 21 Nov 2012 15:11:48 -0000
> @@ -1582,6 +1582,27 @@ i386_skip_prologue (struct gdbarch *gdba
>    CORE_ADDR pc;
>    gdb_byte op;
>    int i;
> +  CORE_ADDR func_addr;
> +
> +  if (find_pc_partial_function (start_pc, NULL, &func_addr, NULL))
> +    {
> +      CORE_ADDR post_prologue_pc
> + = skip_prologue_using_sal (gdbarch, func_addr);
> +      struct symtab *s = find_pc_symtab (func_addr);
> +
> +      /* GCC and clang always emits a line note before the prologue and another
> + one after, even if the two are at the same address or on the
> + same line.  Take advantage of this so that we do not need to
> + know every instruction that might appear in the prologue.  We
> + will have producer information for most binaries; if it is
> + missing (e.g. for -gstabs), assuming the GNU tools.  */
> +      if (post_prologue_pc
> +   && (s == NULL
> +       || s->producer == NULL
> +       || strncmp (s->producer, "GNU ", sizeof ("GNU ") - 1) == 0 
> +       || strncmp (s->producer, "clang ", sizeof ("clang ") - 1) == 0))
> + return max (start_pc, post_prologue_pc);
> +    }
>  
>    cache.locals = -1;
>    pc = i386_analyze_prologue (gdbarch, start_pc, 0xffffffff, &cache);
> 
> 
> I wanted to add this patch in GDB to fix a problem which we are
> currently facing when we use gdb with binary compiled with clang.
> The problem faced is clang generates function prologue which is a
> bit different from that of GCC as a result when we try to skip
> prologue instruction by instruction it results in incorrect
> prologue_end.

It should only ever result in a prologue_end that's pointing to an
instruction before the "real" end of the prologue.  That shouldn't be
a big issue if your compiler emits proper debug information (in
particular unwind information adn location information) for the
prologue.  With today's optimizing compilers the concept of function
prologue is fuzzy anyway.

> There is one more method to skip prologue which is used in other
> architectures such as ARM(arm-tdep.c), MIPS(mips-tdep.c) etc. In
> this method we try to determine prologue end via symbol table.  If
> we are unable to do this we then we examine instruction to determine
> prologue end.

The problem with that approach is that compilers can not always be
trusted to emit the right information for this to work.  In the past
GCC has been particularly flaky in this respect, with the unfortunate
outcome that there were branch instructions before the the prologue
end as determined via the symbol table.  That makes debugging really,
really painful.

If you can vouch for clang always getting this right, I have no
objection doing this when clang is the producer.  Perhaps these days
GCC can be trusted as well.  But we'd need a version check to make
sure we don't use the symbol table approach on known to be broken
versions of GCC.  Probably the best thing would be to establish a
known-to-be-good version of GCC and only use the symbol table approach
for GCC starting with that version number.

If your immediate goal is to fix things for clang, I recommend you
resubmit your change addressing only clang and worry about GCC later
(or let somebody else worry about it).  Be sure to update the comment.
I'd simple replace it with something like "We trust clang to emit
usable line notes".

You probably want to add similar code to the prologue skipping code in
amd64-tdep.c.

Cheers,

Mark