Re: [patch 2/2] Disable epilogue unwinders on recent GCCs [Re: Regression: Re: [PATCH] Fix some i386 unwinder inconcistencies]

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

> Date: Sun, 26 Jun 2011 10:41:41 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> On Mon, 13 Jun 2011 12:49:11 +0200, Jan Kratochvil wrote:
> > On all the tested platforms Fedora-{13,14,15,Rawhide} for {i686,x86_64-m32}
> > (but not for x86_64):
> > -PASS: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> > +FAIL: gdb.base/watchpoint-cond-gone.exp: Catch the no longer valid watchpoint
> > -XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (stopped at wrong place)
> > +XFAIL: gdb.mi/mi-watch.exp: sw: watchpoint trigger (unknown output after running)
> > -XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (stopped at wrong place)
> > +XFAIL: gdb.mi/mi2-watch.exp: sw: watchpoint trigger (unknown output after running)
> 
> On recent GCCs:
> 	http://gcc.gnu.org/gcc-4.5/changes.html
> 	GCC now generates unwind info also for epilogues.
> 
> The epilogue unwinders are therefore no longer useful in common cases.

Good news!  The epilogue unwinders will still be useful for debugging
code without debug information.

> Moreover as they precede the DWARF unwinder they prevent
> archer-jankratochvil-entryval (to be submitted these days) functionality at
> those return instructions as entryval can provide more constructed parameters
> debug info even at those PCs.  It is a bit offtopic now but it causes:
> 
> 
> (gdb) bt
> #0  d (i=70) at tailcall.c:6
>        ^^^^ value provided by archer-jankratochvil-entryval
> #1  0x00000000004004ba in c (i=7) at tailcall.c:8
>     ^^^^^^^^^^^^^^^^^^^^^^^ frame provided by archer-jankratochvil-entryval
> #2  0x00000000004004d8 in b (i=5) at tailcall.c:12
>     ^^^^^^^^^^^^^^^^^^^^^^^ frame provided by archer-jankratochvil-entryval
> #3  0x0000000000400384 in main () at tailcall.c:15
> (gdb) disass
> Dump of assembler code for function d:
>    0x0000000000400490 <+0>:	callq  0x400480 <e>
>    0x0000000000400495 <+5>:	mov    0x20046d(%rip),%edi        # 0x600908 <v>
>    0x000000000040049b <+11>:	callq  0x400480 <e>
> => 0x00000000004004a0 <+16>:	xor    %eax,%eax
>    0x00000000004004a2 <+18>:	retq   
> End of assembler dump.
> (gdb) bt
> #0  d (i=70) at tailcall.c:6
> #1  0x00000000004004ba in c (i=7) at tailcall.c:8
> #2  0x00000000004004d8 in b (i=5) at tailcall.c:12
> #3  0x0000000000400384 in main () at tailcall.c:15

Interesting.  Don't see how you can reliably do that, but I'm looking
forward to seeing your patch.  Tail-call optimizations often confuse
GDB users.

> I find questionable this detection uses DWARF DW_AT_producer.
> Therefore on binaries with .eh_frame but no (DWARF) debug info the
> detection fails and epilogue unwinders will get into affect.

I think it is an acceptable compromise.  People should expect some
lost functionality when they strip their binaries.  

> 1441      /* This restriction could be lifted if other unwinders are known to
> 1442         compute the frame base in a way compatible with the DWARF
> 1443         unwinder.  */
> 1444      if (! frame_unwinder_is (this_frame, &dwarf2_frame_unwind))
> 1445        error (_("can't compute CFA for this frame"));

I still think, this code should be removed.  Tom, since you added that
bit, what's your take on that?

> No regressions on {x86_64,x86_64-m32,i686}-fedora15-linux-gnu, it fixes the
> regression reported above.  I will check it in some time if no comments
> appear.

Implementation makes sense to me.  So go ahead once 1/2 is approved
(not really in my area).

> gdb/
> 2011-06-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Disable epilogue unwinders on recent GCCs.
> 	* amd64-tdep.c (amd64_in_function_epilogue_p): New variable symtab,
> 	initialize it, return 0 on EPILOGUE_UNWIND_VALID.
> 	* dwarf2read.c (process_full_comp_unit): Initialize
> 	EPILOGUE_UNWIND_VALID.
> 	* i386-tdep.c (i386_in_function_epilogue_p): New variable symtab,
> 	initialize it, return 0 on EPILOGUE_UNWIND_VALID.
> 	* symtab.h (struct symtab): New field epilogue_unwind_valid.
> 
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -2219,6 +2219,11 @@ static int
>  amd64_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>  {
>    gdb_byte insn;
> +  struct symtab *symtab;
> +
> +  symtab = find_pc_symtab (pc);
> +  if (symtab && symtab->epilogue_unwind_valid)
> +    return 0;
>  
>    if (target_read_memory (pc, &insn, 1))
>      return 0;   /* Can't read memory at pc.  */
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -4751,6 +4751,9 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu)
>  	 */ 
>        if (cu->has_loclist && gcc_4_minor >= 0)
>  	symtab->locations_valid = 1;
> +
> +      if (gcc_4_minor >= 5)
> +	symtab->epilogue_unwind_valid = 1;
>      }
>  
>    if (dwarf2_per_objfile->using_index)
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -1885,6 +1885,11 @@ static int
>  i386_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>  {
>    gdb_byte insn;
> +  struct symtab *symtab;
> +
> +  symtab = find_pc_symtab (pc);
> +  if (symtab && symtab->epilogue_unwind_valid)
> +    return 0;
>  
>    if (target_read_memory (pc, &insn, 1))
>      return 0;	/* Can't read memory at pc.  */
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -779,6 +779,11 @@ struct symtab
>  
>    unsigned int locations_valid : 1;
>  
> +  /* DWARF unwinder for this CU is valid even for epilogues (PC at the return
> +     instruction).  This is supported by GCC since 4.5.0.  */
> +
> +  unsigned int epilogue_unwind_valid : 1;
> +
>    /* The macro table for this symtab.  Like the blockvector, this
>       may be shared between different symtabs --- and normally is for
>       all the symtabs in a given compilation unit.  */
>