c++/modules: Fix revealing with using-decls [PR114867]

This patch fixes a couple issues with the current handling of revealing
declarations with using-decls.

Firstly, doing 'remove_node' when handling function overload sets is not
safe, because it not only mutates the OVERLOAD we're walking over but
potentially any other references to this OVERLOAD that are cached from
phase-1 template lookup.  This causes the attached using-17 testcase to
fail because the overload set in 'X::test()' no longer contains the
'ns::f(T)' template once instantiated at the end of the file.

This patch works around this by simply not removing the old declaration.
This does make the overload list potentially longer than it otherwise
would have been, but only when re-exporting the same set of functions in
a using-decl.  Additionally, because 'ovl_insert' always prepends these
newly inserted overloads, repeated exported using-decls won't continue
to add declarations, as the first exported using-decl will be found
before the original (unexported) declaration.

Another, related, issue is that using-decls of GMF entities currently
doesn't mark them as reachable unless they are also exported, and thus
they may not be available in e.g. module implementation units.  We solve
this with a new flag on OVERLOADs set when they are declared within the
module purview.  This starts to run into the more general issue of
handling using-decls of non-functions (see e.g. PR114863) but by just
marking such GMF entities as purview we can work around this for now.

This also allows us to get rid of the special-casing of exported
using-decls in 'add_binding_entity', which was incorrect anyway: a
non-exported using-decl still needs to be emitted anyway if it lives in
the module purview, even if referring to a non-purview item.

	PR c++/114867

gcc/cp/ChangeLog:

	* cp-tree.h (OVL_PURVIEW_P): New.
	(ovl_iterator::purview_p): New.
	* module.cc (depset::hash::add_binding_entity): Only ignore
	entities not within module purview. Set OVL_PURVIEW_P on new
	OVERLOADs for emitted declarations.
	(module_state::read_cluster): Imported using-decls are always
	in purview, mark as OVL_PURVIEW_P.
	* name-lookup.h (enum WMB_Flags): New WMB_Purview flag.
	* name-lookup.cc (walk_module_binding): Set WMB_Purview as
	needed.
	(do_nonmember_using_decl): Don't remove from existing OVERLOADs.
	Also reveal non-exported decls. Also reveal 'extern "C"' decls.
	Add workaround to reveal non-function decls.
	* tree.cc (ovl_insert): Adjust to also set OVL_PURVIEW_P when
	needed.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/using-17_a.C: New test.
	* g++.dg/modules/using-17_b.C: New test.
	* g++.dg/modules/using-18_a.C: New test.
	* g++.dg/modules/using-18_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 6206482c602dddfdb92187f90324d24d6700078b..565e4a9290e22e465661221e74f5b695c6ca8296 100644 (file)
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -813,6 +813,8 @@ typedef struct ptrmem_cst * ptrmem_cst_t;
 #define OVL_LOOKUP_P(NODE)     TREE_LANG_FLAG_4 (OVERLOAD_CHECK (NODE))
 /* If set, this OVL_USING_P overload is exported.  */
 #define OVL_EXPORT_P(NODE)     TREE_LANG_FLAG_5 (OVERLOAD_CHECK (NODE))
+/* If set, this OVL_USING_P overload is in the module purview.  */
+#define OVL_PURVIEW_P(NODE)    (OVERLOAD_CHECK (NODE)->base.public_flag)
 /* If set, this overload includes name-independent declarations.  */
 #define OVL_NAME_INDEPENDENT_DECL_P(NODE) \
   TREE_LANG_FLAG_6 (OVERLOAD_CHECK (NODE))
@@ -887,6 +889,11 @@ class ovl_iterator {
     return (TREE_CODE (ovl) == USING_DECL
            || (TREE_CODE (ovl) == OVERLOAD && OVL_USING_P (ovl)));
   }
