diff mbox

RFC: PATCH to correct the scopes of various types

Message ID 4FA19426.3010302@redhat.com
State New
Headers show

Commit Message

Jason Merrill May 2, 2012, 8:08 p.m. UTC
I've been working on a patch to split out the DWARF debug info for 
COMDAT functions into COMDAT CUs to go in the same group.  One problem I 
ran into was that modified versions of function-local types (pointers, 
cv-qualified variants, etc) were ending up at global scope.  Separately, 
Tom Tromey asked me why GCC was using a declaration/definition pair for 
namespace-scope types rather than just defining them in the namespace. 
This patch fixes both of those issues; it also fixes an issue with 
typedef scoping that I haven't been able to reproduce more recently.

Tested x86_64-pc-linux-gnu.  Does this make sense to you?

Comments

Richard Henderson May 2, 2012, 8:43 p.m. UTC | #1
On 05/02/2012 01:08 PM, Jason Merrill wrote:
>      	* dwarf2out.c (modified_type_die): Use scope_die_for.
>      	(gen_type_die_with_usage, dwarf2out_finish): Likewise.
>      	(uses_local_type_r, uses_local_type): New.
>      	(scope_die_for): Keep a type that uses a local type in local scope.
>      	Use get_context_die for namespace and type scope.

Makes sense.


r~
H.J. Lu May 25, 2012, 12:33 p.m. UTC | #2
On Wed, May 2, 2012 at 1:08 PM, Jason Merrill <jason@redhat.com> wrote:
> I've been working on a patch to split out the DWARF debug info for COMDAT
> functions into COMDAT CUs to go in the same group.  One problem I ran into
> was that modified versions of function-local types (pointers, cv-qualified
> variants, etc) were ending up at global scope.  Separately, Tom Tromey asked
> me why GCC was using a declaration/definition pair for namespace-scope types
> rather than just defining them in the namespace. This patch fixes both of
> those issues; it also fixes an issue with typedef scoping that I haven't
> been able to reproduce more recently.
>
> Tested x86_64-pc-linux-gnu.  Does this make sense to you?

This caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53470
diff mbox

Patch

