[RFC] PR gdb/17445 fix

[RFC] PR gdb/17445 fix

[Pierre Muller]

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

  Bug report gdb/17445
shows that use of explicit xmm15 register in windows x86_64
code leads to wrong unwinding of stacktrace by GDB.
  This problem comes from the fact that
the return address column is set to 32 for x86_64 pe objects,
while it is 16 for other targets.
  Dwarf x86_64 register 16 is RIP,
while register 32 is XMM15.
  The reason for this was apparently that the value of
the return address column is also interpreted as the highest
index of the register that needs to be saved according to the ABI 
which is indeed different for Microsoft.
  Nevertheless, I found nothing inside DWARF4 documentation
that makes any relation between the resisters that should be saved
and the return address column.
  I came to the conclusion that this is a mis-interpretation
of the dwarf standard that is specific to GNU bfd-gas-gdb.
  The patch proposed below tries to fix the current PR
by removing the above assumptions and restoring RIP
as return address register for pe(i)-x86-64 targets.

 Comments most welcome,

Pierre Muller
Pascal language maintainer for GDB.

PS: One problem is that I was not able to correctly run
the testsuite before and after my patch, as
there are still cygwin/mingw specific issues with testsuite runs.


ChangeLog (needs to be split into binutils/gas/gdb directories)

2014-10-01  Pierre Muller  <muller@sourceware.org>

        PR gdb/17445
        * binutils/dwarf.c (display_debug_frames): Handle return address
column
        specifically, do not limit num_regs to the value of return address
        column.
        Modified handling of DW_CFA_restore{_extended} to check that
        a valid storage is present at restore point.

        * gas/config/tc-i386.c (x86_dwarf2_return_column): Remove special
        value for x86_64 pe coff return address column.
        This restores RIP register for return address column.

        * gdb/dwarf2-frame.c (dwarf2_frame_cache): Avoid double handling of
        return address column.


