diff mbox series

Expunge namespace-scope IDENTIFIER_TYPE_VALUE & global_type_name [PR 99039]

Message ID d2d36aa8-4a7c-7795-6a39-df3db683bc59@acm.org
State New
Headers show
Series Expunge namespace-scope IDENTIFIER_TYPE_VALUE & global_type_name [PR 99039] | expand

Commit Message

Nathan Sidwell Feb. 12, 2021, 9:28 p.m. UTC
IDENTIFIER_TYPE_VALUE and friends is a remnant of G++'s C origins.  It 
holds elaborated types on identifier-nodes.  While this is fine for C 
and for local and class-scopes in C++, it fails badly for namespaces. In 
that case a marker 'global_type_node' was used, which essentially 
signified 'this is a namespace-scope type *somewhere*', and you'd have 
to do a regular name_lookup to find it.  As the parser and substitution 
machinery has avanced over the last 25 years or so, there's not much 
outside of actual name-lookup that uses that. Amusingly the 
IDENTIFIER_HAS_TYPE_VALUE predicate will do an actual name-lookup and 
then users would repeat that lookup to find the now-known to be there type.

Rather late I realized that this interferes with the lazy loading of 
module entities, because we were setting IDENTIFIER_TYPE_VALUE to 
global_type_node.  But we could be inside some local scope where that 
identifier is bound to some local type.  Not good!

Rather than add more cruft to look at an identifier's shadow stack and 
alter that as necessary, this takes the approach of removing the 
existing cruft.

We nuke the few places outside of name lookup that use 
IDENTIFIER_TYPE_VALUE.  Replacing them with either proper name lookups, 
alternative sequences, or in some cases asserting that they (no longer) 
happen.  Class template instantiation was calling pushtag after setting 
IDENTIFIER_TYPE_VALUE in order to stop pushtag creating an implicit 
typedef and pushing it, but to get the bookkeeping it needed.  Let's 
just do the bookkeeping directly.

Then we can stop having a 'bound at namespace-scope' marker at all, 
which means lazy loading won't screw up local shadow stacks.  Also, it 
simplifies set_identifier_type_value_with_scope, as it never needs to 
inspect the scope stack.  When developing this patch, I discovered a 
number of places we'd put an actual namespace-scope type on the 
type_value slot, rather than global_type_node.  You might notice this is 
killing at least two 'why are we doing this?' comments.

While this doesn't fix the two PRs mentioned, it is a necessary step.

             PR c++/99039
             PR c++/99040
             gcc/cp/
             * cp-tree.h (CPTI_GLOBAL_TYPE): Delete.
             (global_type_node): Delete.
             (IDENTIFIER_TYPE_VALUE): Delete.
             (IDENTIFIER_HAS_TYPE_VALUE): Delete.
             (get_type_value): Delete.
             * name-lookup.h (identifier_type_value): Delete.
             * name-lookup.c (check_module_override): Don't
             SET_IDENTIFIER_TYPE_VALUE here.
             (do_pushdecl): Nor here.
             (identifier_type_value_1, identifier_type_value): Delete.
             (set_identifier_type_value_with_scope): Only
             SET_IDENTIFIER_TYPE_VALUE for local and class scopes.
             (pushdecl_nanmespace_level): Remove shadow stack nadgering.
             (do_pushtag): Use REAL_IDENTIFIER_TYPE_VALUE.
             * call.c (check_dtor_name): Use lookup_name.
             * decl.c (cxx_init_decl_processing): Drop global_type_node.
             * decl2.c (cplus_decl_attributes): Don't 
SET_IDENTIFIER_TYPE_VALUE
             here.
             * init.c (get_type_value): Delete.
             * pt.c (instantiate_class_template_1): Don't call pushtag or
             SET_IDENTIFIER_TYPE_VALUE here.
             (tsubst): Assert never an identifier.
             (dependent_type_p): Drop global_type_node assert.
             * typeck.c (error_args_num): Don't use 
IDENTIFIER_HAS_TYPE_VALUE
             to determine ctorness.
             gcc/testsuite/
             * g++.dg/lookup/pr99039.C: New.
diff mbox series

Patch

