c++/modules: depending local enums [PR104919, PR106009]

For local enums defined in a non-template function or a function template
instantiation it seems we neglect to make the function depend on the enum
definition (which modules considers logically separate), which ultimately
causes the enum definition to not be properly streamed before uses
within the function definition are streamed.

The code responsible for noting such dependencies is

gcc/cp/module.cc
@@ -8784,17 +8784,6 @@ trees_out::decl_node (tree decl, walk_kind ref)
   depset *dep = NULL;
   if (streaming_p ())
     dep = dep_hash->find_dependency (decl);
!  else if (TREE_CODE (ctx) != FUNCTION_DECL
!          || TREE_CODE (decl) == TEMPLATE_DECL
!          || (dep_hash->sneakoscope && DECL_IMPLICIT_TYPEDEF_P (decl))
!          || (DECL_LANG_SPECIFIC (decl)
!              && DECL_MODULE_IMPORT_P (decl)))
!    {
!      auto kind = (TREE_CODE (decl) == NAMESPACE_DECL
!                  && !DECL_NAMESPACE_ALIAS (decl)
!                  ? depset::EK_NAMESPACE : depset::EK_DECL);
!      dep = dep_hash->add_dependency (decl, kind);
!    }

   if (!dep)
     {

and the condition there notably excludes local TYPE_DECLs from a
non-template-pattern function (when streaming a template pattern
we'll see be dealing with the corresponding TEMPLATE_DECL of the
local TYPE_DECL here, so we'll add the dependency).

Local classes on the other hand seem to work properly, but perhaps by
accident: with a local class we end up making the function depend on the
injected-class-name of the local class rather than the local class as a
whole because the injected-class-name satisfies the criteria (since its
context is the local class, not the function).

The 'sneakoscope' flag is set when walking a function declaration and
its purpose seems to be to catch a local type that escapes the function
via a deduced return type (so called voldemort types) and note a
dependency on them.  But there seems to be no reason to restrict this
behavior to voldemort types, and indeed consistently noting the dependency
for all local types fixes these PRs (almost).  So this patch gets rid of
this flag and enables the dependency tracking unconditionally.

This was nearly enough to make things work, except we now ran into
issues with the local TYPE_/CONST_DECL copies from the pre-gimplified
version of a constexpr function body during streaming.  Rather than
making modules cope with this, it occurred to me that we don't need to
make copies of local types when saving the pre-gimplified body (and when
making further copies thereof); only VAR_DECLs etc need to be copied
(so that we don't conflate local variables from different recursive
calls to the same function during constexpr evaluation).  So this patch
adjusts copy_fn accordingly.

	PR c++/104919
	PR c++/106009

gcc/cp/ChangeLog:

	* module.cc (depset::hash::sneakoscope): Remove.
	(trees_out::decl_node): Always add a dependency on a local type.
	(depset::hash::find_dependencies): Remove sneakoscope stuff.

gcc/ChangeLog:

	* tree-inline.cc (remap_decl): Handle copy_decl returning the
	original decl.
	(remap_decls): Handle remap_decl returning the original decl.
	(copy_fn): Adjust copy_decl callback to skip TYPE_DECL and
	CONST_DECL.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/tdef-7.h: Remove outdated comment.
	* g++.dg/modules/tdef-7_b.C: Don't expect two TYPE_DECLs.
	* g++.dg/modules/enum-13_a.C: New test.
	* g++.dg/modules/enum-13_b.C: New test.

diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 355ee5d775dc4a21b65460001178bceabba50572..67f132d28d7aab7552487a842a7a20ee9e1c5513 100644 (file)
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -2522,13 +2522,12 @@ public:
     hash *chain;            /* Original table.  */
     depset *current;         /* Current depset being depended.  */
     unsigned section;       /* When writing out, the section.  */
-    bool sneakoscope;        /* Detecting dark magic (of a voldemort).  */
     bool reached_unreached;  /* We reached an unreached entity.  */
 
   public:
     hash (size_t size, hash *c = NULL)
       : parent (size), chain (c), current (NULL), section (0),
-       sneakoscope (false), reached_unreached (false)
+       reached_unreached (false)
     {
       worklist.create (size);
     }
@@ -8753,7 +8752,7 @@ trees_out::decl_node (tree decl, walk_kind ref)
     dep = dep_hash->find_dependency (decl);
   else if (TREE_CODE (ctx) != FUNCTION_DECL
           || TREE_CODE (decl) == TEMPLATE_DECL
-          || (dep_hash->sneakoscope && DECL_IMPLICIT_TYPEDEF_P (decl))
+          || DECL_IMPLICIT_TYPEDEF_P (decl)
           || (DECL_LANG_SPECIFIC (decl)
               && DECL_MODULE_IMPORT_P (decl)))
     {
@@ -13432,14 +13431,7 @@ depset::hash::find_dependencies (module_state *module)
                      add_namespace_context (item, ns);
                    }
 
-                 // FIXME: Perhaps p1815 makes this redundant? Or at
-                 // least simplifies it.  Voldemort types are only
-                 // ever emissable when containing (inline) function
-                 // definition is emitted?
-                 /* Turn the Sneakoscope on when depending the decl.  */
-                 sneakoscope = true;
                  walker.decl_value (decl, current);
-                 sneakoscope = false;
                  if (current->has_defn ())
                    walker.write_definition (decl);
                }

diff --git a/gcc/testsuite/g++.dg/modules/enum-13_a.C b/gcc/testsuite/g++.dg/modules/enum-13_a.C
new file mode 100644 (file)
index 0000000..1d0c86d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/enum-13_a.C
@@ -0,0 +1,23 @@
+// PR c++/104919
+// PR c++/106009
+// { dg-additional-options -fmodules-ts }
+// { dg-module-cmi Enum13 }
+
+export module Enum13;
+
+export
+constexpr int f() {
+  enum E { e = 42 };
+  return e;
+}
+
+template<class T>
+constexpr int ft(T) {
+  enum E { e = 43 };
+  return e;
+}
+
+export
+constexpr int g() {
+  return ft(0);
+}

diff --git a/gcc/testsuite/g++.dg/modules/enum-13_b.C b/gcc/testsuite/g++.dg/modules/enum-13_b.C
new file mode 100644 (file)
index 0000000..16d4a7c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/enum-13_b.C
@@ -0,0 +1,8 @@
+// PR c++/104919
+// PR c++/106009
+// { dg-additional-options -fmodules-ts }
+
+import Enum13;
+
+static_assert(f() == 42);
+static_assert(g() == 43);

diff --git a/gcc/testsuite/g++.dg/modules/tdef-7.h b/gcc/testsuite/g++.dg/modules/tdef-7.h
index 5bc21e176cb4e0eed4a3105e70b7452e73380071..6125f0460e29f0ce01a037b1b91272e420b39ff5 100644 (file)
--- a/gcc/testsuite/g++.dg/modules/tdef-7.h
+++ b/gcc/testsuite/g++.dg/modules/tdef-7.h
@@ -1,7 +1,5 @@
 
 constexpr void duration_cast ()
 {
-  // the constexpr's body's clone merely duplicates the TYPE_DECL, it
-  // doesn't create a kosher typedef
   typedef int __to_rep;
 }

diff --git a/gcc/testsuite/g++.dg/modules/tdef-7_b.C b/gcc/testsuite/g++.dg/modules/tdef-7_b.C
index c526ca8dd4fbc2c9659efb6c34c0a31f92a9a912..ea76458715b75b169681a5f68ec770d41fb9a5fe 100644 (file)
--- a/gcc/testsuite/g++.dg/modules/tdef-7_b.C
+++ b/gcc/testsuite/g++.dg/modules/tdef-7_b.C
@@ -5,5 +5,5 @@ import "tdef-7_a.H";
 
 // { dg-final { scan-lang-dump-times {merge key \(matched\) function_decl:'::duration_cast} 1 module } }
 // { dg-final { scan-lang-dump-not {merge key \(new\)} module } }
-// { dg-final { scan-lang-dump-times {merge key \(unique\) type_decl:'#null#'} 2 module } }
+// { dg-final { scan-lang-dump-times {merge key \(unique\) type_decl:'#null#'} 1 module } }
 // { dg-final { scan-lang-dump-times {Cloned:-[0-9]* typedef integer_type:'::duration_cast::__to_rep'} 1 module } }

diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc
index 75c10eb7dfc2d99c00a661cee941e8e00cf67f59..f0a067f5812a1ad687fb27416447f2eafb398f17 100644 (file)
--- a/gcc/tree-inline.cc
+++ b/gcc/tree-inline.cc
@@ -370,7 +370,7 @@ remap_decl (tree decl, copy_body_data *id)
         need this decl for TYPE_STUB_DECL.  */
       insert_decl_map (id, decl, t);
 
-      if (!DECL_P (t))
+      if (!DECL_P (t) || t == decl)
        return t;
 
       /* Remap types, if necessary.  */
@@ -765,7 +765,7 @@ remap_decls (tree decls, vec<tree, va_gc> **nonlocalized_list,
         TREE_CHAIN.  If we remapped this variable to the return slot, it's
         already declared somewhere else, so don't declare it here.  */
 
-      if (new_var == id->retvar)
+      if (new_var == old_var || new_var == id->retvar)
        ;
       else if (!new_var)
         {
@@ -6610,7 +6610,15 @@ copy_fn (tree fn, tree& parms, tree& result)
   id.src_cfun = DECL_STRUCT_FUNCTION (fn);
   id.decl_map = &decl_map;
 
-  id.copy_decl = copy_decl_no_change;
+  id.copy_decl = [] (tree decl, copy_body_data *id)
+    {
+      if (TREE_CODE (decl) == TYPE_DECL || TREE_CODE (decl) == CONST_DECL)
+       /* Don't make copies of local types or injected enumerators,
+          the C++ constexpr evaluator doesn't need them and they
+          confuse modules streaming.  */
+       return decl;
+      return copy_decl_no_change (decl, id);
+    };
   id.transform_call_graph_edges = CB_CGE_DUPLICATE;
   id.transform_new_cfg = false;
   id.transform_return_to_modify = false;