diff mbox

Backport PR47324 to gcc 4.5 branch

Message ID 20110214115359.GA9219@bromo.med.uc.edu
State New
Headers show

Commit Message

Jack Howarth Feb. 14, 2011, 11:53 a.m. UTC 
The attached patch backports the missing usage of the DWARF2_FRAME_REG_OUT
macro in gcc/dwarf2out.c for gcc 4.5.3. This bug is latent in gcc-4_5-branch
to the absence of...

r160124 | hubicka | 2010-06-01 17:55:49 -0400 (Tue, 01 Jun 2010) | 4 lines

        * ipa-pure-const.c (local_pure_const): Do NORETURN discovery.

        * testsuite/gcc.dg/noreturn-8.c: New testcase.

Since the existing code is clearly wrong not utilize DWARF2_FRAME_REG_OUT, this
r170077 should be backported. Otherwise, can we really be certain that the 
testsuite is comprehensive enough to capture any potential failures from
the omission of these DWARF2_FRAME_REG_OUT macro calls? Bootstrap and 
regression tested on gcc-4_5-branch. Okay for gcc 4.5.3?
            Jack


2011-02-13  Jack Howarth  <howarth@bromo.med.uc.edu>

	Backport from mainline
	2011-02-12  Mike Stump  <mikestump@comcast.net>
	            Jakub Jelinek  <jakub@redhat.com>
		    Iain Sandoe  <iains@gcc.gnu.org>

        PR target/47324
        * dwarf2out.c (output_cfa_loc): When required, apply the
        DWARF2_FRAME_REG_OUT macro to adjust register numbers.
        (output_loc_sequence): Likewise.
        (output_loc_operands_raw): Likewise.
        (output_loc_sequence_raw): Likewise.
        (output_cfa_loc): Likewise.
        (output_loc_list): Suppress register number adjustment when
        calling output_loc_sequence()
        (output_die): Likewise.

Comments

Jason Merrill Feb. 16, 2011, 10:17 p.m. UTC | #1 
On 02/14/2011 06:53 AM, Jack Howarth wrote:
>     The attached patch backports the missing usage of the DWARF2_FRAME_REG_OUT
> macro in gcc/dwarf2out.c for gcc 4.5.3. This bug is latent in gcc-4_5-branch
> to the absence of...

Hmm, since as you say this bug is latent in 4.5, it can't be a 
regression, and therefore doesn't really seem suitable for that branch.

Jason
Jack Howarth Feb. 16, 2011, 10:33 p.m. UTC | #2 
On Wed, Feb 16, 2011 at 05:17:35PM -0500, Jason Merrill wrote:
> On 02/14/2011 06:53 AM, Jack Howarth wrote:
>>     The attached patch backports the missing usage of the DWARF2_FRAME_REG_OUT
>> macro in gcc/dwarf2out.c for gcc 4.5.3. This bug is latent in gcc-4_5-branch
>> to the absence of...
>
> Hmm, since as you say this bug is latent in 4.5, it can't be a  
> regression, and therefore doesn't really seem suitable for that branch.

  Latent in the sense that we don't have a test case in hand that triggers
the wrong code dwarf eh generation. However this requires an execution test
so it doesn't mean such code doesn't exist in the wild. In a sense, this should
fall under the obvious rule because darwin requires use of DWARF2_FRAME_REG_OUT
on any registers emitted in the dwarf for eh on i386.
               Jack

>
> Jason
Jack Howarth Feb. 16, 2011, 11:51 p.m. UTC | #3 
On Wed, Feb 16, 2011 at 05:17:35PM -0500, Jason Merrill wrote:
> On 02/14/2011 06:53 AM, Jack Howarth wrote:
>>     The attached patch backports the missing usage of the DWARF2_FRAME_REG_OUT
>> macro in gcc/dwarf2out.c for gcc 4.5.3. This bug is latent in gcc-4_5-branch
>> to the absence of...
>
> Hmm, since as you say this bug is latent in 4.5, it can't be a  
> regression, and therefore doesn't really seem suitable for that branch.
>
> Jason