diff --git c/gcc/cp/call.c w/gcc/cp/call.c
index 4744c9768ec..186feef6fe3 100644
--- c/gcc/cp/call.c
+++ w/gcc/cp/call.c
@@ -236,8 +236,15 @@  check_dtor_name (tree basetype, tree name)
 	   || TREE_CODE (basetype) == ENUMERAL_TYPE)
 	  && name == constructor_name (basetype))
 	return true;
-      else
-	name = get_type_value (name);
+
+      /* Otherwise lookup the name, it could be an unrelated typedef
+	 of the correct type.  */
+      name = lookup_name (name, LOOK_want::TYPE);
+      if (!name)
+	return false;
+      name = TREE_TYPE (name);
+      if (name == error_mark_node)
+	return false;
     }
   else
     {
@@ -252,8 +259,6 @@  check_dtor_name (tree basetype, tree name)
       return false;
     }
 
-  if (!name || name == error_mark_node)
-    return false;
   return same_type_p (TYPE_MAIN_VARIANT (basetype), TYPE_MAIN_VARIANT (name));
 }
 
diff --git c/gcc/cp/cp-tree.h w/gcc/cp/cp-tree.h
index 4ed3936ade2..38b31e3908f 100644
--- c/gcc/cp/cp-tree.h
+++ w/gcc/cp/cp-tree.h
@@ -129,7 +129,6 @@  enum cp_tree_index
     CPTI_VTBL_TYPE,
     CPTI_VTBL_PTR_TYPE,
     CPTI_GLOBAL,
-    CPTI_GLOBAL_TYPE,
     CPTI_ABORT_FNDECL,
     CPTI_AGGR_TAG,
     CPTI_CONV_OP_MARKER,
@@ -250,7 +249,6 @@  extern GTY(()) tree cp_global_trees[CPTI_MAX];
 #define std_node			cp_global_trees[CPTI_STD]
 #define abi_node			cp_global_trees[CPTI_ABI]
 #define global_namespace		cp_global_trees[CPTI_GLOBAL]
-#define global_type_node		cp_global_trees[CPTI_GLOBAL_TYPE]
 #define const_type_info_type_node	cp_global_trees[CPTI_CONST_TYPE_INFO_TYPE]
 #define type_info_ptr_type		cp_global_trees[CPTI_TYPE_INFO_PTR_TYPE]
 #define conv_op_marker			cp_global_trees[CPTI_CONV_OP_MARKER]
