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

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

[KARTHIKVENKATESH BHAT]

Dear All,
I wanted to add a patch in i386-tdep.c .  Similar to what is done in other architectures such as ARM,
instead of actually going through the complete prologue if we can use the symbol table information to resolve prologue end.


Index: gdb/i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.362
diff -u -p -r1.362 i386-tdep.c
--- gdb/i386-tdep.c	12 Nov 2012 21:59:06 -0000	1.362
+++ gdb/i386-tdep.c	19 Nov 2012 07:56:45 -0000
@@ -1582,8 +1582,30 @@ i386_skip_prologue (struct gdbarch *gdba
   CORE_ADDR pc;
   gdb_byte op;
   int i;
+  cache.locals = -1; 
+  CORE_ADDR func_addr;
+  struct symtab *s = find_pc_symtab (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);
+    
+    /* GCC 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);
+  }
+
 

Please let me know your i/p if we can push this onto mainstream.

Thanks
Karthik

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

[H.J. Lu]

On Mon, Nov 19, 2012 at 12:06 AM, KARTHIKVENKATESH BHAT
<kv.bhat@samsung.com> wrote:
> Dear All,
> I wanted to add a patch in i386-tdep.c .  Similar to what is done in other architectures such as ARM,
> instead of actually going through the complete prologue if we can use the symbol table information to resolve prologue end.
>
>
> Index: gdb/i386-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/i386-tdep.c,v
> retrieving revision 1.362
> diff -u -p -r1.362 i386-tdep.c
> --- gdb/i386-tdep.c     12 Nov 2012 21:59:06 -0000      1.362
> +++ gdb/i386-tdep.c     19 Nov 2012 07:56:45 -0000
> @@ -1582,8 +1582,30 @@ i386_skip_prologue (struct gdbarch *gdba
>    CORE_ADDR pc;
>    gdb_byte op;
>    int i;
> +  cache.locals = -1;
> +  CORE_ADDR func_addr;
> +  struct symtab *s = find_pc_symtab (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);
> +
> +    /* GCC 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);
> +  }
> +
>
>

It doesn't look like target-dependent.  If we do this, why not
make it a target-independent function make all targets call it?


-- 
H.J.

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

[Tom Tromey]

>>>>> "Karthik" == KARTHIKVENKATESH BHAT <kv.bhat@samsung.com> writes:

Karthik> I wanted to add a patch in i386-tdep.c .  Similar to what is done in
Karthik> other architectures such as ARM,
Karthik> instead of actually going through the complete prologue if we can use
Karthik> the symbol table information to resolve prologue end.

I'm not the person to review this patch, but I did notice that it is
missing a ChangeLog entry and also a test case.

Karthik> +	      || strncmp (s->producer, "GNU ", sizeof ("GNU ") - 1) == 0
Karthik> +	      || strncmp (s->producer, "clang ", sizeof ("clang ") - 1) == 0))

Is there any other way?

Tom

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

[Doug Evans]

Mark Kettenis writes:
 > > Date: Tue, 04 Dec 2012 06:09:29 +0000 (GMT)
 > > From: KARTHIKVENKATESH BHAT <kv.bhat@samsung.com>
 > > 
 > > Hi Mark/Tom/Palves,
 > > Thanks for taking your time out for review.
 > > I have implemented the review comments. Please let me know if i can commit the same.
 > > 
 > > cvs diff: Diffing .
 > > Index: ChangeLog
 > > ===================================================================
 > > RCS file: /cvs/src/src/gdb/ChangeLog,v
 > > retrieving revision 1.14880
 > > diff -u -p -r1.14880 ChangeLog
 > > --- ChangeLog	3 Dec 2012 22:31:02 -0000	1.14880
 > > +++ ChangeLog	4 Dec 2012 05:06:29 -0000
 > > @@ -1,3 +1,10 @@
 > > +2012-12-04  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.
 > 
 > Missing space between : and Using in your ChangeLog entry.  Otherwise
 > this looks ok to me.
 > 
 > > 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	4 Dec 2012 05:06:30 -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	4 Dec 2012 05:06:30 -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)
 > > 

This indentation looks wonky.

Ok to check in?

2012-12-04  Doug Evans  <dje@google.com>

	* amd64-tdep.c (amd64_skip_prologue): Fix indentation.
	* i386-tdep.c (i386_skip_prologue): Fix indentation.

Index: amd64-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/amd64-tdep.c,v
retrieving revision 1.116
diff -u -p -r1.116 amd64-tdep.c
--- amd64-tdep.c	4 Dec 2012 07:43:19 -0000	1.116
+++ amd64-tdep.c	4 Dec 2012 23:18:19 -0000
@@ -2263,9 +2263,9 @@ amd64_skip_prologue (struct gdbarch *gdb
       /* 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))
+	  && s != NULL
+	  && s->producer != NULL
+	  && strncmp (s->producer, "clang ", sizeof ("clang ") - 1) == 0)
         return max (start_pc, post_prologue_pc);
     }
 
Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.365
diff -u -p -r1.365 i386-tdep.c
--- i386-tdep.c	4 Dec 2012 07:43:19 -0000	1.365
+++ i386-tdep.c	4 Dec 2012 23:18:19 -0000
@@ -1593,9 +1593,9 @@ i386_skip_prologue (struct gdbarch *gdba
       /* 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))
+	  && s != NULL
+	  && s->producer != NULL
+	  && strncmp (s->producer, "clang ", sizeof ("clang ") - 1) == 0)
         return max (start_pc, post_prologue_pc);
     }
  

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

[Tom Tromey]

>>>>> "Karthik" == KARTHIKVENKATESH BHAT <kv.bhat@samsung.com> writes:

Karthik> Please find the Changes after comments -

There are some formatting issues in the patch.

Karthik> +      /* Clang always emits a line note before the prologue and another
Karthik> +	 one after.We trust clang to emit usable line notes  */

Two spaces after the ".".
A "." followed by 2 spaces at the end of the second sentence.

Karthik> +        return max(start_pc,post_prologue_pc);

Space before open paren and after the comma.

Tom

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

[Mark Kettenis]

> Date: Wed, 21 Nov 2012 15:39:22 +0000 (GMT)
> From: KARTHIKVENKATESH BHAT <kv.bhat@samsung.com>
> 
> 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  <kv.bhat@samsung.com>
> +
> +	* 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  <yao@codesourcery.com>
>  
>  	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

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

[Pedro Alves]

On 11/21/2012 03:39 PM, KARTHIKVENKATESH BHAT wrote:

> Thanks Pedro/Mark. Appologies for the build break. I'm a bit new to GDB community will take care of it from next time.

No problem.

Each of your recent replies breaks the email threading, as if you composed
a new email, instead of replying to a previous message -- AFAICS, your replies
miss an In-Reply-To header.  Is there any way you could fix that?

E.g., see http://sourceware.org/ml/gdb-patches/2012-11/ and notice how
your replies don't have "References: " links at the bottom, unlike
other replies on other threads, e.g.,
<http://sourceware.org/ml/gdb-patches/2012-11/msg00569.html>.

(You can follow the "Raw text" link to see the email headers of your
replies.)

-- 
Pedro Alves

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

[Pedro Alves]

On 11/21/2012 01:20 PM, Mark Kettenis wrote:

> Please back it out.

Since this breaking the build I went ahead and reverted it.

-- 
Pedro Alves

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

[Mark Kettenis]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

> Date: Wed, 21 Nov 2012 13:10:55 +0000
> From: Pedro Alves <palves@redhat.com>
> 
> I updated my checkout this morning and stumbled on:
> 
> ../../src/gdb/i386-tdep.c: In function ‘i386_skip_prologue’:
> ../../src/gdb/i386-tdep.c:1586:3: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
> cc1: all warnings being treated as errors
> 
> I also noticed the indentation of the if block is not according to
> the convention.
> 
> I can't find an email approving the change.  Was there some private
> discussion perhaps?

I did not approve that change.  I haven't had the time to properly
review the change yet, but it is almost certainly wrong.  Please back
it out.

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

[Pedro Alves]

I updated my checkout this morning and stumbled on:

../../src/gdb/i386-tdep.c: In function ‘i386_skip_prologue’:
../../src/gdb/i386-tdep.c:1586:3: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
cc1: all warnings being treated as errors

I also noticed the indentation of the if block is not according to
the convention.

I can't find an email approving the change.  Was there some private discussion
perhaps?

-- 
Pedro Alves