Jason,
  I would also point out that historically darwin has been deemed weird because
of the difficulties with its system unwinder under FSF gcc. This can be seen from
the fact that these failures previously existed in gcc trunk until at least r152966.

http://gcc.gnu.org/ml/gcc-testresults/2009-09/msg02532.html
http://gcc.gnu.org/ml/gcc-testresults/2009-10/msg01825.html

after which they went latent for a period. A full regression test reveals that the breakage
fixed by...

Author: iains
Date: Sat Feb 12 16:51:58 2011
New Revision: 170077

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=170077
Log:

gcc:
    PR target/47324
    * dwarf2out.c (output_cfa_loc): When required, apply the
    DWARF2_FRAME_REG_OUT macro to adjust register numbers.
    (output_loc_sequence): Likewise.
    (output_loc_operands_raw): Likewise.
    (output_loc_sequence_raw): Likewise.
    (output_cfa_loc): Likewise.
    (output_loc_list): Suppress register number adjustment when
    calling output_loc_sequence()
    (output_die): Likewise.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/dwarf2out.c

was actually triggered in gcc trunk by...

r160124 | hubicka | 2010-06-01 17:55:49 -0400 (Tue, 01 Jun 2010) | 4 lines

        * ipa-pure-const.c (local_pure_const): Do NORETURN discovery.

        * testsuite/gcc.dg/noreturn-8.c: New testcase.

which is not present on gcc-4_5-branch. So we can view this issue two ways.
The first is to claim that because we don't see it failing testcases in gcc-4_5-branch
that no problem can exist. The second is to recognize that the existing code
fixed in gcc trunk also exists in gcc branch and that these failures have a
history of going latent and re-appearing later. Darwin should not be emitting
incorrect registers in its dwarf2 code for eh. Whether we have a test case in
hand to demonstrate this for gcc 4.5.3 is orthogonal to the problem. Given its
history of periodic latency and the limited eh execution testcases in the
gcc testsuite it is cold comfort to assume it won't be triggered in some 
existing code.
               Jack
Jason Merrill Feb. 17, 2011, 2:32 a.m. UTC | #4 
I'm certainly sympathetic; this seems like a problem that is definitely 
worth fixing, I'm just concerned about the potential for unforseen 
consequences.  I think I'll defer to the release managers on this question.

Jason
Richard Biener Feb. 17, 2011, 9:41 a.m. UTC | #5 
On Thu, Feb 17, 2011 at 3:32 AM, Jason Merrill <jason@redhat.com> wrote:
> I'm certainly sympathetic; this seems like a problem that is definitely
> worth fixing, I'm just concerned about the potential for unforseen
> consequences.  I think I'll defer to the release managers on this question.

I'd like to defer backporting this to the point where 4.6 is released and we
got wider testing of this patch.

Richard.

> Jason
>
Jack Howarth Feb. 17, 2011, 2:39 p.m. UTC | #6 
On Wed, Feb 16, 2011 at 09:32:33PM -0500, Jason Merrill wrote:
> I'm certainly sympathetic; this seems like a problem that is definitely  
> worth fixing, I'm just concerned about the potential for unforseen  
> consequences.  I think I'll defer to the release managers on this 
> question.
>
> Jason

Jason,
   Our main problem on darwin, since FSF gcc 4.5, is that we now use always
use the system unwinder (obtained from libgcc_s.10.4/10.5 for darwin8/9 or
from libSystem for darwin10). Unfortunately Apple doesn't ship debug versions
of those libraries so it is impossible to walk through the system unwinder
to debug issues. In theory one ought to be able to build the required debug
code from the darwin open source releses from Apple, however in practice this
apparently requires over 20 darwin open source packages to be built first in
an entirely undocumented procedure. So we should be esctatic to have found such
an obvious wrong code generation bug in the FSF gcc unwind code generation.
           Jack