@@ -1149,24 +1147,16 @@  enum GTY(()) abstract_class_use {
 
 /* Macros for access to language-specific slots in an identifier.  */
 
-/* The IDENTIFIER_BINDING is the innermost cxx_binding for the
-    identifier.  Its PREVIOUS is the next outermost binding.  Each
-    VALUE field is a DECL for the associated declaration.  Thus,
-    name lookup consists simply of pulling off the node at the front
-    of the list (modulo oddities for looking up the names of types,
-    and such.)  You can use SCOPE field to determine the scope
-    that bound the name.  */
+/* Identifiers map directly to block or class-scope bindings.
+   Namespace-scope bindings are held in hash tables on the respective
+   namespaces.  The identifier bindings are the innermost active
+   binding, from whence you can get the decl and/or implicit-typedef
+   of an elaborated type.   When not bound to a local entity the
+   values are NULL.  */
 #define IDENTIFIER_BINDING(NODE) \
   (LANG_IDENTIFIER_CAST (NODE)->bindings)
-
-/* TREE_TYPE only indicates on local and class scope the current
-   type. For namespace scope, the presence of a type in any namespace
-   is indicated with global_type_node, and the real type behind must
-   be found through lookup.  */
-#define IDENTIFIER_TYPE_VALUE(NODE) identifier_type_value (NODE)
 #define REAL_IDENTIFIER_TYPE_VALUE(NODE) TREE_TYPE (NODE)
 #define SET_IDENTIFIER_TYPE_VALUE(NODE,TYPE) (TREE_TYPE (NODE) = (TYPE))
-#define IDENTIFIER_HAS_TYPE_VALUE(NODE) (IDENTIFIER_TYPE_VALUE (NODE) ? 1 : 0)
 
 /* Kinds of identifiers.  Values are carefully chosen.  */
 enum cp_identifier_kind {
@@ -6862,7 +6852,6 @@  extern void emit_mem_initializers		(tree);
 extern tree build_aggr_init			(tree, tree, int,
                                                  tsubst_flags_t);
 extern int is_class_type			(tree, int);
-extern tree get_type_value			(tree);
 extern tree build_zero_init			(tree, tree, bool);
 extern tree build_value_init			(tree, tsubst_flags_t);
 extern tree build_value_init_noctor		(tree, tsubst_flags_t);
diff --git c/gcc/cp/decl.c w/gcc/cp/decl.c
index 0aa49f91d5a..6f3414f058e 100644
--- c/gcc/cp/decl.c
+++ w/gcc/cp/decl.c
@@ -4510,9 +4510,6 @@  cxx_init_decl_processing (void)
   abi_node = current_namespace;
   pop_namespace ();
 
-  global_type_node = make_node (LANG_TYPE);
-  record_unknown_type (global_type_node, "global type");
-
   any_targ_node = make_node (LANG_TYPE);
   record_unknown_type (any_targ_node, "any type");
 
diff --git c/gcc/cp/decl2.c w/gcc/cp/decl2.c
index b087753cfba..c46100de89a 100644
--- c/gcc/cp/decl2.c
+++ w/gcc/cp/decl2.c
@@ -1596,9 +1596,6 @@  cplus_decl_attributes (tree *decl, tree attributes, int flags)
       decl_attributes (decl, attributes, flags, last_decl);
     }
 
-  if (TREE_CODE (*decl) == TYPE_DECL)
-    SET_IDENTIFIER_TYPE_VALUE (DECL_NAME (*decl), TREE_TYPE (*decl));
-
   /* Propagate deprecation out to the template.  */
   if (TREE_DEPRECATED (*decl))
     if (tree ti = get_template_info (*decl))
diff --git c/gcc/cp/init.c w/gcc/cp/init.c
index 1381a23449e..49950d40521 100644
--- c/gcc/cp/init.c
+++ w/gcc/cp/init.c
@@ -2118,18 +2118,6 @@  is_class_type (tree type, int or_else)
   return 1;
 }
 
-tree
-get_type_value (tree name)
-{
-  if (name == error_mark_node)
-    return NULL_TREE;
-
-  if (IDENTIFIER_HAS_TYPE_VALUE (name))
-    return IDENTIFIER_TYPE_VALUE (name);
-  else
-    return NULL_TREE;
-}
-
 /* Build a reference to a member of an aggregate.  This is not a C++
    `&', but really something which can have its address taken, and
    then act as a pointer to member, for example TYPE :: FIELD can have
diff --git c/gcc/cp/name-lookup.c w/gcc/cp/name-lookup.c
index 1cd4f67b8b2..8aa490db634 100644
--- c/gcc/cp/name-lookup.c
+++ w/gcc/cp/name-lookup.c
@@ -3620,14 +3620,7 @@  check_module_override (tree decl, tree mvec, bool hiding,
 	  if (iter.using_p ())
 	    ;
 	  else if (tree match = duplicate_decls (decl, *iter, hiding))
-	    {
-	      if (TREE_CODE (match) == TYPE_DECL)
-		/* The IDENTIFIER will have the type referring to the
-		   now-smashed TYPE_DECL, because ...?  Reset it.  */
-		SET_IDENTIFIER_TYPE_VALUE (name, TREE_TYPE (match));
-
-	      return match;
-	    }
+	    return match;
       }
 
   if (TREE_PUBLIC (scope) && TREE_PUBLIC (decl) && !not_module_p ()
@@ -3649,11 +3642,7 @@  check_module_override (tree decl, tree mvec, bool hiding,
 	  tree match = *iter;
 	  
 	  if (duplicate_decls (decl, match, hiding))
-	    {
-	      if (TREE_CODE (match) == TYPE_DECL)
-		SET_IDENTIFIER_TYPE_VALUE (name, TREE_TYPE (match));
-	      return match;
-	    }
+	    return match;
 	}
     }
 
