diff mbox

Improve debug info for various cases where we drop location info on the floor (PR debug/55608)

Message ID 20121206191401.GO2315@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Dec. 6, 2012, 7:14 p.m. UTC 
Hi!

This patch adds DW_AT_const_value or DW_AT_location for unused static vars
(thus, not really modified and their DECL_INITIAL can be used for
location/constant value info), and optimizes various cases using
DW_OP_GNU_implicit_pointer (e.g. DW_OP_addr <symbol> DW_OP_stack_value
where symbol is either an optimized away static var (but with DW_AT_location
or DW_AT_const_value on it's DIE), which would be otherwise dropped
to avoid referencing non-existent symbol, can be replaced with
DW_OP_GNU_implicit_pointer to that DIE.  Or, if symbol above is a constant string
literal, we can create DW_TAG_dwarf_procedure with DW_AT_location
DW_OP_implicit_value (and, surprisingly, GDB handles that out of the box).

In cc1plus .debug_loc grew with this by about 3% and .debug_info grew by
0.25%, in libstdc++ the changes were in the noise.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2012-12-06  Jakub Jelinek  <jakub@redhat.com>

	PR debug/55608
	* dwarf2out.c (tree_add_const_value_attribute): Call ggc_free (array)
	on failure.
	(resolve_one_addr): Fail if referenced STRING_CST hasn't been written.
	(string_cst_pool_decl): New function.
	(resolve_addr_in_expr): Optimize DWARF location expression
	DW_OP_addr DW_OP_stack_value where DW_OP_addr refers to some variable
	which doesn't live in memory, but has DW_AT_location or
	DW_AT_const_value, or refers to a string literal, into
	DW_OP_GNU_implicit_pointer.
	(resolve_addr): If removing DW_AT_location of a variable because
	it was DW_OP_addr of address of the variable, but the variable doesn't
	live in memory, try to emit const value attribute for the initializer.


	Jakub

Comments

Cary Coutant Dec. 7, 2012, 7:36 p.m. UTC | #1 
> This patch adds DW_AT_const_value or DW_AT_location for unused static vars
> (thus, not really modified and their DECL_INITIAL can be used for
> location/constant value info), and optimizes various cases using
> DW_OP_GNU_implicit_pointer (e.g. DW_OP_addr <symbol> DW_OP_stack_value
> where symbol is either an optimized away static var (but with DW_AT_location
> or DW_AT_const_value on it's DIE), which would be otherwise dropped
> to avoid referencing non-existent symbol, can be replaced with
> DW_OP_GNU_implicit_pointer to that DIE.  Or, if symbol above is a constant string
> literal, we can create DW_TAG_dwarf_procedure with DW_AT_location
> DW_OP_implicit_value (and, surprisingly, GDB handles that out of the box).
>
> In cc1plus .debug_loc grew with this by about 3% and .debug_info grew by
> 0.25%, in libstdc++ the changes were in the noise.

Based on my reading of PR 55608, this isn't really a regression, and
it's definitely not a regression from 4.7 to 4.8. While the patch
looks OK to me, the size increase in .debug_loc and the scope of the
change suggest that this would be more appropriate for when Stage 1
reopens.

-cary
Jakub Jelinek March 20, 2013, 12:19 p.m. UTC | #2 
On Fri, Dec 07, 2012 at 11:36:24AM -0800, Cary Coutant wrote:
> > This patch adds DW_AT_const_value or DW_AT_location for unused static vars
> > (thus, not really modified and their DECL_INITIAL can be used for
> > location/constant value info), and optimizes various cases using
> > DW_OP_GNU_implicit_pointer (e.g. DW_OP_addr <symbol> DW_OP_stack_value
> > where symbol is either an optimized away static var (but with DW_AT_location
> > or DW_AT_const_value on it's DIE), which would be otherwise dropped
> > to avoid referencing non-existent symbol, can be replaced with
> > DW_OP_GNU_implicit_pointer to that DIE.  Or, if symbol above is a constant string
> > literal, we can create DW_TAG_dwarf_procedure with DW_AT_location
> > DW_OP_implicit_value (and, surprisingly, GDB handles that out of the box).
> >
> > In cc1plus .debug_loc grew with this by about 3% and .debug_info grew by
> > 0.25%, in libstdc++ the changes were in the noise.
> 
> Based on my reading of PR 55608, this isn't really a regression, and
> it's definitely not a regression from 4.7 to 4.8. While the patch
> looks OK to me, the size increase in .debug_loc and the scope of the
> change suggest that this would be more appropriate for when Stage 1
> reopens.

Now that Stage 1 reopened, is this ok for 4.9?

	Jakub
Cary Coutant March 20, 2013, 6:21 p.m. UTC | #3 
> Now that Stage 1 reopened, is this ok for 4.9?

Looks OK to me; I just had a few comments on the code readability.

> +  if (lookup_decl_die (decl))
> +    return rtl;
> +
> +  len = TREE_STRING_LENGTH (t);
> +  vec_safe_push (used_rtx_array, rtl);
> +  ref = new_die (DW_TAG_dwarf_procedure, comp_unit_die (), decl);
> +  array = (unsigned char *) ggc_alloc_atomic (len);
> +  memcpy (array, TREE_STRING_POINTER (t), len);
> +  l = new_loc_descr (DW_OP_implicit_value, len, 0);
> +  l->dw_loc_oprnd2.val_class = dw_val_class_vec;
> +  l->dw_loc_oprnd2.v.val_vec.length = len;
> +  l->dw_loc_oprnd2.v.val_vec.elt_size = 1;
> +  l->dw_loc_oprnd2.v.val_vec.array = array;
> +  add_AT_loc (ref, DW_AT_location, l);
> +  equate_decl_number_to_die (decl, ref);
> +  return rtl;

This is just a suggestion -- rewrite the above as:

  if (!lookup_decl_die (decl))
    {
      ...
    }

  return rtl;

This makes it more clear that you're actually returning the same thing
in either case, and that the creation of the dwarf procedure is just
a side effect. When I read through this the first time, I was expecting
the fall-through path to return something different just because of the
structure.

> -  for (; loc; loc = loc->dw_loc_next)
> +  bool start_of_dw_expr = true;
> +  for (; loc; start_of_dw_expr = (loc->dw_loc_opc == DW_OP_piece
> +                                 || loc->dw_loc_opc == DW_OP_bit_piece),
> +             loc = loc->dw_loc_next)

Another suggestion to make this a little less unwieldy:

  for (dw_loc_descr_ref prev = NULL; loc; prev = loc, loc = loc->dw_loc_next)
    ...
           if ((prev == NULL
                || prev->dw_loc_opc == DW_OP_piece
                || prev->dw_loc_opc == DW_OP_bit_piece)
               && loc->dw_loc_next
               && loc->dw_loc_next->dw_loc_opc == DW_OP_stack_value
               && !dwarf_strict)
             {
               ...
             }

Can you replace the { ... } with a function call? And a comment about
what you're doing here.

> +             if (l != NULL
> +                 && l->dw_loc_next == NULL
> +                 && l->dw_loc_opc == DW_OP_addr
> +                 && GET_CODE (l->dw_loc_oprnd1.v.val_addr) == SYMBOL_REF
> +                 && SYMBOL_REF_DECL (l->dw_loc_oprnd1.v.val_addr)
> +                 && a->dw_attr == DW_AT_location)
> +               {
> +                 ...
> +               }

Likewise here.

-cary
diff mbox

Patch

--- gcc/dwarf2out.c.jj	2012-11-21 18:44:25.847837373 +0100
+++ gcc/dwarf2out.c	2012-12-06 17:22:14.348761149 +0100
@@ -15492,6 +15492,7 @@  tree_add_const_value_attribute (dw_die_r
 	      add_AT_vec (die, DW_AT_const_value, size, 1, array);
 	      return true;
 	    }
+	  ggc_free (array);
 	}
     }
   return false;
@@ -22370,6 +22371,10 @@  resolve_one_addr (rtx *addr, void *data
       if (!rtl || !MEM_P (rtl))
 	return 1;
       rtl = XEXP (rtl, 0);
+      if (GET_CODE (rtl) == SYMBOL_REF
+	  && SYMBOL_REF_DECL (rtl)
+	  && !TREE_ASM_WRITTEN (SYMBOL_REF_DECL (rtl)))
+	return 1;
       vec_safe_push (used_rtx_array, rtl);
       *addr = rtl;
       return 0;
@@ -22394,6 +22399,46 @@  resolve_one_addr (rtx *addr, void *data
   return 0;
 }
 
+/* For STRING_CST, return SYMBOL_REF of its constant pool entry,
+   if possible, and create DW_TAG_dwarf_procedure that can be referenced
+   from DW_OP_GNU_implicit_pointer if the string hasn't been seen yet.  */
+
+static rtx
+string_cst_pool_decl (tree t)
+{
+  rtx rtl = output_constant_def (t, 1);
+  unsigned char *array;
+  dw_loc_descr_ref l;
+  tree decl;
+  size_t len;
+  dw_die_ref ref;
+
+  if (!rtl || !MEM_P (rtl))
+    return NULL_RTX;
+  rtl = XEXP (rtl, 0);
+  if (GET_CODE (rtl) != SYMBOL_REF
+      || SYMBOL_REF_DECL (rtl) == NULL_TREE)
+    return NULL_RTX;
+
+  decl = SYMBOL_REF_DECL (rtl);
+  if (lookup_decl_die (decl))
+    return rtl;
+
+  len = TREE_STRING_LENGTH (t);
+  vec_safe_push (used_rtx_array, rtl);
+  ref = new_die (DW_TAG_dwarf_procedure, comp_unit_die (), decl);
+  array = (unsigned char *) ggc_alloc_atomic (len);
+  memcpy (array, TREE_STRING_POINTER (t), len);
+  l = new_loc_descr (DW_OP_implicit_value, len, 0);
+  l->dw_loc_oprnd2.val_class = dw_val_class_vec;
+  l->dw_loc_oprnd2.v.val_vec.length = len;
+  l->dw_loc_oprnd2.v.val_vec.elt_size = 1;
+  l->dw_loc_oprnd2.v.val_vec.array = array;
+  add_AT_loc (ref, DW_AT_location, l);
+  equate_decl_number_to_die (decl, ref);
+  return rtl;
+}
+
 /* Helper function for resolve_addr, handle one location
    expression, return false if at least one CONST_STRING or SYMBOL_REF in
    the location list couldn't be resolved.  */
@@ -22402,23 +22447,81 @@  static bool
 resolve_addr_in_expr (dw_loc_descr_ref loc)
 {
   dw_loc_descr_ref keep = NULL;
-  for (; loc; loc = loc->dw_loc_next)
+  bool start_of_dw_expr = true;
+  for (; loc; start_of_dw_expr = (loc->dw_loc_opc == DW_OP_piece
+				  || loc->dw_loc_opc == DW_OP_bit_piece),
+	      loc = loc->dw_loc_next)
     switch (loc->dw_loc_opc)
       {
       case DW_OP_addr:
 	if (resolve_one_addr (&loc->dw_loc_oprnd1.v.val_addr, NULL))
-	  return false;
+	  {
+	    if (start_of_dw_expr
+		&& loc->dw_loc_next
+		&& loc->dw_loc_next->dw_loc_opc == DW_OP_stack_value
+		&& !dwarf_strict)
+	      {
+		rtx rtl = loc->dw_loc_oprnd1.v.val_addr;
+		HOST_WIDE_INT offset = 0;
+		dw_die_ref ref = NULL;
+		tree decl;
+
+		if (GET_CODE (rtl) == CONST
+		    && GET_CODE (XEXP (rtl, 0)) == PLUS
+		    && CONST_INT_P (XEXP (XEXP (rtl, 0), 1)))
+		  {
+		    offset = INTVAL (XEXP (XEXP (rtl, 0), 1));
+		    rtl = XEXP (XEXP (rtl, 0), 0);
+		  }
+		if (GET_CODE (rtl) == CONST_STRING)
+		  {
+		    size_t len = strlen (XSTR (rtl, 0)) + 1;
+		    tree t = build_string (len, XSTR (rtl, 0));
+		    tree tlen = size_int (len - 1);
+
+		    TREE_TYPE (t)
+		      = build_array_type (char_type_node,
+					  build_index_type (tlen));
+		    rtl = string_cst_pool_decl (t);
+		    if (!rtl)
+		      return false;
+		  }
+		if (GET_CODE (rtl) == SYMBOL_REF
+		    && SYMBOL_REF_DECL (rtl))
+		  {
+		    decl = SYMBOL_REF_DECL (rtl);
+		    if (TREE_CODE (decl) == VAR_DECL
+			&& !DECL_EXTERNAL (decl))
+		      {
+			ref = lookup_decl_die (decl);
+			if (ref
+			    && (get_AT (ref, DW_AT_location)
+				|| get_AT (ref, DW_AT_const_value)))
+			  {
+			    loc->dw_loc_opc = DW_OP_GNU_implicit_pointer;
+			    loc->dw_loc_oprnd1.val_class
+			      = dw_val_class_die_ref;
+			    loc->dw_loc_oprnd1.val_entry = NULL;
+			    loc->dw_loc_oprnd1.v.val_die_ref.die = ref;
+			    loc->dw_loc_oprnd1.v.val_die_ref.external = 0;
+			    loc->dw_loc_next = loc->dw_loc_next->dw_loc_next;
+			    loc->dw_loc_oprnd2.v.val_int = offset;
+			    break;
+			  }
+		      }
+		  }
+	      }
+	    return false;
+	  }
 	break;
       case DW_OP_GNU_addr_index:
       case DW_OP_GNU_const_index:
-        {
-          if ((loc->dw_loc_opc == DW_OP_GNU_addr_index
-               || (loc->dw_loc_opc == DW_OP_GNU_const_index && loc->dtprel))
-              && resolve_one_addr (&loc->dw_loc_oprnd1.val_entry->addr.rtl,
-                                   NULL))
-            return false;
-        }
-       break;
+	if ((loc->dw_loc_opc == DW_OP_GNU_addr_index
+	     || (loc->dw_loc_opc == DW_OP_GNU_const_index && loc->dtprel))
+	    && resolve_one_addr (&loc->dw_loc_oprnd1.val_entry->addr.rtl,
+				 NULL))
+	  return false;
+	break;
       case DW_OP_const4u:
       case DW_OP_const8u:
 	if (loc->dtprel
@@ -22601,8 +22704,79 @@  resolve_addr (dw_die_ref die)
 	       || l->dw_loc_next != NULL)
 	      && !resolve_addr_in_expr (l))
 	    {
-              if (dwarf_split_debug_info)
-                remove_loc_list_addr_table_entries (l);
+	      if (dwarf_split_debug_info)
+		remove_loc_list_addr_table_entries (l);
+	      if (l != NULL
+		  && l->dw_loc_next == NULL
+		  && l->dw_loc_opc == DW_OP_addr
+		  && GET_CODE (l->dw_loc_oprnd1.v.val_addr) == SYMBOL_REF
+		  && SYMBOL_REF_DECL (l->dw_loc_oprnd1.v.val_addr)
+		  && a->dw_attr == DW_AT_location)
+		{
+		  tree decl = SYMBOL_REF_DECL (l->dw_loc_oprnd1.v.val_addr);
+		  remove_AT (die, a->dw_attr);
+		  ix--;
+		  if (TREE_CODE (decl) == VAR_DECL
+		      && lookup_decl_die (decl) == die
+		      && !DECL_EXTERNAL (decl)
+		      && TREE_STATIC (decl)
+		      && DECL_INITIAL (decl)
+		      && !DECL_P (DECL_INITIAL (decl))
+		      && !get_AT (die, DW_AT_const_value))
+		    {
+		      tree init = DECL_INITIAL (decl);
+		      HOST_WIDE_INT offset = 0;
+		      /* For variables that have been optimized away and thus
+			 don't have a memory location, see if we can emit
+			 DW_AT_const_value instead.  */
+		      if (tree_add_const_value_attribute (die, init))
+			break;
+		      if (dwarf_strict)
+			break;
+		      STRIP_NOPS (init);
+		      if (TREE_CODE (init) == POINTER_PLUS_EXPR
+			  && host_integerp (TREE_OPERAND (init, 1), 0))
+			{
+			  offset = tree_low_cst (TREE_OPERAND (init, 1), 0);
+			  init = TREE_OPERAND (init, 0);
+			  STRIP_NOPS (init);
+			}
+		      if (TREE_CODE (init) != ADDR_EXPR)
+			break;
+		      if ((TREE_CODE (TREE_OPERAND (init, 0)) == STRING_CST
+			   && !TREE_ASM_WRITTEN (TREE_OPERAND (init, 0)))
+			  || (TREE_CODE (TREE_OPERAND (init, 0)) == VAR_DECL
+			      && !DECL_EXTERNAL (TREE_OPERAND (init, 0))
+			      && TREE_OPERAND (init, 0) != decl))
+			{
+			  dw_die_ref ref;
+			  tree decl;
+
+			  if (TREE_CODE (TREE_OPERAND (init, 0)) == STRING_CST)
+			    {
+			      rtx rtl
+				= string_cst_pool_decl (TREE_OPERAND (init, 0));
+			      if (!rtl)
+				break;
+			      decl = SYMBOL_REF_DECL (rtl);
+			    }
+			  else
+			    decl = TREE_OPERAND (init, 0);
+			  ref = lookup_decl_die (decl);
+			  if (ref == NULL
+			      || (!get_AT (ref, DW_AT_location)
+				  && !get_AT (ref, DW_AT_const_value)))
+			    break;
+			  l = new_loc_descr (DW_OP_GNU_implicit_pointer, 0,
+					     offset);
+			  l->dw_loc_oprnd1.val_class = dw_val_class_die_ref;
+			  l->dw_loc_oprnd1.v.val_die_ref.die = ref;
+			  l->dw_loc_oprnd1.v.val_die_ref.external = 0;
+			  add_AT_loc (die, DW_AT_location, l);
+			}
+		    }
+		  break;
+		}
 	      remove_AT (die, a->dw_attr);
 	      ix--;
 	    }