commit c2e5280c5e39f4656c3fadc1a7a049a1678a468e
Author: Jason Merrill <jason@redhat.com>
Date:   Thu Apr 19 11:21:09 2012 -0400

    	* dwarf2out.c (modified_type_die): Use scope_die_for.
    	(gen_type_die_with_usage, dwarf2out_finish): Likewise.
    	(uses_local_type_r, uses_local_type): New.
    	(scope_die_for): Keep a type that uses a local type in local scope.
    	Use get_context_die for namespace and type scope.

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 311914a..dbb305a 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -9021,6 +9021,7 @@  modified_type_die (tree type, int is_const_type, int is_volatile_type,
   tree item_type = NULL;
   tree qualified_type;
   tree name, low, high;
+  dw_die_ref mod_scope;
 
   if (code == ERROR_MARK)
     return NULL;
@@ -9081,6 +9082,8 @@  modified_type_die (tree type, int is_const_type, int is_volatile_type,
       /* Else cv-qualified version of named type; fall through.  */
     }
 
+  mod_scope = scope_die_for (type, context_die);
+
   if (is_const_type
       /* If both is_const_type and is_volatile_type, prefer the path
 	 which leads to a qualified type.  */
@@ -9088,17 +9091,17 @@  modified_type_die (tree type, int is_const_type, int is_volatile_type,
 	  || get_qualified_type (type, TYPE_QUAL_CONST) == NULL_TREE
 	  || get_qualified_type (type, TYPE_QUAL_VOLATILE) != NULL_TREE))
     {
-      mod_type_die = new_die (DW_TAG_const_type, comp_unit_die (), type);
+      mod_type_die = new_die (DW_TAG_const_type, mod_scope, type);
       sub_die = modified_type_die (type, 0, is_volatile_type, context_die);
     }
   else if (is_volatile_type)
     {
-      mod_type_die = new_die (DW_TAG_volatile_type, comp_unit_die (), type);
+      mod_type_die = new_die (DW_TAG_volatile_type, mod_scope, type);
       sub_die = modified_type_die (type, is_const_type, 0, context_die);
     }
   else if (code == POINTER_TYPE)
     {
-      mod_type_die = new_die (DW_TAG_pointer_type, comp_unit_die (), type);
+      mod_type_die = new_die (DW_TAG_pointer_type, mod_scope, type);
       add_AT_unsigned (mod_type_die, DW_AT_byte_size,
 		       simple_type_size_in_bits (type) / BITS_PER_UNIT);
       item_type = TREE_TYPE (type);
@@ -9109,10 +9112,10 @@  modified_type_die (tree type, int is_const_type, int is_volatile_type,
   else if (code == REFERENCE_TYPE)
     {
       if (TYPE_REF_IS_RVALUE (type) && dwarf_version >= 4)
-	mod_type_die = new_die (DW_TAG_rvalue_reference_type, comp_unit_die (),
+	mod_type_die = new_die (DW_TAG_rvalue_reference_type, mod_scope,
 				type);
       else
-	mod_type_die = new_die (DW_TAG_reference_type, comp_unit_die (), type);
+	mod_type_die = new_die (DW_TAG_reference_type, mod_scope, type);
       add_AT_unsigned (mod_type_die, DW_AT_byte_size,
 		       simple_type_size_in_bits (type) / BITS_PER_UNIT);
       item_type = TREE_TYPE (type);
@@ -15301,10 +15304,36 @@  pop_decl_scope (void)
   VEC_pop (tree, decl_scope_table);
 }
 
+/* walk_tree helper function for uses_local_type, below.  */
+
+static tree
+uses_local_type_r (tree *tp, int *walk_subtrees, void *data ATTRIBUTE_UNUSED)
+{
+  if (!TYPE_P (*tp))
+    *walk_subtrees = 0;
+  else
+    {
+      tree name = TYPE_NAME (*tp);
+      if (name && DECL_P (name) && decl_function_context (name))
+	return *tp;
+    }
+  return NULL_TREE;
+}
+
+/* If TYPE involves a function-local type (including a local typedef to a
+   non-local type), returns that type; otherwise returns NULL_TREE.  */
+
+static tree
+uses_local_type (tree type)
+{
+  tree used = walk_tree_without_duplicates (&type, uses_local_type_r, NULL);
+  return used;
+}
+
 /* Return the DIE for the scope that immediately contains this type.
-   Non-named types get global scope.  Named types nested in other
-   types get their containing scope if it's open, or global scope
-   otherwise.  All other types (i.e. function-local named types) get
+   Non-named types that do not involve a function-local type get global
+   scope.  Named types nested in namespaces or other types get their
+   containing scope.  All other types (i.e. function-local named types) get
    the current active scope.  */
 
 static dw_die_ref
@@ -15312,18 +15341,24 @@  scope_die_for (tree t, dw_die_ref context_die)
 {
   dw_die_ref scope_die = NULL;
   tree containing_scope;
-  int i;
 
   /* Non-types always go in the current scope.  */
   gcc_assert (TYPE_P (t));
 
-  containing_scope = TYPE_CONTEXT (t);
+  /* Use the scope of the typedef, rather than the scope of the type
+     it refers to.  */
+  if (TYPE_NAME (t) && DECL_P (TYPE_NAME (t)))
+    containing_scope = DECL_CONTEXT (TYPE_NAME (t));
+  else
+    containing_scope = TYPE_CONTEXT (t);
 
-  /* Use the containing namespace if it was passed in (for a declaration).  */
+  /* Use the containing namespace if there is one.  */
   if (containing_scope && TREE_CODE (containing_scope) == NAMESPACE_DECL)
     {
       if (context_die == lookup_decl_die (containing_scope))
 	/* OK */;
+      else if (debug_info_level > DINFO_LEVEL_TERSE)
+	context_die = get_context_die (containing_scope);
       else
 	containing_scope = NULL_TREE;
     }
@@ -15335,30 +15370,25 @@  scope_die_for (tree t, dw_die_ref context_die)
     containing_scope = NULL_TREE;
 
   if (SCOPE_FILE_SCOPE_P (containing_scope))
-    scope_die = comp_unit_die ();
+    {
+      /* If T uses a local type keep it local as well, to avoid references
+	 to function-local DIEs from outside the function.  */
+      if (current_function_decl && uses_local_type (t))
+	scope_die = context_die;
+      else
+	scope_die = comp_unit_die ();
+    }
   else if (TYPE_P (containing_scope))
     {
-      /* For types, we can just look up the appropriate DIE.  But
-	 first we check to see if we're in the middle of emitting it
-	 so we know where the new DIE should go.  */
-      for (i = VEC_length (tree, decl_scope_table) - 1; i >= 0; --i)
-	if (VEC_index (tree, decl_scope_table, i) == containing_scope)
-	  break;
-
-      if (i < 0)
+      /* For types, we can just look up the appropriate DIE.  */
+      if (debug_info_level > DINFO_LEVEL_TERSE)
+	scope_die = get_context_die (containing_scope);
+      else
 	{
-	  gcc_assert (debug_info_level <= DINFO_LEVEL_TERSE
-		      || TREE_ASM_WRITTEN (containing_scope));
-	  /*We are not in the middle of emitting the type
-	    CONTAINING_SCOPE. Let's see if it's emitted already.  */
-	  scope_die = lookup_type_die (containing_scope);
-
-	  /* If none of the current dies are suitable, we get file scope.  */
+	  scope_die = lookup_type_die_strip_naming_typedef (containing_scope);
 	  if (scope_die == NULL)
 	    scope_die = comp_unit_die ();
 	}
-      else
-	scope_die = lookup_type_die_strip_naming_typedef (containing_scope);
     }
   else
     scope_die = context_die;
@@ -18154,12 +18184,8 @@  gen_type_die_with_usage (tree type, dw_die_ref context_die,
       /* Prevent broken recursion; we can't hand off to the same type.  */
       gcc_assert (DECL_ORIGINAL_TYPE (TYPE_NAME (type)) != type);
 
-      /* Use the DIE of the containing namespace as the parent DIE of
-         the type description DIE we want to generate.  */
-      if (DECL_FILE_SCOPE_P (TYPE_NAME (type))
-	  || (DECL_CONTEXT (TYPE_NAME (type))
-	      && TREE_CODE (DECL_CONTEXT (TYPE_NAME (type))) == NAMESPACE_DECL))
-	context_die = get_context_die (DECL_CONTEXT (TYPE_NAME (type)));
+      /* Give typedefs the right scope.  */
+      context_die = scope_die_for (type, context_die);
 
       TREE_ASM_WRITTEN (type) = 1;
 
@@ -21780,16 +21806,15 @@  dwarf2out_finish (const char *filename)
 		 inlined and optimized out.  In that case we are lost and
 		 assign the empty child.  This should not be big issue as
 		 the function is likely unreachable too.  */
-	      tree context = NULL_TREE;
-
 	      gcc_assert (node->created_for);
 
 	      if (DECL_P (node->created_for))
-		context = DECL_CONTEXT (node->created_for);
+		origin = get_context_die (DECL_CONTEXT (node->created_for));
 	      else if (TYPE_P (node->created_for))
-		context = TYPE_CONTEXT (node->created_for);
+		origin = scope_die_for (node->created_for, comp_unit_die ());
+	      else
+		origin = comp_unit_die ();
 
-	      origin = get_context_die (context);
 	      add_child_die (origin, die);
 	    }
 	}
diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/localclass3.C b/gcc/testsuite/g++.dg/debug/dwarf2/localclass3.C
new file mode 100644
index 0000000..be28a19
--- /dev/null
+++ b/gcc/testsuite/g++.dg/debug/dwarf2/localclass3.C
@@ -0,0 +1,11 @@ 
+// Test that the A* pointer_type is also within the debug info for f.
+// Currently GCC emits it immediately before A, which is simple to test for.
+// { dg-options "-g -dA" }
+
+void f()
+{
+  struct A { int i; } *ap;
+  ap->i = 42;
+}
+
+// { dg-final { scan-assembler "DW_TAG_pointer_type.\[^)\]*. DW_TAG_structure_type" } }
diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/namespace-2.C b/gcc/testsuite/g++.dg/debug/dwarf2/namespace-2.C
new file mode 100644
index 0000000..0289e90
--- /dev/null
+++ b/gcc/testsuite/g++.dg/debug/dwarf2/namespace-2.C
@@ -0,0 +1,10 @@ 
+// Test that we define A inside the namespace rather than declaring it
+// there and then defining it at CU scope.
+// { dg-options "-g -dA" }
+// { dg-final { scan-assembler-not "DW_AT_declaration" } }
+
+namespace N {
+  struct A;
+}
+
+struct N::A { } a;