+  /* Whether this using is in the module purview.  */
+  bool purview_p () const
+  {
+    return OVL_PURVIEW_P (get_using ());
+  }
   /* Whether this using is being exported.  */
   bool exporting_p () const
   {

diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 3f8f84bb9fd2ccd206f925c2043b3c608b3e37a0..ed24814b601d22c99a82378b0a03899ae0fc4171 100644 (file)
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -13137,9 +13137,8 @@ depset::hash::add_binding_entity (tree decl, WMB_Flags flags, void *data_)
        inner = DECL_TEMPLATE_RESULT (inner);
 
       if ((!DECL_LANG_SPECIFIC (inner) || !DECL_MODULE_PURVIEW_P (inner))
-         && !((flags & WMB_Using) && (flags & WMB_Export)))
-       /* Ignore global module fragment entities unless explicitly
-          exported with a using declaration.  */
+         && !((flags & WMB_Using) && (flags & WMB_Purview)))
+       /* Ignore entities not within the module purview.  */
        return false;
 
       if (VAR_OR_FUNCTION_DECL_P (inner)
@@ -13189,6 +13188,7 @@ depset::hash::add_binding_entity (tree decl, WMB_Flags flags, void *data_)
                {
                  if (!(flags & WMB_Hidden))
                    d->clear_hidden_binding ();
+                 OVL_PURVIEW_P (d->get_entity ()) = true;
                  if (flags & WMB_Export)
                    OVL_EXPORT_P (d->get_entity ()) = true;
                  return bool (flags & WMB_Export);
@@ -13230,6 +13230,7 @@ depset::hash::add_binding_entity (tree decl, WMB_Flags flags, void *data_)
          decl = ovl_make (decl, NULL_TREE);
          if (!unscoped_enum_const_p)
            OVL_USING_P (decl) = true;
+         OVL_PURVIEW_P (decl) = true;
          if (flags & WMB_Export)
            OVL_EXPORT_P (decl) = true;
        }
@@ -15311,6 +15312,7 @@ module_state::read_cluster (unsigned snum)
                          {
                            dedup = true;
                            OVL_USING_P (decls) = true;
+                           OVL_PURVIEW_P (decls) = true;
                            if (flags & cbf_export)
                              OVL_EXPORT_P (decls) = true;
                          }

diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
index f1f8c19feb1cf35328d5bbee86748e1d840a060b..3d3e20f48cbdec5084f890fa1375b8f9b59b6fb6 100644 (file)
--- a/gcc/cp/name-lookup.cc
+++ b/gcc/cp/name-lookup.cc
@@ -4234,6 +4234,8 @@ walk_module_binding (tree binding, bitmap partitions,
          if (iter.using_p ())
            {
              flags = WMB_Flags (flags | WMB_Using);
+             if (iter.purview_p ())
+               flags = WMB_Flags (flags | WMB_Purview);
              if (iter.exporting_p ())
                flags = WMB_Flags (flags | WMB_Export);
            }
@@ -4307,6 +4309,8 @@ walk_module_binding (tree binding, bitmap partitions,
                        if (iter.using_p ())
                          {
                            flags = WMB_Flags (flags | WMB_Using);
+                           if (iter.purview_p ())
+                             flags = WMB_Flags (flags | WMB_Purview);
                            if (iter.exporting_p ())
                              flags = WMB_Flags (flags | WMB_Export);
                          }
@@ -5210,9 +5214,10 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
       for (lkp_iterator usings (lookup.value); usings; ++usings)
        {
          tree new_fn = *usings;
-         bool exporting = revealing_p && module_exporting_p ();
-         if (exporting)
-           exporting = check_can_export_using_decl (new_fn);
+         tree inner = STRIP_TEMPLATE (new_fn);
+         bool exporting_p = revealing_p && module_exporting_p ();
+         if (exporting_p)
+           exporting_p = check_can_export_using_decl (new_fn);
 
          /* [namespace.udecl]
 
@@ -5239,18 +5244,18 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
                    ;
                  else if (old.using_p ())
                    {
-                     if (exporting)
-                       /* Update in place.  'tis ok.  */
+                     /* Update in place.  'tis ok.  */
+                     OVL_PURVIEW_P (old.get_using ()) = true;
+                     if (exporting_p)
                        OVL_EXPORT_P (old.get_using ()) = true;
-                     ;
-                   }
-                 else if (DECL_MODULE_EXPORT_P (new_fn))
-                   ;
-                 else
-                   {
-                     value = old.remove_node (value);
-                     found = false;
                    }
+                 else if (!DECL_LANG_SPECIFIC (inner)
+                          || !DECL_MODULE_PURVIEW_P (inner))
+                   /* We need to re-insert this function as a revealed
+                      (possibly exported) declaration.  We can't remove
+                      the existing decl because that will change any
+                      overloads cached in template functions.  */
+                   found = false;
                  break;
                }
              else if (old.using_p ())
@@ -5261,8 +5266,14 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
                continue; /* Parameters do not match.  */
              else if (decls_match (new_fn, old_fn))
                {
-                 /* Extern "C" in different namespaces.  */
+                 /* Extern "C" in different namespaces.  But similarly
+                    to above, if revealing a not-revealed thing we may
+                    need to reinsert.  */
                  found = true;
+                 if (revealing_p
+                     && (!DECL_LANG_SPECIFIC (inner)
+                         || !DECL_MODULE_PURVIEW_P (inner)))
+                   found = false;
                  break;
                }
              else
@@ -5278,7 +5289,7 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
            /* Unlike the decl-pushing case we don't drop anticipated
               builtins here.  They don't cause a problem, and we'd
               like to match them with a future declaration.  */
-           value = ovl_insert (new_fn, value, 1 + exporting);
+           value = ovl_insert (new_fn, value, 1 + revealing_p + exporting_p);
        }
     }
   else if (value
@@ -5291,32 +5302,32 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
     }
   else if (insert_p)
     {
-      if (revealing_p
-         && module_exporting_p ()
-         && check_can_export_using_decl (lookup.value)
-         && lookup.value == value
-         && !DECL_MODULE_EXPORT_P (value))
+      /* FIXME: Handle exporting declarations from a different scope
+        without also marking those declarations as exported.
+        This will require not just binding directly to the underlying
+        value; see c++/114863 and c++/114865.  We allow this for purview
+        declarations for now as this doesn't (currently) cause ICEs
+        later down the line, but this should be revisited.  */
+      if (revealing_p)
        {
-         /* We're redeclaring the same value, but this time as
-            newly exported: make sure to mark it as such.  */
-         if (TREE_CODE (value) == TEMPLATE_DECL)
-           {
-             DECL_MODULE_EXPORT_P (value) = true;
-
-             tree result = DECL_TEMPLATE_RESULT (value);
-             retrofit_lang_decl (result);
-             DECL_MODULE_PURVIEW_P (result) = true;
-             DECL_MODULE_EXPORT_P (result) = true;
-           }
-         else
+         if (module_exporting_p ()
+             && check_can_export_using_decl (lookup.value)
+             && lookup.value == value
+             && !DECL_MODULE_EXPORT_P (lookup.value))
            {
-             retrofit_lang_decl (value);
-             DECL_MODULE_PURVIEW_P (value) = true;
-             DECL_MODULE_EXPORT_P (value) = true;
+             /* We're redeclaring the same value, but this time as
+                newly exported: make sure to mark it as such.  */
+             if (TREE_CODE (lookup.value) == TEMPLATE_DECL)
+               DECL_MODULE_EXPORT_P (DECL_TEMPLATE_RESULT (lookup.value)) = true;
+             DECL_MODULE_EXPORT_P (lookup.value) = true;
            }
+         /* We're also revealing this declaration with a using-decl,
+            so mark it as purview to ensure it's emitted.  */
+         tree inner = STRIP_TEMPLATE (lookup.value);
+         retrofit_lang_decl (inner);
+         DECL_MODULE_PURVIEW_P (inner) = true;
        }
-      else
-       value = lookup.value;
+      value = lookup.value;
     }
   
   /* Now the type binding.  */
@@ -5329,20 +5340,17 @@ do_nonmember_using_decl (name_lookup &lookup, bool fn_scope_p,
        }
       else if (insert_p)
        {
-         if (revealing_p
-             && module_exporting_p ()
-             && check_can_export_using_decl (lookup.type)
-             && lookup.type == type
-             && !DECL_MODULE_EXPORT_P (type))
+         if (revealing_p)
            {
-             /* We're redeclaring the same type, but this time as
-                newly exported: make sure to mark it as such.  */
-             retrofit_lang_decl (type);
-             DECL_MODULE_PURVIEW_P (type) = true;
-             DECL_MODULE_EXPORT_P (type) = true;
+             /* As with revealing value bindings.  */
+             if (module_exporting_p ()
+                 && check_can_export_using_decl (lookup.type)
+                 && lookup.type == type)
+               DECL_MODULE_EXPORT_P (lookup.type) = true;
+             retrofit_lang_decl (lookup.type);
+             DECL_MODULE_PURVIEW_P (lookup.type) = true;
            }
-         else
-           type = lookup.type;
+         type = lookup.type;
        }
     }
 

diff --git a/gcc/cp/name-lookup.h b/gcc/cp/name-lookup.h
index d2371323337ef78c1b9d60ea67509b773f78124d..5cf6ae6374af6a4fa3f03986aa2e1466984bf592 100644 (file)
--- a/gcc/cp/name-lookup.h
+++ b/gcc/cp/name-lookup.h
@@ -495,6 +495,7 @@ enum WMB_Flags
   WMB_Export = 1 << 1,
   WMB_Using = 1 << 2,
   WMB_Hidden = 1 << 3,