@@ -3736,9 +3725,14 @@  do_pushdecl (tree decl, bool hiding)
 	    if (match == error_mark_node)
 	      ;
 	    else if (TREE_CODE (match) == TYPE_DECL)
-	      /* The IDENTIFIER will have the type referring to the
-		 now-smashed TYPE_DECL, because ...?  Reset it.  */
-	      SET_IDENTIFIER_TYPE_VALUE (name, TREE_TYPE (match));
+	      {
+		auto *l = level;
+		while (l->kind == sk_template_parms)
+		  l = l->level_chain;
+		gcc_checking_assert (REAL_IDENTIFIER_TYPE_VALUE (name)
+				     == (l->kind == sk_namespace
+					 ? NULL_TREE : TREE_TYPE (match)));
+	      }
 	    else if (iter.hidden_p () && !hiding)
 	      {
 		/* Unhiding a previously hidden decl.  */
@@ -4748,37 +4742,6 @@  print_binding_stack (void)
   print_binding_level (NAMESPACE_LEVEL (global_namespace));
 }
 
-/* Return the type associated with ID.  */
-
-static tree
-identifier_type_value_1 (tree id)
-{
-  /* There is no type with that name, anywhere.  */
-  if (REAL_IDENTIFIER_TYPE_VALUE (id) == NULL_TREE)
-    return NULL_TREE;
-  /* This is not the type marker, but the real thing.  */
-  if (REAL_IDENTIFIER_TYPE_VALUE (id) != global_type_node)
-    return REAL_IDENTIFIER_TYPE_VALUE (id);
-  /* Have to search for it. It must be on the global level, now.
-     Ask lookup_name not to return non-types.  */
-  id = lookup_name (id, LOOK_where::BLOCK_NAMESPACE, LOOK_want::TYPE);
-  if (id)
-    return TREE_TYPE (id);
-  return NULL_TREE;
-}
-
-/* Wrapper for identifier_type_value_1.  */
-
-tree
-identifier_type_value (tree id)
-{
-  tree ret;
-  timevar_start (TV_NAME_LOOKUP);
-  ret = identifier_type_value_1 (id);
-  timevar_stop (TV_NAME_LOOKUP);
-  return ret;
-}
-
 /* Push a definition of struct, union or enum tag named ID.  into
    binding_level B.  DECL is a TYPE_DECL for the type.  DECL has
    already been pushed into its binding level.  This is bookkeeping to
@@ -4787,26 +4750,21 @@  identifier_type_value (tree id)
 static void
 set_identifier_type_value_with_scope (tree id, tree decl, cp_binding_level *b)
 {
-  tree type;
+  while (b->kind == sk_template_parms)
+    b = b->level_chain;
 
-  if (b->kind != sk_namespace)
-    {
-      /* Shadow the marker, not the real thing, so that the marker
-	 gets restored later.  */
-      tree old_type_value = REAL_IDENTIFIER_TYPE_VALUE (id);
-      b->type_shadowed = tree_cons (id, old_type_value, b->type_shadowed);
-      type = decl ? TREE_TYPE (decl) : NULL_TREE;
-      TREE_TYPE (b->type_shadowed) = type;
-    }
+  if (b->kind == sk_namespace)
+    /* At namespace scope we should not see an identifier type value.  */
+    gcc_checking_assert (!REAL_IDENTIFIER_TYPE_VALUE (id));
   else
     {
-      gcc_assert (decl);
-
-      /* Store marker instead of real type.  */
-      type = global_type_node;
+      /* Push the current type value, so we can restore it later  */
+      tree old = REAL_IDENTIFIER_TYPE_VALUE (id);
+      b->type_shadowed = tree_cons (id, old, b->type_shadowed);
+      tree type = decl ? TREE_TYPE (decl) : NULL_TREE;
+      TREE_TYPE (b->type_shadowed) = type;
+      SET_IDENTIFIER_TYPE_VALUE (id, type);
     }
