On Mon, 26 Feb 2024, Patrick Palka wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this approach
> look reasonable?
>
> -- >8 --
>
> One known missing piece in the modules implementation is merging of a
> streamed-in local class with the corresponding in-TU version of the
> local class. This missing piece turns out to cause a hard-to-reduce
> use-after-free GC issue due to the entity_ary not being marked as a GC
> root (deliberately), and manifests as a serialization error on stream-in
> as in PR99426 (see comment #6 for a reduction). It's also reproducible
> on trunk when running the xtreme-header tests without -fno-module-lazy.
>
> This patch makes us merge such local classes according to their position
> within the containing function's definition, similar to how we merge
> FIELD_DECLs of a class according to their index in the TYPE_FIELDS
> list.
>
> PR c++/99426
>
> gcc/cp/ChangeLog:
>
> * module.cc (merge_kind::MK_local_class): New enumerator.
> (merge_kind_name): Update.
> (trees_out::chained_decls): Move BLOCK-specific handling
> of DECL_LOCAL_DECL_P decls to ...
> (trees_out::core_vals) <case BLOCK>: ... here. Stream
> BLOCK_VARS manually.
> (trees_in::core_vals) <case BLOCK>: Stream BLOCK_VARS
> manually. Handle deduplicated local classes.
> (trees_out::key_local_class): Define.
> (trees_in::key_local_class): Define.
> (trees_out::get_merge_kind) <case FUNCTION_DECL>: Return
> MK_local_class for a local class.
> (trees_out::key_mergeable) <case FUNCTION_DECL>: Use
> key_local_class.
> (trees_in::key_mergeable) <case FUNCTION_DECL>: Likewise.
> (trees_in::is_matching_decl): Be flexible with type mismatches
> for local entities.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/modules/xtreme-header-7_a.H: New test.
> * g++.dg/modules/xtreme-header-7_b.C: New test.
> ---
> gcc/cp/module.cc | 167 +++++++++++++++---
> .../g++.dg/modules/xtreme-header-7_a.H | 4 +
> .../g++.dg/modules/xtreme-header-7_b.C | 6 +
> 3 files changed, 149 insertions(+), 28 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/modules/xtreme-header-7_a.H
> create mode 100644 gcc/testsuite/g++.dg/modules/xtreme-header-7_b.C
>
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index fa91c6ff9cb..f77f73a59ed 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -2771,6 +2771,7 @@ enum merge_kind
>
> MK_enum, /* Found by CTX, & 1stMemberNAME. */
> MK_keyed, /* Found by key & index. */
> + MK_local_class, /* Found by CTX, index. */
>
> MK_friend_spec, /* Like named, but has a tmpl & args too. */
> MK_local_friend, /* Found by CTX, index. */
> @@ -2799,7 +2800,7 @@ static char const *const merge_kind_name[MK_hwm] =
> "unique", "named", "field", "vtable", /* 0...3 */
> "asbase", "partial", "enum", "attached", /* 4...7 */
>
> - "friend spec", "local friend", NULL, NULL, /* 8...11 */
> + "local class", "friend spec", "local friend", NULL, /* 8...11 */
> NULL, NULL, NULL, NULL,
>
> "type spec", "type tmpl spec", /* 16,17 type (template). */
> @@ -2928,6 +2929,7 @@ public:
> unsigned binfo_mergeable (tree *);
>
> private:
> + tree key_local_class (const merge_key&, tree);
> uintptr_t *find_duplicate (tree existing);
> void register_duplicate (tree decl, tree existing);
> /* Mark as an already diagnosed bad duplicate. */
> @@ -3086,6 +3088,7 @@ public:
> void binfo_mergeable (tree binfo);
>
> private:
> + void key_local_class (merge_key&, tree, tree);
> bool decl_node (tree, walk_kind ref);
> void type_node (tree);
> void tree_value (tree);
> @@ -4952,18 +4955,7 @@ void
> trees_out::chained_decls (tree decls)
> {
> for (; decls; decls = DECL_CHAIN (decls))
> - {
> - if (VAR_OR_FUNCTION_DECL_P (decls)
> - && DECL_LOCAL_DECL_P (decls))
> - {
> - /* Make sure this is the first encounter, and mark for
> - walk-by-value. */
> - gcc_checking_assert (!TREE_VISITED (decls)
> - && !DECL_TEMPLATE_INFO (decls));
> - mark_by_value (decls);
> - }
> - tree_node (decls);
> - }
> + tree_node (decls);
> tree_node (NULL_TREE);
> }
>
> @@ -6204,7 +6196,21 @@ trees_out::core_vals (tree t)
>
> /* DECL_LOCAL_DECL_P decls are first encountered here and
> streamed by value. */
> - chained_decls (t->block.vars);
> + for (tree decls = t->block.vars; decls; decls = DECL_CHAIN (decls))
> + {
> + if (VAR_OR_FUNCTION_DECL_P (decls)
> + && DECL_LOCAL_DECL_P (decls))
> + {
> + /* Make sure this is the first encounter, and mark for
> + walk-by-value. */
> + gcc_checking_assert (!TREE_VISITED (decls)
> + && !DECL_TEMPLATE_INFO (decls));
> + mark_by_value (decls);
> + }
> + tree_node (decls);
> + }
> + tree_node (NULL_TREE);
> +
> /* nonlocalized_vars is a middle-end thing. */
> WT (t->block.subblocks);
> WT (t->block.supercontext);
> @@ -6717,7 +6723,34 @@ trees_in::core_vals (tree t)
> case BLOCK:
> t->block.locus = state->read_location (*this);
> t->block.end_locus = state->read_location (*this);
> - t->block.vars = chained_decls ();
> +
> + for (tree *chain = &t->block.vars;;)
> + if (tree decl = tree_node ())
> + {
> + /* For a deduplicated local class, chain the to-be-discarded
> + decl not the in-TU decl (which is already chained to in-TU
> + entities). */
> + if (is_duplicate (decl))
> + decl = maybe_duplicate (decl);
> + else if (DECL_IMPLICIT_TYPEDEF_P (decl)
> + && TYPE_TEMPLATE_INFO (TREE_TYPE (decl)))
> + {
> + tree tmpl = TYPE_TI_TEMPLATE (TREE_TYPE (decl));
> + if (DECL_TEMPLATE_RESULT (tmpl) == decl && is_duplicate (tmpl))
> + decl = DECL_TEMPLATE_RESULT (maybe_duplicate (tmpl));
> + }
> +
> + if (!DECL_P (decl) || DECL_CHAIN (decl))
> + {
> + set_overrun ();
> + break;
> + }
> + *chain = decl;
> + chain = &DECL_CHAIN (decl);
> + }
> + else
> + break;
> +
> /* nonlocalized_vars is middle-end. */
> RT (t->block.subblocks);
> RT (t->block.supercontext);
> @@ -10335,6 +10368,83 @@ trees_in::fn_parms_fini (int tag, tree fn, tree
> existing, bool is_defn)
> }
> }
>
> +/* Encode into KEY the position of the local class declaration DECL
> + within FN. The position is encoded as the index of the innermost
> + BLOCK (numbered in BFS order) along with the index within its
> + BLOCK_VARS list. */
> +
> +void
> +trees_out::key_local_class (merge_key& key, tree decl, tree fn)
> +{
> + auto_vec<tree, 4> blocks;
> + blocks.quick_push (DECL_INITIAL (fn));
> + unsigned block_ix = 0;
> + while (block_ix != blocks.length ())
> + {
> + tree block = blocks[block_ix];
> + unsigned decl_ix = 0;
> + for (tree var = BLOCK_VARS (block); var; var = DECL_CHAIN (var))
> + {
> + if (TREE_CODE (var) != TYPE_DECL)
> + continue;
> + if (var == decl)
> + {
> + key.index = (block_ix << 10) | decl_ix;
> + return;
> + }
> + ++decl_ix;
> + }
> + for (tree sub = BLOCK_SUBBLOCKS (block); sub; sub = BLOCK_CHAIN (sub))
> + blocks.safe_push (sub);
> + ++block_ix;
> + }
> +
> + /* Not-found value. */
> + key.index = 1023;
> +}
> +
> +/* Look up the local class corresponding at the position encoded by
> + KEY within FN. */
> +
> +tree
> +trees_in::key_local_class (const merge_key& key, tree fn)
> +{
> + if (!DECL_INITIAL (fn))
> + return NULL_TREE;
> +
> + const unsigned block_pos = key.index >> 10;
> + const unsigned decl_pos = key.index & 1023;
> +
> + if (decl_pos == 1023)
> + return NULL_TREE;
> +
> + auto_vec<tree, 4> blocks;
> + blocks.quick_push (DECL_INITIAL (fn));
> + unsigned block_ix = 0;
> + while (block_ix != blocks.length ())
> + {
> + tree block = blocks[block_ix];
> + if (block_ix == block_pos)
> + {
> + unsigned decl_ix = 0;
> + for (tree var = BLOCK_VARS (block); var; var = DECL_CHAIN (var))
> + {
> + if (TREE_CODE (var) != TYPE_DECL)
> + continue;
> + if (decl_ix == decl_pos)
> + return var;
> + ++decl_ix;
> + }
> + return NULL_TREE;
> + }
> + for (tree sub = BLOCK_SUBBLOCKS (block); sub; sub = BLOCK_CHAIN (sub))
> + blocks.safe_push (sub);
> + ++block_ix;
> + }
> +
> + return NULL_TREE;
> +}
> +
> /* DEP is the depset of some decl we're streaming by value. Determine
> the merging behaviour. */
>
> @@ -10454,17 +10564,10 @@ trees_out::get_merge_kind (tree decl, depset *dep)
> gcc_unreachable ();
>
> case FUNCTION_DECL:
> - // FIXME: This can occur for (a) voldemorty TYPE_DECLS
> - // (which are returned from a function), or (b)
> - // block-scope class definitions in template functions.
> - // These are as unique as the containing function. While
> - // on read-back we can discover if the CTX was a
> - // duplicate, we don't have a mechanism to get from the
> - // existing CTX to the existing version of this decl.
> gcc_checking_assert
> (DECL_IMPLICIT_TYPEDEF_P (STRIP_TEMPLATE (decl)));
>
> - mk = MK_unique;
> + mk = MK_local_class;
> break;
>
> case RECORD_TYPE:
> @@ -10768,6 +10871,10 @@ trees_out::key_mergeable (int tag, merge_kind mk,
> tree decl, tree inner,
> }
> break;
>
> + case MK_local_class:
> + key_local_class (key, STRIP_TEMPLATE (decl), container);
> + break;
> +
> case MK_enum:
> {
> /* Anonymous enums are located by their first identifier,
> @@ -11117,11 +11224,10 @@ trees_in::key_mergeable (int tag, merge_kind mk,
> tree decl, tree inner,
> break;
>
> case FUNCTION_DECL:
> - // FIXME: What about a voldemort? how do we find what it
> - // duplicates? Do we have to number vmorts relative to
> - // their containing function? But how would that work
> - // when matching an in-TU declaration?
> - kind = "unique";
> + gcc_checking_assert (mk == MK_local_class);
> + existing = key_local_class (key, container);
> + if (existing && inner != decl)
> + existing = TYPE_TI_TEMPLATE (TREE_TYPE (existing));
> break;
>
> case TYPE_DECL:
> @@ -11374,6 +11480,11 @@ trees_in::is_matching_decl (tree existing, tree
> decl, bool is_typedef)
> /* Just like duplicate_decls, presum the user knows what
> they're doing in overriding a builtin. */
> TREE_TYPE (existing) = TREE_TYPE (decl);
> + else if (decl_function_context (decl))
> + /* The type of a mergeable local entity (such as a function scope
> + capturing lambda's closure type fields) can depend on an
> + unmergeable local entity (such as a local variable), so type
> + equality isn't feasible in general for local entities. */;
> else
> {
> // FIXME:QOI Might be template specialization from a module,
> diff --git a/gcc/testsuite/g++.dg/modules/xtreme-header-7_a.H
> b/gcc/testsuite/g++.dg/modules/xtreme-header-7_a.H
> new file mode 100644
> index 00000000000..bf7859fba99
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/xtreme-header-7_a.H
> @@ -0,0 +1,4 @@
> +// { dg-additional-options -fmodule-header }
> +
> +// { dg-module-cmi {} }
> +#include "xtreme-header.h"
> diff --git a/gcc/testsuite/g++.dg/modules/xtreme-header-7_b.C
> b/gcc/testsuite/g++.dg/modules/xtreme-header-7_b.C
> new file mode 100644
> index 00000000000..03f3dc1bae6
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/xtreme-header-7_b.C
> @@ -0,0 +1,6 @@
> +// A version of xtreme-header_{a.H,b.C} that doesn't pass
> +// -fno-module-lazy.
> +// { dg-additional-options -fmodules-ts }
> +
> +#include "xtreme-header.h"
> +import "xtreme-header-7_a.H";
> --
Consider the following minimal testcase added that verifies we now
properly merge local classes (and local enums):
gcc/testsuite/g++.dg/modules/merge-17.h | 28 ++++++++++++++++++++++++++++
gcc/testsuite/g++.dg/modules/merge-17_a.H | 3 +++
gcc/testsuite/g++.dg/modules/merge-17_b.C | 3 +++
3 files changed, 34 insertions(+)
diff --git a/gcc/testsuite/g++.dg/modules/merge-17.h
b/gcc/testsuite/g++.dg/modules/merge-17.h
new file mode 100644
index 00000000000..246ccd8011d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/merge-17.h
@@ -0,0 +1,28 @@
+// PR c++/99426
+
+inline auto f() {
+ struct A { int m = 42; };
+ return A{};
+}
+
+template<class T>
+inline auto ft() {
+ decltype(+T()) x;
+ return [&x] { };
+}
+
+inline auto g() {
+ enum E { e };
+ return e;
+}
+
+template<class T>
+inline auto gt() {
+ enum E : T { e };
+ return e;
+}
+
+using ty1 = decltype(f());
+using ty2 = decltype(ft<int>());
+using ty3 = decltype(g());
+using ty4 = decltype(gt<int>());
diff --git a/gcc/testsuite/g++.dg/modules/merge-17_a.H
b/gcc/testsuite/g++.dg/modules/merge-17_a.H
new file mode 100644
index 00000000000..0440cd765e9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/merge-17_a.H
@@ -0,0 +1,3 @@
+// { dg-additional-options "-fmodule-header" }
+// { dg-module-cmi {} }
+#include "merge-17.h"
diff --git a/gcc/testsuite/g++.dg/modules/merge-17_b.C
b/gcc/testsuite/g++.dg/modules/merge-17_b.C
new file mode 100644
index 00000000000..4315b99f172
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/merge-17_b.C
@@ -0,0 +1,3 @@
+// { dg-additional-options "-fmodules-ts -fno-module-lazy" }
+#include "merge-17.h"
+import "merge-17_a.H";
