public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] c++/modules: depending local enums [PR104919, PR106009]
@ 2024-02-29 20:56 Patrick Palka
  2024-03-01 13:32 ` Jason Merrill
  0 siblings, 1 reply; 14+ messages in thread
From: Patrick Palka @ 2024-02-29 20:56 UTC (permalink / raw)
  To: gcc-patches; +Cc: jason, nathan, Patrick Palka

Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
OK for trunk?

-- >8 --

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 ultimately causes streaming to fail due to the enum
definition not being streamed before uses of its enumerators are streamed,
as far as I can tell.

The code responsible for adding 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 function or a function template instantiation.  (For a
TYPE_DECL from a function template definition, we'll be dealing with the
corresponding TEMPLATE_DECL instead, so we'll add the dependency.)

Local classes seem fine as-is but perhaps by accident: with a local
class we end up depending on the injected-class-name of the local class
since it satisfies the above conditions.  A local enum doesn't have
such a TYPE_DECL member than we can depend on (its CONST_DECLs are
handled earlier as tt_enum_decl tags).

This patch attempts to fix this by keeping the 'sneakoscope' flag set
while walking the definition of a function, so that we add this needed
dependency between a containing function (non-template or specialization)
and its local types.  Currently it's set only when walking the
declaration (presumably to catch local types that escape via a deduced
return type), but it seems to make sense to add a dependency regardless
of the type escapes.

This was nearly enough to make things work, except we now ran into
issues with the local TYPE/CONST_DECL copies when streaming the
constexpr version of a function body.  It occurred to me that we don't
need to make copies of local types when copying a constexpr function
body; only VAR_DECLs etc need to be copied for sake of recursive
constexpr calls.  So this patch adjusts copy_fn accordingly.

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

gcc/cp/ChangeLog:

	* module.cc (depset::hash::find_dependencies): Keep sneakoscope
	set when walking the definition.

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:
	* g++.dg/modules/tdef-7_b.C:
	* g++.dg/modules/enum-13_a.C: New test.
	* g++.dg/modules/enum-13_b.C: New test.
---
 gcc/cp/module.cc                         |  2 +-
 gcc/testsuite/g++.dg/modules/enum-13_a.C | 23 +++++++++++++++++++++++
 gcc/testsuite/g++.dg/modules/enum-13_b.C |  8 ++++++++
 gcc/testsuite/g++.dg/modules/tdef-7.h    |  2 --
 gcc/testsuite/g++.dg/modules/tdef-7_b.C  |  2 +-
 gcc/tree-inline.cc                       | 14 +++++++++++---
 6 files changed, 44 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/enum-13_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/enum-13_b.C

diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 66ef0bcaa94..29e57716297 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -13547,9 +13547,9 @@ depset::hash::find_dependencies (module_state *module)
 		  /* 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);
+		  sneakoscope = false;
 		}
 	      walker.end ();
 
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
index 00000000000..2e570c6c4fb
--- /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 blah { 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
index 00000000000..16d4a7c8ac5
--- /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 5bc21e176cb..6125f0460e2 100644
--- 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 c526ca8dd4f..ea76458715b 100644
--- 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 75c10eb7dfc..b35760ae9f0 100644
--- 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 enumerators, the C++
+	   constexpr evaluator doesn't need them and they cause problems
+	   with modules.  */
+	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;
-- 
2.44.0.53.g0f9d4d28b7


^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-03-01 20:34 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-29 20:56 [PATCH] c++/modules: depending local enums [PR104919, PR106009] Patrick Palka
2024-03-01 13:32 ` Jason Merrill
2024-03-01 15:00   ` Patrick Palka
2024-03-01 15:32     ` Jason Merrill
2024-03-01 16:12       ` Jason Merrill
2024-03-01 16:45         ` Patrick Palka
2024-03-01 17:52           ` Jason Merrill
2024-03-01 16:39       ` Patrick Palka
2024-03-01 17:08         ` Patrick Palka
2024-03-01 18:04           ` Jason Merrill
2024-03-01 18:28             ` Patrick Palka
2024-03-01 19:06               ` Jason Merrill
2024-03-01 19:34                 ` Patrick Palka
2024-03-01 20:34                   ` Jason Merrill

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).