[-- Attachment #2: fix-PR-17445-v5.patch --]
[-- Type: application/octet-stream, Size: 6253 bytes --]

2014-10-01  Pierre Muller  <muller@sourceware.org>

	PR gdb/17445
	* binutils/dwarf.c (display_debug_frames): Handle return address column
	specifically, do not limit num_regs to the value of return address
	column.
	Modified handling of DW_CFA_restore{_extended} to check that
	a valid storage is present at restore point.

	* gas/config/tc-i386.c (x86_dwarf2_return_column): Remove special
	value for x86_64 pe coff return address column.
	This restores RIP register for return address column.

	* gdb/dwarf2-frame.c (dwarf2_frame_cache): Avoid double handling of
	return address column.

diff --git a/binutils/dwarf.c b/binutils/dwarf.c
index 3f4095a..8e903dc 100644
--- a/binutils/dwarf.c
+++ b/binutils/dwarf.c
@@ -5337,6 +5337,7 @@ display_debug_frames (struct dwarf_section *section,
   unsigned int length_return;
   int max_regs = 0;
   const char *bad_reg = _("bad register: ");
+  const char *bad_cfa = _("bad cfa: ");
   int saved_eh_addr_size = eh_addr_size;
 
   printf (_("Contents of the %s section:\n"), section->name);
@@ -5632,8 +5633,8 @@ display_debug_frames (struct dwarf_section *section,
 		    fc->col_type[opa] = DW_CFA_undefined;
 		  break;
 		case DW_CFA_restore:
-		  if (frame_need_space (fc, opa) >= 0)
-		    fc->col_type[opa] = DW_CFA_undefined;
+		  /* This reg must be valid before, so
+		     do not increase size of fc here.  */
 		  break;
 		case DW_CFA_set_loc:
 		  start += encoded_ptr_size;
@@ -5655,9 +5656,8 @@ display_debug_frames (struct dwarf_section *section,
 		  break;
 		case DW_CFA_restore_extended:
 		  reg = LEB ();
-		  frame_need_space (fc, reg);
-		  if (frame_need_space (fc, reg) >= 0)
-		    fc->col_type[reg] = DW_CFA_undefined;
+		  /* This reg must be valid before, so
+		     do not increase size of fc here.  */
 		  break;
 		case DW_CFA_undefined:
 		  reg = LEB ();
@@ -5735,8 +5735,10 @@ display_debug_frames (struct dwarf_section *section,
 	  long l;
 	  dwarf_vma ofs;
 	  dwarf_vma vma;
-	  const char *reg_prefix = "";
+	  const char *reg_name;
+	  const char *reg_prefix;
 
+	  reg_prefix = "";
 	  op = *start++;
 	  opa = op & 0x3f;
 	  if (op & 0xc0)
@@ -5761,10 +5763,16 @@ display_debug_frames (struct dwarf_section *section,
 	    case DW_CFA_offset:
 	      roffs = LEB ();
 	      if (opa >= (unsigned int) fc->ncols)
-		reg_prefix = bad_reg;
+		{
+		  reg_prefix = bad_reg;
+		}
+	      if (opa == (unsigned int) fc->ra)
+		reg_name = "RA ";
+	      else
+		reg_name = regname (opa, 0);
 	      if (! do_debug_frames_interp || *reg_prefix != '\0')
 		printf ("  DW_CFA_offset: %s%s at cfa%+ld\n",
-			reg_prefix, regname (opa, 0),
+			reg_prefix, reg_name,
 			roffs * fc->data_factor);
 	      if (*reg_prefix == '\0')
 		{
@@ -5774,19 +5782,29 @@ display_debug_frames (struct dwarf_section *section,
 	      break;
 
 	    case DW_CFA_restore:
-	      if (opa >= (unsigned int) cie->ncols
-		  || opa >= (unsigned int) fc->ncols)
+	      if (/* opa >= (unsigned int) cie->ncols
+		  || */ opa >= (unsigned int) fc->ncols)
 		reg_prefix = bad_reg;
+	      else if (fc->col_type[opa] == DW_CFA_undefined)
+		reg_prefix = bad_cfa;
 	      if (! do_debug_frames_interp || *reg_prefix != '\0')
 		printf ("  DW_CFA_restore: %s%s\n",
 			reg_prefix, regname (opa, 0));
 	      if (*reg_prefix == '\0')
 		{
-		  fc->col_type[opa] = cie->col_type[opa];
-		  fc->col_offset[opa] = cie->col_offset[opa];
-		  if (do_debug_frames_interp
-		      && fc->col_type[opa] == DW_CFA_unreferenced)
-		    fc->col_type[opa] = DW_CFA_undefined;
+		  if (opa <= (unsigned int) cie->ncols)
+		    {
+		      fc->col_type[opa] = cie->col_type[opa];
+		      fc->col_offset[opa] = cie->col_offset[opa];
+		      if (do_debug_frames_interp
+			  && fc->col_type[opa] == DW_CFA_unreferenced)
+			fc->col_type[opa] = DW_CFA_undefined;
+		    }
+		  else
+		    {
+		      fc->col_type[opa] = DW_CFA_undefined;
+		      fc->col_offset[opa] = 0;
+		    }
 		}
 	      break;
 
@@ -5874,16 +5892,26 @@ display_debug_frames (struct dwarf_section *section,
 
 	    case DW_CFA_restore_extended:
 	      reg = LEB ();
-	      if (reg >= (unsigned int) cie->ncols
-		  || reg >= (unsigned int) fc->ncols)
+	      if (/*reg >= (unsigned int) cie->ncols
+		  || */reg >= (unsigned int) fc->ncols)
 		reg_prefix = bad_reg;
+	      else if (fc->col_type[reg] == DW_CFA_undefined)
+		reg_prefix = bad_cfa;
 	      if (! do_debug_frames_interp || *reg_prefix != '\0')
 		printf ("  DW_CFA_restore_extended: %s%s\n",
 			reg_prefix, regname (reg, 0));
 	      if (*reg_prefix == '\0')
 		{
-		  fc->col_type[reg] = cie->col_type[reg];
-		  fc->col_offset[reg] = cie->col_offset[reg];
+		  if (reg < (unsigned int) cie->ncols)
+		    {
+		      fc->col_type[reg] = cie->col_type[reg];
+		      fc->col_offset[reg] = cie->col_offset[reg];
+		    }
+		  else
+		    {
+		      fc->col_type[opa] = DW_CFA_undefined;
+		      fc->col_offset[opa] = 0;
+		    }
 		}
 	      break;
 
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 68ca7e4..fde7eba 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -2632,7 +2632,9 @@ md_begin (void)
 
   if (flag_code == CODE_64BIT)
     {
-#if defined (OBJ_COFF) && defined (TE_PE)
+#if 0
+      // defined (OBJ_COFF) && defined (TE_PE)
+      // This was wrong
       x86_dwarf2_return_column = (OUTPUT_FLAVOR == bfd_target_coff_flavour
 				  ? 32 : 16);
 #else
diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index f2330d9..5e17da6 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1160,6 +1160,11 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
 	/* Use the GDB register number as the destination index.  */
 	int regnum = gdbarch_dwarf2_reg_to_regnum (gdbarch, column);
 
+	/* Do not handle return address here, it is done in the next
+	   loop. This also avoids problem with pei-x86-64 target
+	   having retaddr_column set to 32 (which is xmm15). */
+	if (column == fs->retaddr_column)
+	  continue;
 	/* If there's no corresponding GDB register, ignore it.  */
 	if (regnum < 0 || regnum >= num_regs)
 	  continue;

PING [PATCH RFC] PR gdb/17445 fix

[Pierre Muller]

  Nobody reacted yet to my first email.

  This email is about a fix to:
  https://sourceware.org/bugzilla/show_bug.cgi?id=17445
The bug report is about a problem that arises if you
use xmm15 a register variable in windows 64-bit code.

  The current GDB code overlaps xmm15 and pc saved addresses,
which leads a wrong backtrace.

  
  The problem relates to an older thread
https://sourceware.org/ml/binutils/2011-01/msg00323.html
and
https://sourceware.org/ml/binutils/2013-12/msg00232.html

  which mainly involved Kai.

Kai, did you see my previous email?
Could you comment on my analysis of the problem.

Pierre Muller

> -----Message d'origine-----
> De : binutils-owner@sourceware.org [mailto:binutils-
> owner@sourceware.org] De la part de Pierre Muller
> Envoyé : mercredi 1 octobre 2014 09:46
> À : binutils@sourceware.org; gdb-patches@sourceware.org
> Objet : [RFC] PR gdb/17445 fix
> 
>   Bug report gdb/17445
> shows that use of explicit xmm15 register in windows x86_64
> code leads to wrong unwinding of stacktrace by GDB.
>   This problem comes from the fact that
> the return address column is set to 32 for x86_64 pe objects,
> while it is 16 for other targets.
>   Dwarf x86_64 register 16 is RIP,
> while register 32 is XMM15.
>   The reason for this was apparently that the value of
> the return address column is also interpreted as the highest
> index of the register that needs to be saved according to the ABI
> which is indeed different for Microsoft.
>   Nevertheless, I found nothing inside DWARF4 documentation
> that makes any relation between the resisters that should be saved
> and the return address column.
>   I came to the conclusion that this is a mis-interpretation
> of the dwarf standard that is specific to GNU bfd-gas-gdb.
>   The patch proposed below tries to fix the current PR
> by removing the above assumptions and restoring RIP
> as return address register for pe(i)-x86-64 targets.
> 
>  Comments most welcome,
> 
> Pierre Muller
> Pascal language maintainer for GDB.
> 
> PS: One problem is that I was not able to correctly run
> the testsuite before and after my patch, as
> there are still cygwin/mingw specific issues with testsuite runs.
> 
> 
> ChangeLog (needs to be split into binutils/gas/gdb directories)
> 
> 2014-10-01  Pierre Muller  <muller@sourceware.org>
> 
>         PR gdb/17445
>         * binutils/dwarf.c (display_debug_frames): Handle return
> address
> column
>         specifically, do not limit num_regs to the value of return
> address
>         column.
>         Modified handling of DW_CFA_restore{_extended} to check that
>         a valid storage is present at restore point.
> 
>         * gas/config/tc-i386.c (x86_dwarf2_return_column): Remove
> special
>         value for x86_64 pe coff return address column.
>         This restores RIP register for return address column.
> 
>         * gdb/dwarf2-frame.c (dwarf2_frame_cache): Avoid double
> handling of
>         return address column.

Re: [RFC] PR gdb/17445 fix

[Ulrich Weigand]

Pierre Muller wrote:

> 	* gdb/dwarf2-frame.c (dwarf2_frame_cache): Avoid double handling of
> 	return address column.

Commenting just on the GDB part, this seems incorrect:

>  	/* Use the GDB register number as the destination index.  */
>  	int regnum = gdbarch_dwarf2_reg_to_regnum (gdbarch, column);
> 
> +	/* Do not handle return address here, it is done in the next
> +	   loop. This also avoids problem with pei-x86-64 target
> +	   having retaddr_column set to 32 (which is xmm15). */
> +	if (column == fs->retaddr_column)
> +	  continue;
>  	/* If there's no corresponding GDB register, ignore it.  */
>  	if (regnum < 0 || regnum >= num_regs)
>  	  continue;

The "next loop" does not handle the return address.  It handles *other*
registers that *refer* to the return address.

Usually, you have a "return address column" in DWARF, which may or may
not itself refer to a register of the platform, and then you have some
other rule (either explicit or implicitly defined by the ABI) where the
PC refers to the return address column.  Conceptually, unwinding is
performed in two parts: first, the return address is computed following
the instructions for the return address column, and then the PC is set
to the unwound return address value (or some value derived from it).

On some platforms, the return address itself does not correspond to any
register, and is *only* used for setting the PC.  On some other platforms,
the return address itself corresponds to an actual hardware register, and
the unwind instructions for the return address column are in fact also
used to determine the unwound value of that register.

The latter is the case e.g. on s390, where %r14 is used as return address
column.  The unwind instructions for %r14 are used both to unwind the
actual value of %r14, and to unwind the value of the PC.

Your patch would break that usage since you now no longer apply the
contents of the return address column to unwind register %r14 on s390.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com