-
-  SET_IDENTIFIER_TYPE_VALUE (id, type);
 }
 
 /* As set_identifier_type_value_with_scope, but using
@@ -6238,51 +6196,17 @@  do_namespace_alias (tree alias, tree name_space)
     (*debug_hooks->early_global_decl) (alias);
 }
 
-/* Like pushdecl, only it places X in the current namespace,
+/* Like pushdecl, only it places DECL in the current namespace,
    if appropriate.  */
 
 tree
-pushdecl_namespace_level (tree x, bool hiding)
+pushdecl_namespace_level (tree decl, bool hiding)
 {
-  cp_binding_level *b = current_binding_level;
-  tree t;
-
   bool subtime = timevar_cond_start (TV_NAME_LOOKUP);
-  t = do_pushdecl_with_scope (x, NAMESPACE_LEVEL (current_namespace), hiding);
-
-  /* Now, the type_shadowed stack may screw us.  Munge it so it does
-     what we want.  */
-  if (TREE_CODE (t) == TYPE_DECL)
-    {
-      tree name = DECL_NAME (t);
-      tree newval;
-      tree *ptr = (tree *)0;
-      for (; !global_scope_p (b); b = b->level_chain)
-	{
-	  tree shadowed = b->type_shadowed;
-	  for (; shadowed; shadowed = TREE_CHAIN (shadowed))
-	    if (TREE_PURPOSE (shadowed) == name)
-	      {
-		ptr = &TREE_VALUE (shadowed);
-		/* Can't break out of the loop here because sometimes
-		   a binding level will have duplicate bindings for
-		   PT names.  It's gross, but I haven't time to fix it.  */
-	      }
-	}
-      newval = TREE_TYPE (t);
-      if (ptr == (tree *)0)
-	{
-	  /* @@ This shouldn't be needed.  My test case "zstring.cc" trips
-	     up here if this is changed to an assertion.  --KR  */
-	  SET_IDENTIFIER_TYPE_VALUE (name, t);
-	}
-      else
-	{
-	  *ptr = newval;
-	}
-    }
+  tree res = do_pushdecl_with_scope (decl, NAMESPACE_LEVEL (current_namespace),
+				     hiding);
   timevar_cond_stop (TV_NAME_LOOKUP, subtime);
-  return t;
+  return res;
 }
 
 /* Wrapper around push_local_binding to push the bindings for
@@ -8257,6 +8181,8 @@  do_pushtag (tree name, tree type, TAG_how how)
 {
   tree decl;
 
+  gcc_assert (identifier_p (name));
+
   cp_binding_level *b = current_binding_level;
   while (true)
     {
@@ -8282,10 +8208,8 @@  do_pushtag (tree name, tree type, TAG_how how)
 	break;
     }
 
-  gcc_assert (identifier_p (name));
-
   /* Do C++ gratuitous typedefing.  */
-  if (identifier_type_value_1 (name) != type)
+  if (REAL_IDENTIFIER_TYPE_VALUE (name) != type)
     {
       tree tdef;
       tree context = TYPE_CONTEXT (type);
diff --git c/gcc/cp/name-lookup.h w/gcc/cp/name-lookup.h
index 75db5b38061..e159942eda4 100644
--- c/gcc/cp/name-lookup.h
+++ w/gcc/cp/name-lookup.h
@@ -188,7 +188,6 @@  struct GTY(()) tree_binding_vec {
 #define BINDING_VECTOR_PENDING_IS_PARTITION_P(NODE) \
   (BINDING_VECTOR_CHECK (NODE)->base.private_flag)
 
-extern tree identifier_type_value (tree);
 extern void set_identifier_type_value (tree, tree);
 extern void push_binding (tree, tree, cp_binding_level*);
 extern void pop_local_binding (tree, tree);
diff --git c/gcc/cp/pt.c w/gcc/cp/pt.c
index 50cdb00310f..60de8e93ff7 100644
--- c/gcc/cp/pt.c
+++ w/gcc/cp/pt.c
@@ -11971,14 +11971,13 @@  instantiate_class_template_1 (tree type)
 		       instantiation, but the TYPE is not.) */
 		    CLASSTYPE_USE_TEMPLATE (newtag) = 0;
 
-		  /* Now, we call pushtag to put this NEWTAG into the scope of
-		     TYPE.  We first set up the IDENTIFIER_TYPE_VALUE to avoid
-		     pushtag calling push_template_decl.  We don't have to do
-		     this for enums because it will already have been done in
-		     tsubst_enum.  */
-		  if (name)
-		    SET_IDENTIFIER_TYPE_VALUE (name, newtag);
-		  pushtag (name, newtag);
+		  /* Now, install the tag.  We don't use pushtag
+		     because that does too much work -- creating an
+		     implicit typedef, which we've already done.  */
+		  set_identifier_type_value (name, TYPE_NAME (newtag));
+		  maybe_add_class_template_decl_list (type, newtag, false);
+		  TREE_PUBLIC (TYPE_NAME (newtag)) = true;
+		  determine_visibility (TYPE_NAME (newtag));
 		}
 	    }
 	  else if (DECL_DECLARES_FUNCTION_P (t))
@@ -15424,10 +15423,8 @@  tsubst (tree t, tree args, tsubst_flags_t complain, tree in_decl)
 
   code = TREE_CODE (t);
 
-  if (code == IDENTIFIER_NODE)
-    type = IDENTIFIER_TYPE_VALUE (t);
-  else
-    type = TREE_TYPE (t);
+  gcc_assert (code != IDENTIFIER_NODE);
+  type = TREE_TYPE (t);
 
   gcc_assert (type != unknown_type_node);
 
@@ -26774,10 +26771,6 @@  dependent_type_p (tree type)
   if (type == error_mark_node)
     return false;
 
-  /* Getting here with global_type_node means we improperly called this
-     function on the TREE_TYPE of an IDENTIFIER_NODE.  */
-  gcc_checking_assert (type != global_type_node);
-
   /* If we have not already computed the appropriate value for TYPE,
      do so now.  */
   if (!TYPE_DEPENDENT_P_VALID (type))
diff --git c/gcc/cp/typeck.c w/gcc/cp/typeck.c
index a87d5e5f2ac..d06d7440d5c 100644
--- c/gcc/cp/typeck.c
+++ w/gcc/cp/typeck.c
@@ -4060,7 +4060,8 @@  error_args_num (location_t loc, tree fndecl, bool too_many_p)
       if (TREE_CODE (TREE_TYPE (fndecl)) == METHOD_TYPE)
 	{
 	  if (DECL_NAME (fndecl) == NULL_TREE
-	      || IDENTIFIER_HAS_TYPE_VALUE (DECL_NAME (fndecl)))
+	      || (DECL_NAME (fndecl)
+		  == DECL_NAME (TYPE_NAME (DECL_CONTEXT (fndecl)))))
 	    error_at (loc,
 		      too_many_p
 		      ? G_("too many arguments to constructor %q#D")
diff --git c/gcc/testsuite/g++.dg/lookup/pr99039.C w/gcc/testsuite/g++.dg/lookup/pr99039.C
new file mode 100644
index 00000000000..982a991daa7
--- /dev/null
+++ w/gcc/testsuite/g++.dg/lookup/pr99039.C
@@ -0,0 +1,51 @@ 
+// PR 99039, we need to remove the namespace-scope meaning of
+// IDENTIFIER_TYPE_VALUE.
+
+namespace std
+{
+typedef long unsigned int size_t;
+
+template<typename _CharT>
+struct char_traits
+{
+  typedef _CharT char_type;
+
+  template<typename U>
+  static int
+    compare(const char_type* __s1, const char_type* __s2, std::size_t __n);
+};
+
+template<typename _CharT>
+template<typename U>
+int
+char_traits<_CharT>::
+compare(const char_type* __s1, const char_type* __s2, std::size_t __n)
+{
+  return 0;
+}
+
+}
+
+struct CHAR_TRAITS;
+namespace std
+{
+typedef long unsigned int size_t;
+
+struct CHAR_TRAITS
+{
+  typedef char char_type;
+
+  static int
+    compare(const char_type* __s1, const char_type* __s2, std::size_t __n);
+};
+
+int
+CHAR_TRAITS::
+compare(const char_type* __s1, const char_type* __s2, std::size_t __n)
+{
+  return 0;
+}
+
+}
+
+struct CHAR_TRAITS;