ps While we could file a radar requesting that Apple provide these debug libraries, it
would be problematic since that would have to release new debug libraries with each
system update (which would either require a sister software update package from Apple
or bloat their current software updates).
diff mbox

Patch

Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 170108)
+++ gcc/dwarf2out.c	(working copy)
@@ -475,7 +475,7 @@  static bool clobbers_queued_reg_save (co
 static void dwarf2out_frame_debug_expr (rtx, const char *);
 
 /* Support for complex CFA locations.  */
-static void output_cfa_loc (dw_cfi_ref);
+static void output_cfa_loc (dw_cfi_ref, int);
 static void output_cfa_loc_raw (dw_cfi_ref);
 static void get_cfa_from_loc_descr (dw_cfa_location *,
 				    struct dw_loc_descr_struct *);
@@ -3161,7 +3161,7 @@  output_cfi (dw_cfi_ref cfi, dw_fde_ref f
 
 	case DW_CFA_def_cfa_expression:
 	case DW_CFA_expression:
-	  output_cfa_loc (cfi);
+	  output_cfa_loc (cfi, for_eh);
 	  break;
 
 	case DW_CFA_GNU_negative_offset_extended:
@@ -4835,10 +4835,15 @@  size_of_locs (dw_loc_descr_ref loc)
 static HOST_WIDE_INT extract_int (const unsigned char *, unsigned);
 #endif
 
-/* Output location description stack opcode's operands (if any).  */
+/* Output location description stack opcode's operands (if any).
+   The for_eh_or_skip parameter controls whether register numbers are
+   converted using DWARF2_FRAME_REG_OUT, which is needed in the case that
+   hard reg numbers have been processed via DWARF_FRAME_REGNUM (i.e. for unwind
+   info).  This should be suppressed for the cases that have not been converted
+   (i.e. symbolic debug info), by setting the parameter < 0.  See PR47324.  */
 
 static void
-output_loc_operands (dw_loc_descr_ref loc)
+output_loc_operands (dw_loc_descr_ref loc, int for_eh_or_skip)
 {
   dw_val_ref val1 = &loc->dw_loc_oprnd1;
   dw_val_ref val2 = &loc->dw_loc_oprnd2;
@@ -4991,14 +4996,28 @@  output_loc_operands (dw_loc_descr_ref lo
       dw2_asm_output_data_sleb128 (val1->v.val_int, NULL);
       break;
     case DW_OP_regx:
-      dw2_asm_output_data_uleb128 (val1->v.val_unsigned, NULL);
+      {
+	unsigned r = val1->v.val_unsigned;
+	if (for_eh_or_skip >= 0)
+	  r = DWARF2_FRAME_REG_OUT (r, for_eh_or_skip);
+	gcc_assert (size_of_uleb128 (r) 
+		    == size_of_uleb128 (val1->v.val_unsigned));
+	dw2_asm_output_data_uleb128 (r, NULL);	
+      }
       break;
     case DW_OP_fbreg:
       dw2_asm_output_data_sleb128 (val1->v.val_int, NULL);
       break;
     case DW_OP_bregx:
-      dw2_asm_output_data_uleb128 (val1->v.val_unsigned, NULL);
-      dw2_asm_output_data_sleb128 (val2->v.val_int, NULL);
+      {
+	unsigned r = val1->v.val_unsigned;
+	if (for_eh_or_skip >= 0)
+	  r = DWARF2_FRAME_REG_OUT (r, for_eh_or_skip);
+	gcc_assert (size_of_uleb128 (r) 
+		    == size_of_uleb128 (val1->v.val_unsigned));
+	dw2_asm_output_data_uleb128 (r, NULL);	
+	dw2_asm_output_data_sleb128 (val2->v.val_int, NULL);
+      }
       break;
     case DW_OP_piece:
       dw2_asm_output_data_uleb128 (val1->v.val_unsigned, NULL);
@@ -5037,19 +5056,42 @@  output_loc_operands (dw_loc_descr_ref lo
     }
 }
 
-/* Output a sequence of location operations.  */
+/* Output a sequence of location operations.  
+   The for_eh_or_skip parameter controls whether register numbers are
+   converted using DWARF2_FRAME_REG_OUT, which is needed in the case that
+   hard reg numbers have been processed via DWARF_FRAME_REGNUM (i.e. for unwind
+   info).  This should be suppressed for the cases that have not been converted
+   (i.e. symbolic debug info), by setting the parameter < 0.  See PR47324.  */
 
 static void
-output_loc_sequence (dw_loc_descr_ref loc)
+output_loc_sequence (dw_loc_descr_ref loc, int for_eh_or_skip)
 {
   for (; loc != NULL; loc = loc->dw_loc_next)
     {
+      enum dwarf_location_atom opc = loc->dw_loc_opc;
       /* Output the opcode.  */
-      dw2_asm_output_data (1, loc->dw_loc_opc,
-			   "%s", dwarf_stack_op_name (loc->dw_loc_opc));
+      if (for_eh_or_skip >= 0 
+          && opc >= DW_OP_breg0 && opc <= DW_OP_breg31)
+	{
+	  unsigned r = (opc - DW_OP_breg0);
+	  r = DWARF2_FRAME_REG_OUT (r, for_eh_or_skip);
+	  gcc_assert (r <= 31);
+	  opc = (enum dwarf_location_atom) (DW_OP_breg0 + r);
+	}
+      else if (for_eh_or_skip >= 0 
+	       && opc >= DW_OP_reg0 && opc <= DW_OP_reg31)
+	{
+	  unsigned r = (opc - DW_OP_reg0);
+	  r = DWARF2_FRAME_REG_OUT (r, for_eh_or_skip);
+	  gcc_assert (r <= 31);
+	  opc = (enum dwarf_location_atom) (DW_OP_reg0 + r);
+	}
+
+      dw2_asm_output_data (1, opc,
+			     "%s", dwarf_stack_op_name (opc));
 
       /* Output the operand(s) (if any).  */
-      output_loc_operands (loc);
+      output_loc_operands (loc, for_eh_or_skip);
     }
 }
 
@@ -5110,9 +5152,18 @@  output_loc_operands_raw (dw_loc_descr_re
       }
       break;
 
+    case DW_OP_regx:
+      {
+	unsigned r = DWARF2_FRAME_REG_OUT (val1->v.val_unsigned, 1);
+	gcc_assert (size_of_uleb128 (r) 
+		    == size_of_uleb128 (val1->v.val_unsigned));
+	fputc (',', asm_out_file);
+	dw2_asm_output_data_uleb128_raw (r);
+      }
+      break;
+      
     case DW_OP_constu:
     case DW_OP_plus_uconst:
-    case DW_OP_regx:
     case DW_OP_piece:
       fputc (',', asm_out_file);
       dw2_asm_output_data_uleb128_raw (val1->v.val_unsigned);
@@ -5157,10 +5208,15 @@  output_loc_operands_raw (dw_loc_descr_re
       break;
 
     case DW_OP_bregx:
-      fputc (',', asm_out_file);
-      dw2_asm_output_data_uleb128_raw (val1->v.val_unsigned);
-      fputc (',', asm_out_file);
-      dw2_asm_output_data_sleb128_raw (val2->v.val_int);
+      {
+	unsigned r = DWARF2_FRAME_REG_OUT (val1->v.val_unsigned, 1);
+	gcc_assert (size_of_uleb128 (r) 
+		    == size_of_uleb128 (val1->v.val_unsigned));
+	fputc (',', asm_out_file);
+	dw2_asm_output_data_uleb128_raw (r);
+	fputc (',', asm_out_file);
+	dw2_asm_output_data_sleb128_raw (val2->v.val_int);
+      }
       break;
 
     default:
@@ -5175,7 +5231,23 @@  output_loc_sequence_raw (dw_loc_descr_re
   while (1)
     {
       /* Output the opcode.  */
-      fprintf (asm_out_file, "0x%x", loc->dw_loc_opc);
+      enum dwarf_location_atom opc = loc->dw_loc_opc;
+      if (opc >= DW_OP_breg0 && opc <= DW_OP_breg31)
+      {
+        unsigned r = (opc - DW_OP_breg0);
+        r = DWARF2_FRAME_REG_OUT (r, 1);
+        gcc_assert (r <= 31);
+        opc = (enum dwarf_location_atom) (DW_OP_breg0 + r);
+      }
+      else if (opc >= DW_OP_reg0 && opc <= DW_OP_reg31)
+      {
+        unsigned r = (opc - DW_OP_reg0);
+        r = DWARF2_FRAME_REG_OUT (r, 1);
+        gcc_assert (r <= 31);
+        opc = (enum dwarf_location_atom) (DW_OP_reg0 + r);
+      }
+      /* Output the opcode.  */
+      fprintf (asm_out_file, "%#x", opc);
       output_loc_operands_raw (loc);
 
       if (!loc->dw_loc_next)
@@ -5190,14 +5262,16 @@  output_loc_sequence_raw (dw_loc_descr_re
    description based on a cfi entry with a complex address.  */
 
 static void
-output_cfa_loc (dw_cfi_ref cfi)
+output_cfa_loc (dw_cfi_ref cfi, int for_eh)
 {
   dw_loc_descr_ref loc;
   unsigned long size;
 
   if (cfi->dw_cfi_opc == DW_CFA_expression)
     {
-      dw2_asm_output_data (1, cfi->dw_cfi_oprnd1.dw_cfi_reg_num, NULL);
+      unsigned r = 
+	DWARF2_FRAME_REG_OUT (cfi->dw_cfi_oprnd1.dw_cfi_reg_num, for_eh);
+      dw2_asm_output_data (1, r, NULL);
       loc = cfi->dw_cfi_oprnd2.dw_cfi_loc;
     }
   else
@@ -5208,7 +5282,7 @@  output_cfa_loc (dw_cfi_ref cfi)
   dw2_asm_output_data_uleb128 (size, NULL);
 
   /* Now output the operations themselves.  */
-  output_loc_sequence (loc);
+  output_loc_sequence (loc, for_eh);
 }
 
 /* Similar, but used for .cfi_escape.  */
@@ -5221,7 +5295,9 @@  output_cfa_loc_raw (dw_cfi_ref cfi)
 
   if (cfi->dw_cfi_opc == DW_CFA_expression)
     {
-      fprintf (asm_out_file, "0x%x,", cfi->dw_cfi_oprnd1.dw_cfi_reg_num);
+      unsigned r =
+      DWARF2_FRAME_REG_OUT (cfi->dw_cfi_oprnd1.dw_cfi_reg_num, 1);
+      fprintf (asm_out_file, "%#x,", r);
       loc = cfi->dw_cfi_oprnd2.dw_cfi_loc;
     }
   else
@@ -10386,7 +10462,7 @@  output_loc_list (dw_loc_list_ref list_he
       gcc_assert (size <= 0xffff);
       dw2_asm_output_data (2, size, "%s", "Location expression size");
 
-      output_loc_sequence (curr->expr);
+      output_loc_sequence (curr->expr, -1);
     }
 
   dw2_asm_output_data (DWARF2_ADDR_SIZE, 0,
@@ -10464,7 +10540,7 @@  output_die (dw_die_ref die)
 	  else
 	    dw2_asm_output_data (constant_size (size), size, "%s", name);
 
-	  output_loc_sequence (AT_loc (a));
+	  output_loc_sequence (AT_loc (a), -1);
 	  break;
 
 	case dw_val_class_const: