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

[Mark Kettenis <mark dot kettenis at xs4all dot nl>]

> 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