+  WMB_Purview = 1 << 4,
 };
 
 extern unsigned walk_module_binding (tree binding, bitmap partitions,

diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc
index 72dd46e1bd1c5b6cfd4ef00fbf13eacba6b4cf2e..d2a8f79ffab46bf5af9541f2a147bde674fd1ddb 100644 (file)
--- a/gcc/cp/tree.cc
+++ b/gcc/cp/tree.cc
@@ -2353,11 +2353,11 @@ ovl_make (tree fn, tree next)
   return result;
 }
 
-/* Add FN to the (potentially NULL) overload set OVL.  USING_OR_HIDDEN is >
-   zero if this is a using-decl.  It is > 1 if we're exporting the
-   using decl.  USING_OR_HIDDEN is < 0, if FN is hidden.  (A decl
-   cannot be both using and hidden.)  We keep the hidden decls first,
-   but remaining ones are unordered.  */
+/* Add FN to the (potentially NULL) overload set OVL.  USING_OR_HIDDEN is > 0
+   if this is a using-decl.  It is > 1 if we're revealing the using decl.
+   It is > 2 if we're also exporting it.  USING_OR_HIDDEN is < 0, if FN is
+   hidden.  (A decl cannot be both using and hidden.)  We keep the hidden
+   decls first, but remaining ones are unordered.  */
 
 tree
 ovl_insert (tree fn, tree maybe_ovl, int using_or_hidden)
@@ -2384,6 +2384,8 @@ ovl_insert (tree fn, tree maybe_ovl, int using_or_hidden)
        {
          OVL_DEDUP_P (maybe_ovl) = OVL_USING_P (maybe_ovl) = true;
          if (using_or_hidden > 1)
+           OVL_PURVIEW_P (maybe_ovl) = true;
+         if (using_or_hidden > 2)
            OVL_EXPORT_P (maybe_ovl) = true;
        }
     }

diff --git a/gcc/testsuite/g++.dg/modules/using-17_a.C b/gcc/testsuite/g++.dg/modules/using-17_a.C
new file mode 100644 (file)
index 0000000..de601ea
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/using-17_a.C
@@ -0,0 +1,31 @@
+// PR c++/114867
+// { dg-additional-options "-fmodules-ts -Wno-global-module" }
+// { dg-module-cmi M }
+
+module;
+
+namespace ns {
+  template <typename T> void f(T);
+
+  namespace inner {
+    class E {};
+    int f(E);
+  }
+  using inner::f;
+}
+
+export module M;
+
+template <typename T>
+struct X {
+  void test() { ns::f(T{}); }
+};
+template struct X<int>;
+
+export namespace ns {
+  using ns::f;
+}
+
+export auto get_e() {
+  return ns::inner::E{};
+}

diff --git a/gcc/testsuite/g++.dg/modules/using-17_b.C b/gcc/testsuite/g++.dg/modules/using-17_b.C
new file mode 100644 (file)
index 0000000..e315821
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/using-17_b.C
@@ -0,0 +1,13 @@
+// PR c++/114867
+// { dg-additional-options "-fmodules-ts" }
+
+import M;
+
+int main() {
+  int a = 0;
+  ns::f(a);
+
+  // Should also still find the inner::f overload
+  auto e = get_e();
+  int r = ns::f(e);
+}

diff --git a/gcc/testsuite/g++.dg/modules/using-18_a.C b/gcc/testsuite/g++.dg/modules/using-18_a.C
new file mode 100644 (file)
index 0000000..72c2bf8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/using-18_a.C
@@ -0,0 +1,29 @@
+// { dg-additional-options "-fmodules-ts -Wno-global-module" }
+// { dg-module-cmi M }
+// Test revealing non-exported declarations still prevents
+// needed GMF declarations from being discarded
+
+module;
+
+struct A {};
+int f();
+
+namespace ns {
+  struct B {};
+  int g();
+}
+
+extern "C" int h();
+namespace ns {
+  extern "C" int h();
+}
+
+export module M;
+
+using ::A;
+using ::f;
+
+using ns::B;
+using ns::g;
+
+using ns::h;

diff --git a/gcc/testsuite/g++.dg/modules/using-18_b.C b/gcc/testsuite/g++.dg/modules/using-18_b.C
new file mode 100644 (file)
index 0000000..f5f608d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/using-18_b.C
@@ -0,0 +1,11 @@
+// { dg-additional-options "-fmodules-ts" }
+
+module M;
+
+void test() {
+  A a;
+  B b;
+  int x = f();
+  int y = g();
+  int z = h();
+}