* [PATCH] c++/modules: local class merging [PR99426] @ 2024-02-27 2:37 Patrick Palka 2024-02-27 18:10 ` Patrick Palka 0 siblings, 1 reply; 14+ messages in thread From: Patrick Palka @ 2024-02-27 2:37 UTC (permalink / raw) To: gcc-patches; +Cc: jason, nathan, Patrick Palka 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"; -- 2.44.0.rc1.15.g4fc51f00ef ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] c++/modules: local class merging [PR99426] 2024-02-27 2:37 [PATCH] c++/modules: local class merging [PR99426] Patrick Palka @ 2024-02-27 18:10 ` Patrick Palka 2024-03-05 15:31 ` Patrick Palka 0 siblings, 1 reply; 14+ messages in thread From: Patrick Palka @ 2024-02-27 18:10 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, jason, nathan 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"; ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] c++/modules: local class merging [PR99426] 2024-02-27 18:10 ` Patrick Palka @ 2024-03-05 15:31 ` Patrick Palka 2024-03-26 14:24 ` Patrick Palka 2024-04-09 21:57 ` Jason Merrill 0 siblings, 2 replies; 14+ messages in thread From: Patrick Palka @ 2024-03-05 15:31 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, jason, nathan On Tue, 27 Feb 2024, Patrick Palka wrote: > 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): Ping. I opted to rename "local class" to "local type" throughout to capture that enums are included too. -- >8 -- Subject: [PATCH] c++/modules: local type merging [PR99426] One known missing piece in the modules implementation is merging of a streamed-in local type (class or enum) with the corresponding in-TU version of the local type. 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 types according to their position within the containing function's definition, analogous 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_type): 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 types.. (trees_out::key_local_type): Define. (trees_in::key_local_type): Define. (trees_out::get_merge_kind) <case FUNCTION_DECL>: Return MK_local_type for a local type. (trees_out::key_mergeable) <case FUNCTION_DECL>: Use key_local_type. (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/merge-17.h: New test. * g++.dg/modules/merge-17_a.H: New test. * g++.dg/modules/merge-17_b.C: New test. * g++.dg/modules/xtreme-header-7_a.H: New test. * g++.dg/modules/xtreme-header-7_b.C: New test. --- gcc/cp/module.cc | 170 +++++++++++++++--- 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 + .../g++.dg/modules/xtreme-header-7_a.H | 4 + .../g++.dg/modules/xtreme-header-7_b.C | 6 + 6 files changed, 186 insertions(+), 28 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/merge-17.h create mode 100644 gcc/testsuite/g++.dg/modules/merge-17_a.H create mode 100644 gcc/testsuite/g++.dg/modules/merge-17_b.C 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 80b63a70a62..d9e34e9a4b9 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -2770,6 +2770,7 @@ enum merge_kind MK_enum, /* Found by CTX, & 1stMemberNAME. */ MK_keyed, /* Found by key & index. */ + MK_local_type, /* Found by CTX, index. */ MK_friend_spec, /* Like named, but has a tmpl & args too. */ MK_local_friend, /* Found by CTX, index. */ @@ -2798,7 +2799,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 type", "friend spec", "local friend", NULL, /* 8...11 */ NULL, NULL, NULL, NULL, "type spec", "type tmpl spec", /* 16,17 type (template). */ @@ -2927,6 +2928,7 @@ public: unsigned binfo_mergeable (tree *); private: + tree key_local_type (const merge_key&, tree); uintptr_t *find_duplicate (tree existing); void register_duplicate (tree decl, tree existing); /* Mark as an already diagnosed bad duplicate. */ @@ -3085,6 +3087,7 @@ public: void binfo_mergeable (tree binfo); private: + void key_local_type (merge_key&, tree, tree); bool decl_node (tree, walk_kind ref); void type_node (tree); void tree_value (tree); @@ -4951,18 +4954,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); } @@ -6201,7 +6193,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); @@ -6714,7 +6720,37 @@ 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 type or enumerator, chain the + duplicate decl instead of the canonical in-TU decl. Seeing + a duplicate here means the containing function whose body + we're streaming in is a duplicate too, so we'll end up + discarding this BLOCK (and the rest of the duplicate function + body) anyway. */ + 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); @@ -10337,6 +10373,83 @@ trees_in::fn_parms_fini (int tag, tree fn, tree existing, bool is_defn) } } +/* Encode into KEY the position of the local type (class or enum) + 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_type (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 type corresponding at the position encoded by + KEY within FN. */ + +tree +trees_in::key_local_type (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. */ @@ -10456,17 +10569,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_type; break; case RECORD_TYPE: @@ -10775,6 +10881,10 @@ trees_out::key_mergeable (int tag, merge_kind mk, tree decl, tree inner, } break; + case MK_local_type: + key_local_type (key, STRIP_TEMPLATE (decl), container); + break; + case MK_enum: { /* Anonymous enums are located by their first identifier, @@ -11131,11 +11241,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_type); + existing = key_local_type (key, container); + if (existing && inner != decl) + existing = TYPE_TI_TEMPLATE (TREE_TYPE (existing)); break; case TYPE_DECL: @@ -11388,6 +11497,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/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"; 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..201d092e883 --- /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 use +// -fno-module-lazy. +// { dg-additional-options -fmodules-ts } + +#include "xtreme-header.h" +import "xtreme-header-7_a.H"; -- 2.44.0.84.gb387623c12 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] c++/modules: local class merging [PR99426] 2024-03-05 15:31 ` Patrick Palka @ 2024-03-26 14:24 ` Patrick Palka 2024-04-09 20:27 ` Patrick Palka 2024-04-09 21:57 ` Jason Merrill 1 sibling, 1 reply; 14+ messages in thread From: Patrick Palka @ 2024-03-26 14:24 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, jason, nathan On Tue, 5 Mar 2024, Patrick Palka wrote: > On Tue, 27 Feb 2024, Patrick Palka wrote: > > > 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): > > Ping. I opted to rename "local class" to "local type" throughout to > capture that enums are included too. Ping. > > -- >8 -- > > Subject: [PATCH] c++/modules: local type merging [PR99426] > > One known missing piece in the modules implementation is merging of a > streamed-in local type (class or enum) with the corresponding in-TU > version of the local type. 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 types according to their position > within the containing function's definition, analogous 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_type): 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 types.. > (trees_out::key_local_type): Define. > (trees_in::key_local_type): Define. > (trees_out::get_merge_kind) <case FUNCTION_DECL>: Return > MK_local_type for a local type. > (trees_out::key_mergeable) <case FUNCTION_DECL>: Use > key_local_type. > (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/merge-17.h: New test. > * g++.dg/modules/merge-17_a.H: New test. > * g++.dg/modules/merge-17_b.C: New test. > * g++.dg/modules/xtreme-header-7_a.H: New test. > * g++.dg/modules/xtreme-header-7_b.C: New test. > --- > gcc/cp/module.cc | 170 +++++++++++++++--- > 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 + > .../g++.dg/modules/xtreme-header-7_a.H | 4 + > .../g++.dg/modules/xtreme-header-7_b.C | 6 + > 6 files changed, 186 insertions(+), 28 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/merge-17.h > create mode 100644 gcc/testsuite/g++.dg/modules/merge-17_a.H > create mode 100644 gcc/testsuite/g++.dg/modules/merge-17_b.C > 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 80b63a70a62..d9e34e9a4b9 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -2770,6 +2770,7 @@ enum merge_kind > > MK_enum, /* Found by CTX, & 1stMemberNAME. */ > MK_keyed, /* Found by key & index. */ > + MK_local_type, /* Found by CTX, index. */ > > MK_friend_spec, /* Like named, but has a tmpl & args too. */ > MK_local_friend, /* Found by CTX, index. */ > @@ -2798,7 +2799,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 type", "friend spec", "local friend", NULL, /* 8...11 */ > NULL, NULL, NULL, NULL, > > "type spec", "type tmpl spec", /* 16,17 type (template). */ > @@ -2927,6 +2928,7 @@ public: > unsigned binfo_mergeable (tree *); > > private: > + tree key_local_type (const merge_key&, tree); > uintptr_t *find_duplicate (tree existing); > void register_duplicate (tree decl, tree existing); > /* Mark as an already diagnosed bad duplicate. */ > @@ -3085,6 +3087,7 @@ public: > void binfo_mergeable (tree binfo); > > private: > + void key_local_type (merge_key&, tree, tree); > bool decl_node (tree, walk_kind ref); > void type_node (tree); > void tree_value (tree); > @@ -4951,18 +4954,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); > } > > @@ -6201,7 +6193,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); > @@ -6714,7 +6720,37 @@ 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 type or enumerator, chain the > + duplicate decl instead of the canonical in-TU decl. Seeing > + a duplicate here means the containing function whose body > + we're streaming in is a duplicate too, so we'll end up > + discarding this BLOCK (and the rest of the duplicate function > + body) anyway. */ > + 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); > @@ -10337,6 +10373,83 @@ trees_in::fn_parms_fini (int tag, tree fn, tree existing, bool is_defn) > } > } > > +/* Encode into KEY the position of the local type (class or enum) > + 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_type (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 type corresponding at the position encoded by > + KEY within FN. */ > + > +tree > +trees_in::key_local_type (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. */ > > @@ -10456,17 +10569,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_type; > break; > > case RECORD_TYPE: > @@ -10775,6 +10881,10 @@ trees_out::key_mergeable (int tag, merge_kind mk, tree decl, tree inner, > } > break; > > + case MK_local_type: > + key_local_type (key, STRIP_TEMPLATE (decl), container); > + break; > + > case MK_enum: > { > /* Anonymous enums are located by their first identifier, > @@ -11131,11 +11241,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_type); > + existing = key_local_type (key, container); > + if (existing && inner != decl) > + existing = TYPE_TI_TEMPLATE (TREE_TYPE (existing)); > break; > > case TYPE_DECL: > @@ -11388,6 +11497,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/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"; > 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..201d092e883 > --- /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 use > +// -fno-module-lazy. > +// { dg-additional-options -fmodules-ts } > + > +#include "xtreme-header.h" > +import "xtreme-header-7_a.H"; > -- > 2.44.0.84.gb387623c12 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] c++/modules: local class merging [PR99426] 2024-03-26 14:24 ` Patrick Palka @ 2024-04-09 20:27 ` Patrick Palka 0 siblings, 0 replies; 14+ messages in thread From: Patrick Palka @ 2024-04-09 20:27 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, jason, nathan On Tue, 26 Mar 2024, Patrick Palka wrote: > On Tue, 5 Mar 2024, Patrick Palka wrote: > > > On Tue, 27 Feb 2024, Patrick Palka wrote: > > > > > 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): > > > > Ping. I opted to rename "local class" to "local type" throughout to > > capture that enums are included too. > > Ping. Ping. > > > > > -- >8 -- > > > > Subject: [PATCH] c++/modules: local type merging [PR99426] > > > > One known missing piece in the modules implementation is merging of a > > streamed-in local type (class or enum) with the corresponding in-TU > > version of the local type. 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 types according to their position > > within the containing function's definition, analogous 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_type): 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 types.. > > (trees_out::key_local_type): Define. > > (trees_in::key_local_type): Define. > > (trees_out::get_merge_kind) <case FUNCTION_DECL>: Return > > MK_local_type for a local type. > > (trees_out::key_mergeable) <case FUNCTION_DECL>: Use > > key_local_type. > > (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/merge-17.h: New test. > > * g++.dg/modules/merge-17_a.H: New test. > > * g++.dg/modules/merge-17_b.C: New test. > > * g++.dg/modules/xtreme-header-7_a.H: New test. > > * g++.dg/modules/xtreme-header-7_b.C: New test. > > --- > > gcc/cp/module.cc | 170 +++++++++++++++--- > > 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 + > > .../g++.dg/modules/xtreme-header-7_a.H | 4 + > > .../g++.dg/modules/xtreme-header-7_b.C | 6 + > > 6 files changed, 186 insertions(+), 28 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/modules/merge-17.h > > create mode 100644 gcc/testsuite/g++.dg/modules/merge-17_a.H > > create mode 100644 gcc/testsuite/g++.dg/modules/merge-17_b.C > > 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 80b63a70a62..d9e34e9a4b9 100644 > > --- a/gcc/cp/module.cc > > +++ b/gcc/cp/module.cc > > @@ -2770,6 +2770,7 @@ enum merge_kind > > > > MK_enum, /* Found by CTX, & 1stMemberNAME. */ > > MK_keyed, /* Found by key & index. */ > > + MK_local_type, /* Found by CTX, index. */ > > > > MK_friend_spec, /* Like named, but has a tmpl & args too. */ > > MK_local_friend, /* Found by CTX, index. */ > > @@ -2798,7 +2799,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 type", "friend spec", "local friend", NULL, /* 8...11 */ > > NULL, NULL, NULL, NULL, > > > > "type spec", "type tmpl spec", /* 16,17 type (template). */ > > @@ -2927,6 +2928,7 @@ public: > > unsigned binfo_mergeable (tree *); > > > > private: > > + tree key_local_type (const merge_key&, tree); > > uintptr_t *find_duplicate (tree existing); > > void register_duplicate (tree decl, tree existing); > > /* Mark as an already diagnosed bad duplicate. */ > > @@ -3085,6 +3087,7 @@ public: > > void binfo_mergeable (tree binfo); > > > > private: > > + void key_local_type (merge_key&, tree, tree); > > bool decl_node (tree, walk_kind ref); > > void type_node (tree); > > void tree_value (tree); > > @@ -4951,18 +4954,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); > > } > > > > @@ -6201,7 +6193,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); > > @@ -6714,7 +6720,37 @@ 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 type or enumerator, chain the > > + duplicate decl instead of the canonical in-TU decl. Seeing > > + a duplicate here means the containing function whose body > > + we're streaming in is a duplicate too, so we'll end up > > + discarding this BLOCK (and the rest of the duplicate function > > + body) anyway. */ > > + 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); > > @@ -10337,6 +10373,83 @@ trees_in::fn_parms_fini (int tag, tree fn, tree existing, bool is_defn) > > } > > } > > > > +/* Encode into KEY the position of the local type (class or enum) > > + 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_type (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 type corresponding at the position encoded by > > + KEY within FN. */ > > + > > +tree > > +trees_in::key_local_type (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. */ > > > > @@ -10456,17 +10569,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_type; > > break; > > > > case RECORD_TYPE: > > @@ -10775,6 +10881,10 @@ trees_out::key_mergeable (int tag, merge_kind mk, tree decl, tree inner, > > } > > break; > > > > + case MK_local_type: > > + key_local_type (key, STRIP_TEMPLATE (decl), container); > > + break; > > + > > case MK_enum: > > { > > /* Anonymous enums are located by their first identifier, > > @@ -11131,11 +11241,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_type); > > + existing = key_local_type (key, container); > > + if (existing && inner != decl) > > + existing = TYPE_TI_TEMPLATE (TREE_TYPE (existing)); > > break; > > > > case TYPE_DECL: > > @@ -11388,6 +11497,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/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"; > > 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..201d092e883 > > --- /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 use > > +// -fno-module-lazy. > > +// { dg-additional-options -fmodules-ts } > > + > > +#include "xtreme-header.h" > > +import "xtreme-header-7_a.H"; > > -- > > 2.44.0.84.gb387623c12 > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] c++/modules: local class merging [PR99426] 2024-03-05 15:31 ` Patrick Palka 2024-03-26 14:24 ` Patrick Palka @ 2024-04-09 21:57 ` Jason Merrill 2024-04-10 18:48 ` Patrick Palka 1 sibling, 1 reply; 14+ messages in thread From: Jason Merrill @ 2024-04-09 21:57 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, nathan On 3/5/24 10:31, Patrick Palka wrote: > On Tue, 27 Feb 2024, Patrick Palka wrote: > > Subject: [PATCH] c++/modules: local type merging [PR99426] > > One known missing piece in the modules implementation is merging of a > streamed-in local type (class or enum) with the corresponding in-TU > version of the local type. 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 types according to their position > within the containing function's definition, analogous 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_type): 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 types.. > (trees_out::key_local_type): Define. > (trees_in::key_local_type): Define. > (trees_out::get_merge_kind) <case FUNCTION_DECL>: Return > MK_local_type for a local type. > (trees_out::key_mergeable) <case FUNCTION_DECL>: Use > key_local_type. > (trees_in::key_mergeable) <case FUNCTION_DECL>: Likewise. > (trees_in::is_matching_decl): Be flexible with type mismatches > for local entities. > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index 80b63a70a62..d9e34e9a4b9 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -6714,7 +6720,37 @@ 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 type or enumerator, chain the > + duplicate decl instead of the canonical in-TU decl. Seeing > + a duplicate here means the containing function whose body > + we're streaming in is a duplicate too, so we'll end up > + discarding this BLOCK (and the rest of the duplicate function > + body) anyway. */ > + 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)); > + } This seems like a lot of generally-applicable code for finding the duplicate, which other calls to maybe_duplicate/odr_duplicate don't use. If the template is a duplicate, why isn't its result? If there's a good reason for that, should this template handling go into maybe_duplicate? > @@ -10337,6 +10373,83 @@ trees_in::fn_parms_fini (int tag, tree fn, tree existing, bool is_defn) > } > } > > +/* Encode into KEY the position of the local type (class or enum) > + 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. */ Since we already set DECL_DISCRIMINATOR for mangling, could we use it+name for the key as well? Jason ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] c++/modules: local class merging [PR99426] 2024-04-09 21:57 ` Jason Merrill @ 2024-04-10 18:48 ` Patrick Palka 2024-04-10 22:55 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Patrick Palka @ 2024-04-10 18:48 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches, nathan On Tue, 9 Apr 2024, Jason Merrill wrote: > On 3/5/24 10:31, Patrick Palka wrote: > > On Tue, 27 Feb 2024, Patrick Palka wrote: > > > > Subject: [PATCH] c++/modules: local type merging [PR99426] > > > > One known missing piece in the modules implementation is merging of a > > streamed-in local type (class or enum) with the corresponding in-TU > > version of the local type. 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 types according to their position > > within the containing function's definition, analogous 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_type): 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 types.. > > (trees_out::key_local_type): Define. > > (trees_in::key_local_type): Define. > > (trees_out::get_merge_kind) <case FUNCTION_DECL>: Return > > MK_local_type for a local type. > > (trees_out::key_mergeable) <case FUNCTION_DECL>: Use > > key_local_type. > > (trees_in::key_mergeable) <case FUNCTION_DECL>: Likewise. > > (trees_in::is_matching_decl): Be flexible with type mismatches > > for local entities. > > > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > > index 80b63a70a62..d9e34e9a4b9 100644 > > --- a/gcc/cp/module.cc > > +++ b/gcc/cp/module.cc > > @@ -6714,7 +6720,37 @@ 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 type or enumerator, chain the > > + duplicate decl instead of the canonical in-TU decl. Seeing > > + a duplicate here means the containing function whose body > > + we're streaming in is a duplicate too, so we'll end up > > + discarding this BLOCK (and the rest of the duplicate function > > + body) anyway. */ > > + 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)); > > + } > > This seems like a lot of generally-applicable code for finding the duplicate, > which other calls to maybe_duplicate/odr_duplicate don't use. If the template > is a duplicate, why isn't its result? If there's a good reason for that, > should this template handling go into maybe_duplicate? Ah yeah, that makes sense. Some context: IIUC modules treats the TEMPLATE_DECL instead of the DECL_TEMPLATE_RESULT as the canonical decl, which in turn means we'll register_duplicate only the TEMPLATE_DECL. But BLOCK_VARS never contains a TEMPLATE_DECL, always the DECL_TEMPLATE_RESULT (i.e. a TYPE_DECL), hence the extra handling. Given that it's relatively more difficult to get at the TEMPLATE_DECL from the DECL_TEMPLATE_RESULT rather than vice versa, maybe we should just register both as duplicates from register_duplicate? That way callers can just simply pass the DECL_TEMPLATE_RESULT to maybe_duplicate and it'll do the right thing. > > > @@ -10337,6 +10373,83 @@ trees_in::fn_parms_fini (int tag, tree fn, tree > > existing, bool is_defn) > > } > > } > > +/* Encode into KEY the position of the local type (class or enum) > > + 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. */ > > Since we already set DECL_DISCRIMINATOR for mangling, could we use it+name for > the key as well? We could (and IIUc that'd be more robust to ODR violations), but wouldn't it mean we'd have to do a linear walk over all BLOCK_VARs of all BLOCKS in order to find the one with the matching name+discriminator? That'd be slower than the current approach which lets us skip to the correct BLOCK and walk only its BLOCK_VARS. Here's a tested patch that implements the register_duplicate idea to simplify the added call to maybe_duplicate: -- >8 -- Subject: [PATCH] c++/modules: local type merging [PR99426] PR c++/99426 gcc/cp/ChangeLog: * module.cc (merge_kind::MK_local_type): 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 types.. (trees_out::key_local_type): Define. (trees_in::key_local_type): Define. (trees_out::get_merge_kind) <case FUNCTION_DECL>: Return MK_local_type for a local type. (trees_out::key_mergeable) <case FUNCTION_DECL>: Use key_local_type. (trees_in::key_mergeable) <case FUNCTION_DECL>: Likewise. (trees_in::is_matching_decl): Be flexible with type mismatches for local entities. (trees_in::register_duplicate): Also register the DECL_TEMPLATE_RESULT of a TEMPLATE_DECL as a duplicate. gcc/testsuite/ChangeLog: * g++.dg/modules/merge-17.h: New test. * g++.dg/modules/merge-17_a.H: New test. * g++.dg/modules/merge-17_b.C: New test. * g++.dg/modules/xtreme-header-7_a.H: New test. * g++.dg/modules/xtreme-header-7_b.C: New test. --- gcc/cp/module.cc | 168 +++++++++++++++--- 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 + .../g++.dg/modules/xtreme-header-7_a.H | 4 + .../g++.dg/modules/xtreme-header-7_b.C | 5 + 6 files changed, 183 insertions(+), 28 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/merge-17.h create mode 100644 gcc/testsuite/g++.dg/modules/merge-17_a.H create mode 100644 gcc/testsuite/g++.dg/modules/merge-17_b.C 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 ef0280df00a..707142531dc 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -2772,6 +2772,7 @@ enum merge_kind MK_enum, /* Found by CTX, & 1stMemberNAME. */ MK_keyed, /* Found by key & index. */ + MK_local_type, /* Found by CTX, index. */ MK_friend_spec, /* Like named, but has a tmpl & args too. */ MK_local_friend, /* Found by CTX, index. */ @@ -2798,7 +2799,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 type", "friend spec", "local friend", NULL, /* 8...11 */ NULL, NULL, NULL, NULL, "type spec", "type tmpl spec", /* 16,17 type (template). */ @@ -2932,6 +2933,7 @@ public: unsigned binfo_mergeable (tree *); private: + tree key_local_type (const merge_key&, tree); uintptr_t *find_duplicate (tree existing); void register_duplicate (tree decl, tree existing); /* Mark as an already diagnosed bad duplicate. */ @@ -3092,6 +3094,7 @@ public: void binfo_mergeable (tree binfo); private: + void key_local_type (merge_key&, tree, tree); bool decl_node (tree, walk_kind ref); void type_node (tree); void tree_value (tree); @@ -4959,18 +4962,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); } @@ -6244,7 +6236,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); @@ -6757,7 +6763,29 @@ 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 type or enumerator, chain the + duplicate decl instead of the canonical in-TU decl. Seeing + a duplicate here means the containing function whose body + we're streaming in is a duplicate too, so we'll end up + discarding this BLOCK (and the rest of the duplicate function + body) anyway. */ + decl = maybe_duplicate (decl); + + 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); @@ -10373,6 +10401,83 @@ trees_in::fn_parms_fini (int tag, tree fn, tree existing, bool is_defn) } } +/* Encode into KEY the position of the local type (class or enum) + 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_type (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 type corresponding at the position encoded by + KEY within FN. */ + +tree +trees_in::key_local_type (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. */ @@ -10492,17 +10597,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_type; break; case RECORD_TYPE: @@ -10804,6 +10902,10 @@ trees_out::key_mergeable (int tag, merge_kind mk, tree decl, tree inner, } break; + case MK_local_type: + key_local_type (key, STRIP_TEMPLATE (decl), container); + break; + case MK_enum: { /* Anonymous enums are located by their first identifier, @@ -11160,11 +11262,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_type); + existing = key_local_type (key, container); + if (existing && inner != decl) + existing = TYPE_TI_TEMPLATE (TREE_TYPE (existing)); break; case TYPE_DECL: @@ -11417,6 +11518,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, @@ -11666,6 +11772,12 @@ trees_in::register_duplicate (tree decl, tree existing) uintptr_t &slot = duplicates->get_or_insert (existing, &existed); gcc_checking_assert (!existed); slot = reinterpret_cast<uintptr_t> (decl); + if (TREE_CODE (decl) == TEMPLATE_DECL) + /* Also register the DECL_TEMPLATE_RESULT as a duplicate so + that passing the _RESULT to maybe_duplicate gives us the + existing _RESULT back. */ + register_duplicate (DECL_TEMPLATE_RESULT (decl), + DECL_TEMPLATE_RESULT (existing)); } /* We've read a definition of MAYBE_EXISTING. If not a duplicate, 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..a5269959702 --- /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> +auto ft() { + decltype(+T()) x; + return [&x] { }; +} + +inline auto g() { + enum E { e }; + return e; +} + +template<class T> +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"; 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..3992a24501b --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/xtreme-header-7_b.C @@ -0,0 +1,5 @@ +// A version of xtreme-header_b.C that doesn't use -fno-module-lazy. +// { dg-additional-options -fmodules-ts } + +#include "xtreme-header.h" +import "xtreme-header-7_a.H"; -- 2.44.0.548.g91ec36f2cc ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] c++/modules: local class merging [PR99426] 2024-04-10 18:48 ` Patrick Palka @ 2024-04-10 22:55 ` Jason Merrill 2024-04-12 14:35 ` Patrick Palka 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2024-04-10 22:55 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, nathan On 4/10/24 14:48, Patrick Palka wrote: > On Tue, 9 Apr 2024, Jason Merrill wrote: > >> On 3/5/24 10:31, Patrick Palka wrote: >>> On Tue, 27 Feb 2024, Patrick Palka wrote: >>> >>> Subject: [PATCH] c++/modules: local type merging [PR99426] >>> >>> One known missing piece in the modules implementation is merging of a >>> streamed-in local type (class or enum) with the corresponding in-TU >>> version of the local type. 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 types according to their position >>> within the containing function's definition, analogous 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_type): 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 types.. >>> (trees_out::key_local_type): Define. >>> (trees_in::key_local_type): Define. >>> (trees_out::get_merge_kind) <case FUNCTION_DECL>: Return >>> MK_local_type for a local type. >>> (trees_out::key_mergeable) <case FUNCTION_DECL>: Use >>> key_local_type. >>> (trees_in::key_mergeable) <case FUNCTION_DECL>: Likewise. >>> (trees_in::is_matching_decl): Be flexible with type mismatches >>> for local entities. >>> >>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc >>> index 80b63a70a62..d9e34e9a4b9 100644 >>> --- a/gcc/cp/module.cc >>> +++ b/gcc/cp/module.cc >>> @@ -6714,7 +6720,37 @@ 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 type or enumerator, chain the >>> + duplicate decl instead of the canonical in-TU decl. Seeing >>> + a duplicate here means the containing function whose body >>> + we're streaming in is a duplicate too, so we'll end up >>> + discarding this BLOCK (and the rest of the duplicate function >>> + body) anyway. */ >>> + 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)); >>> + } >> >> This seems like a lot of generally-applicable code for finding the duplicate, >> which other calls to maybe_duplicate/odr_duplicate don't use. If the template >> is a duplicate, why isn't its result? If there's a good reason for that, >> should this template handling go into maybe_duplicate? > > Ah yeah, that makes sense. > > Some context: IIUC modules treats the TEMPLATE_DECL instead of the > DECL_TEMPLATE_RESULT as the canonical decl, which in turn means we'll > register_duplicate only the TEMPLATE_DECL. But BLOCK_VARS never contains > a TEMPLATE_DECL, always the DECL_TEMPLATE_RESULT (i.e. a TYPE_DECL), > hence the extra handling. > > Given that it's relatively more difficult to get at the TEMPLATE_DECL > from the DECL_TEMPLATE_RESULT rather than vice versa, maybe we should > just register both as duplicates from register_duplicate? That way > callers can just simply pass the DECL_TEMPLATE_RESULT to maybe_duplicate > and it'll do the right thing. Sounds good. >>> @@ -10337,6 +10373,83 @@ trees_in::fn_parms_fini (int tag, tree fn, tree >>> existing, bool is_defn) >>> } >>> } >>> +/* Encode into KEY the position of the local type (class or enum) >>> + 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. */ >> >> Since we already set DECL_DISCRIMINATOR for mangling, could we use it+name for >> the key as well? > > We could (and IIUc that'd be more robust to ODR violations), but > wouldn't it mean we'd have to do a linear walk over all BLOCK_VARs of > all BLOCKS in order to find the one with the matching > name+discriminator? That'd be slower than the current approach which > lets us skip to the correct BLOCK and walk only its BLOCK_VARS. Ah, good point. How about block number + name instead of the index? Jason ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] c++/modules: local class merging [PR99426] 2024-04-10 22:55 ` Jason Merrill @ 2024-04-12 14:35 ` Patrick Palka 2024-04-12 17:18 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Patrick Palka @ 2024-04-12 14:35 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches, nathan On Wed, 10 Apr 2024, Jason Merrill wrote: > On 4/10/24 14:48, Patrick Palka wrote: > > On Tue, 9 Apr 2024, Jason Merrill wrote: > > > > > On 3/5/24 10:31, Patrick Palka wrote: > > > > On Tue, 27 Feb 2024, Patrick Palka wrote: > > > > > > > > Subject: [PATCH] c++/modules: local type merging [PR99426] > > > > > > > > One known missing piece in the modules implementation is merging of a > > > > streamed-in local type (class or enum) with the corresponding in-TU > > > > version of the local type. 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 types according to their position > > > > within the containing function's definition, analogous 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_type): 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 types.. > > > > (trees_out::key_local_type): Define. > > > > (trees_in::key_local_type): Define. > > > > (trees_out::get_merge_kind) <case FUNCTION_DECL>: Return > > > > MK_local_type for a local type. > > > > (trees_out::key_mergeable) <case FUNCTION_DECL>: Use > > > > key_local_type. > > > > (trees_in::key_mergeable) <case FUNCTION_DECL>: Likewise. > > > > (trees_in::is_matching_decl): Be flexible with type mismatches > > > > for local entities. > > > > > > > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > > > > index 80b63a70a62..d9e34e9a4b9 100644 > > > > --- a/gcc/cp/module.cc > > > > +++ b/gcc/cp/module.cc > > > > @@ -6714,7 +6720,37 @@ 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 type or enumerator, chain the > > > > + duplicate decl instead of the canonical in-TU decl. Seeing > > > > + a duplicate here means the containing function whose body > > > > + we're streaming in is a duplicate too, so we'll end up > > > > + discarding this BLOCK (and the rest of the duplicate function > > > > + body) anyway. */ > > > > + 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)); > > > > + } > > > > > > This seems like a lot of generally-applicable code for finding the > > > duplicate, > > > which other calls to maybe_duplicate/odr_duplicate don't use. If the > > > template > > > is a duplicate, why isn't its result? If there's a good reason for that, > > > should this template handling go into maybe_duplicate? > > > > Ah yeah, that makes sense. > > > > Some context: IIUC modules treats the TEMPLATE_DECL instead of the > > DECL_TEMPLATE_RESULT as the canonical decl, which in turn means we'll > > register_duplicate only the TEMPLATE_DECL. But BLOCK_VARS never contains > > a TEMPLATE_DECL, always the DECL_TEMPLATE_RESULT (i.e. a TYPE_DECL), > > hence the extra handling. > > > > Given that it's relatively more difficult to get at the TEMPLATE_DECL > > from the DECL_TEMPLATE_RESULT rather than vice versa, maybe we should > > just register both as duplicates from register_duplicate? That way > > callers can just simply pass the DECL_TEMPLATE_RESULT to maybe_duplicate > > and it'll do the right thing. > > Sounds good. > > > > > @@ -10337,6 +10373,83 @@ trees_in::fn_parms_fini (int tag, tree fn, tree > > > > existing, bool is_defn) > > > > } > > > > } > > > > +/* Encode into KEY the position of the local type (class or enum) > > > > + 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. */ > > > > > > Since we already set DECL_DISCRIMINATOR for mangling, could we use it+name > > > for > > > the key as well? > > > > We could (and IIUc that'd be more robust to ODR violations), but > > wouldn't it mean we'd have to do a linear walk over all BLOCK_VARs of > > all BLOCKS in order to find the one with the matching > > name+discriminator? That'd be slower than the current approach which > > lets us skip to the correct BLOCK and walk only its BLOCK_VARS. > > Ah, good point. How about block number + name instead of the index? It seems DECL_DISCRIMINATOR is only set at instantiation time and so for local types from a function template pattern the field is empty, which means we can't use it as the key in general :/ > > Jason > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] c++/modules: local class merging [PR99426] 2024-04-12 14:35 ` Patrick Palka @ 2024-04-12 17:18 ` Jason Merrill 2024-04-12 17:48 ` Patrick Palka 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2024-04-12 17:18 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, nathan On 4/12/24 10:35, Patrick Palka wrote: > On Wed, 10 Apr 2024, Jason Merrill wrote: > >> On 4/10/24 14:48, Patrick Palka wrote: >>> On Tue, 9 Apr 2024, Jason Merrill wrote: >>> >>>> On 3/5/24 10:31, Patrick Palka wrote: >>>>> On Tue, 27 Feb 2024, Patrick Palka wrote: >>>>> >>>>> Subject: [PATCH] c++/modules: local type merging [PR99426] >>>>> >>>>> One known missing piece in the modules implementation is merging of a >>>>> streamed-in local type (class or enum) with the corresponding in-TU >>>>> version of the local type. 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 types according to their position >>>>> within the containing function's definition, analogous 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_type): 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 types.. >>>>> (trees_out::key_local_type): Define. >>>>> (trees_in::key_local_type): Define. >>>>> (trees_out::get_merge_kind) <case FUNCTION_DECL>: Return >>>>> MK_local_type for a local type. >>>>> (trees_out::key_mergeable) <case FUNCTION_DECL>: Use >>>>> key_local_type. >>>>> (trees_in::key_mergeable) <case FUNCTION_DECL>: Likewise. >>>>> (trees_in::is_matching_decl): Be flexible with type mismatches >>>>> for local entities. >>>>> >>>>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc >>>>> index 80b63a70a62..d9e34e9a4b9 100644 >>>>> --- a/gcc/cp/module.cc >>>>> +++ b/gcc/cp/module.cc >>>>> @@ -6714,7 +6720,37 @@ 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 type or enumerator, chain the >>>>> + duplicate decl instead of the canonical in-TU decl. Seeing >>>>> + a duplicate here means the containing function whose body >>>>> + we're streaming in is a duplicate too, so we'll end up >>>>> + discarding this BLOCK (and the rest of the duplicate function >>>>> + body) anyway. */ >>>>> + 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)); >>>>> + } >>>> >>>> This seems like a lot of generally-applicable code for finding the >>>> duplicate, >>>> which other calls to maybe_duplicate/odr_duplicate don't use. If the >>>> template >>>> is a duplicate, why isn't its result? If there's a good reason for that, >>>> should this template handling go into maybe_duplicate? >>> >>> Ah yeah, that makes sense. >>> >>> Some context: IIUC modules treats the TEMPLATE_DECL instead of the >>> DECL_TEMPLATE_RESULT as the canonical decl, which in turn means we'll >>> register_duplicate only the TEMPLATE_DECL. But BLOCK_VARS never contains >>> a TEMPLATE_DECL, always the DECL_TEMPLATE_RESULT (i.e. a TYPE_DECL), >>> hence the extra handling. >>> >>> Given that it's relatively more difficult to get at the TEMPLATE_DECL >>> from the DECL_TEMPLATE_RESULT rather than vice versa, maybe we should >>> just register both as duplicates from register_duplicate? That way >>> callers can just simply pass the DECL_TEMPLATE_RESULT to maybe_duplicate >>> and it'll do the right thing. >> >> Sounds good. >> >>>>> @@ -10337,6 +10373,83 @@ trees_in::fn_parms_fini (int tag, tree fn, tree >>>>> existing, bool is_defn) >>>>> } >>>>> } >>>>> +/* Encode into KEY the position of the local type (class or enum) >>>>> + 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. */ >>>> >>>> Since we already set DECL_DISCRIMINATOR for mangling, could we use it+name >>>> for >>>> the key as well? >>> >>> We could (and IIUc that'd be more robust to ODR violations), but >>> wouldn't it mean we'd have to do a linear walk over all BLOCK_VARs of >>> all BLOCKS in order to find the one with the matching >>> name+discriminator? That'd be slower than the current approach which >>> lets us skip to the correct BLOCK and walk only its BLOCK_VARS. >> >> Ah, good point. How about block number + name instead of the index? > > It seems DECL_DISCRIMINATOR is only set at instantiation time and so for > local types from a function template pattern the field is empty, which > means we can't use it as the key in general :/ I meant just block number and name, without DECL_DISCRIMINATOR. Just using the name instead of an index in BLOCK_VARS. Jason ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] c++/modules: local class merging [PR99426] 2024-04-12 17:18 ` Jason Merrill @ 2024-04-12 17:48 ` Patrick Palka 2024-04-12 18:07 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Patrick Palka @ 2024-04-12 17:48 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches, nathan On Fri, 12 Apr 2024, Jason Merrill wrote: > On 4/12/24 10:35, Patrick Palka wrote: > > On Wed, 10 Apr 2024, Jason Merrill wrote: > > > > > On 4/10/24 14:48, Patrick Palka wrote: > > > > On Tue, 9 Apr 2024, Jason Merrill wrote: > > > > > > > > > On 3/5/24 10:31, Patrick Palka wrote: > > > > > > On Tue, 27 Feb 2024, Patrick Palka wrote: > > > > > > > > > > > > Subject: [PATCH] c++/modules: local type merging [PR99426] > > > > > > > > > > > > One known missing piece in the modules implementation is merging of > > > > > > a > > > > > > streamed-in local type (class or enum) with the corresponding in-TU > > > > > > version of the local type. 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 types according to their > > > > > > position > > > > > > within the containing function's definition, analogous 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_type): 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 types.. > > > > > > (trees_out::key_local_type): Define. > > > > > > (trees_in::key_local_type): Define. > > > > > > (trees_out::get_merge_kind) <case FUNCTION_DECL>: Return > > > > > > MK_local_type for a local type. > > > > > > (trees_out::key_mergeable) <case FUNCTION_DECL>: Use > > > > > > key_local_type. > > > > > > (trees_in::key_mergeable) <case FUNCTION_DECL>: Likewise. > > > > > > (trees_in::is_matching_decl): Be flexible with type mismatches > > > > > > for local entities. > > > > > > > > > > > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > > > > > > index 80b63a70a62..d9e34e9a4b9 100644 > > > > > > --- a/gcc/cp/module.cc > > > > > > +++ b/gcc/cp/module.cc > > > > > > @@ -6714,7 +6720,37 @@ 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 type or enumerator, chain the > > > > > > + duplicate decl instead of the canonical in-TU decl. > > > > > > Seeing > > > > > > + a duplicate here means the containing function whose > > > > > > body > > > > > > + we're streaming in is a duplicate too, so we'll end up > > > > > > + discarding this BLOCK (and the rest of the duplicate > > > > > > function > > > > > > + body) anyway. */ > > > > > > + 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)); > > > > > > + } > > > > > > > > > > This seems like a lot of generally-applicable code for finding the > > > > > duplicate, > > > > > which other calls to maybe_duplicate/odr_duplicate don't use. If the > > > > > template > > > > > is a duplicate, why isn't its result? If there's a good reason for > > > > > that, > > > > > should this template handling go into maybe_duplicate? > > > > > > > > Ah yeah, that makes sense. > > > > > > > > Some context: IIUC modules treats the TEMPLATE_DECL instead of the > > > > DECL_TEMPLATE_RESULT as the canonical decl, which in turn means we'll > > > > register_duplicate only the TEMPLATE_DECL. But BLOCK_VARS never > > > > contains > > > > a TEMPLATE_DECL, always the DECL_TEMPLATE_RESULT (i.e. a TYPE_DECL), > > > > hence the extra handling. > > > > > > > > Given that it's relatively more difficult to get at the TEMPLATE_DECL > > > > from the DECL_TEMPLATE_RESULT rather than vice versa, maybe we should > > > > just register both as duplicates from register_duplicate? That way > > > > callers can just simply pass the DECL_TEMPLATE_RESULT to maybe_duplicate > > > > and it'll do the right thing. > > > > > > Sounds good. > > > > > > > > > @@ -10337,6 +10373,83 @@ trees_in::fn_parms_fini (int tag, tree fn, > > > > > > tree > > > > > > existing, bool is_defn) > > > > > > } > > > > > > } > > > > > > +/* Encode into KEY the position of the local type (class or > > > > > > enum) > > > > > > + 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. */ > > > > > > > > > > Since we already set DECL_DISCRIMINATOR for mangling, could we use > > > > > it+name > > > > > for > > > > > the key as well? > > > > > > > > We could (and IIUc that'd be more robust to ODR violations), but > > > > wouldn't it mean we'd have to do a linear walk over all BLOCK_VARs of > > > > all BLOCKS in order to find the one with the matching > > > > name+discriminator? That'd be slower than the current approach which > > > > lets us skip to the correct BLOCK and walk only its BLOCK_VARS. > > > > > > Ah, good point. How about block number + name instead of the index? > > > > It seems DECL_DISCRIMINATOR is only set at instantiation time and so for > > local types from a function template pattern the field is empty, which > > means we can't use it as the key in general :/ > > I meant just block number and name, without DECL_DISCRIMINATOR. Just using > the name instead of an index in BLOCK_VARS. Ah, I think that'd be enough for named local types, but what about anonymous local types? IIUC without DECL_DISCRIMINATOR we wouldn't be able to reliably distinguisth between multiple anonymous local types defined in the same block, since their identifiers aren't stable given that they're based off of a global counter (and so sensitive to #include order) :( > > Jason > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] c++/modules: local class merging [PR99426] 2024-04-12 17:48 ` Patrick Palka @ 2024-04-12 18:07 ` Jason Merrill 2024-04-12 18:39 ` Patrick Palka 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2024-04-12 18:07 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, nathan On 4/12/24 13:48, Patrick Palka wrote: > On Fri, 12 Apr 2024, Jason Merrill wrote: > >> On 4/12/24 10:35, Patrick Palka wrote: >>> On Wed, 10 Apr 2024, Jason Merrill wrote: >>> >>>> On 4/10/24 14:48, Patrick Palka wrote: >>>>> On Tue, 9 Apr 2024, Jason Merrill wrote: >>>>> >>>>>> On 3/5/24 10:31, Patrick Palka wrote: >>>>>>> On Tue, 27 Feb 2024, Patrick Palka wrote: >>>>>>> >>>>>>> Subject: [PATCH] c++/modules: local type merging [PR99426] >>>>>>> >>>>>>> One known missing piece in the modules implementation is merging of >>>>>>> a >>>>>>> streamed-in local type (class or enum) with the corresponding in-TU >>>>>>> version of the local type. 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 types according to their >>>>>>> position >>>>>>> within the containing function's definition, analogous 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_type): 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 types.. >>>>>>> (trees_out::key_local_type): Define. >>>>>>> (trees_in::key_local_type): Define. >>>>>>> (trees_out::get_merge_kind) <case FUNCTION_DECL>: Return >>>>>>> MK_local_type for a local type. >>>>>>> (trees_out::key_mergeable) <case FUNCTION_DECL>: Use >>>>>>> key_local_type. >>>>>>> (trees_in::key_mergeable) <case FUNCTION_DECL>: Likewise. >>>>>>> (trees_in::is_matching_decl): Be flexible with type mismatches >>>>>>> for local entities. >>>>>>> >>>>>>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc >>>>>>> index 80b63a70a62..d9e34e9a4b9 100644 >>>>>>> --- a/gcc/cp/module.cc >>>>>>> +++ b/gcc/cp/module.cc >>>>>>> @@ -6714,7 +6720,37 @@ 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 type or enumerator, chain the >>>>>>> + duplicate decl instead of the canonical in-TU decl. >>>>>>> Seeing >>>>>>> + a duplicate here means the containing function whose >>>>>>> body >>>>>>> + we're streaming in is a duplicate too, so we'll end up >>>>>>> + discarding this BLOCK (and the rest of the duplicate >>>>>>> function >>>>>>> + body) anyway. */ >>>>>>> + 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)); >>>>>>> + } >>>>>> >>>>>> This seems like a lot of generally-applicable code for finding the >>>>>> duplicate, >>>>>> which other calls to maybe_duplicate/odr_duplicate don't use. If the >>>>>> template >>>>>> is a duplicate, why isn't its result? If there's a good reason for >>>>>> that, >>>>>> should this template handling go into maybe_duplicate? >>>>> >>>>> Ah yeah, that makes sense. >>>>> >>>>> Some context: IIUC modules treats the TEMPLATE_DECL instead of the >>>>> DECL_TEMPLATE_RESULT as the canonical decl, which in turn means we'll >>>>> register_duplicate only the TEMPLATE_DECL. But BLOCK_VARS never >>>>> contains >>>>> a TEMPLATE_DECL, always the DECL_TEMPLATE_RESULT (i.e. a TYPE_DECL), >>>>> hence the extra handling. >>>>> >>>>> Given that it's relatively more difficult to get at the TEMPLATE_DECL >>>>> from the DECL_TEMPLATE_RESULT rather than vice versa, maybe we should >>>>> just register both as duplicates from register_duplicate? That way >>>>> callers can just simply pass the DECL_TEMPLATE_RESULT to maybe_duplicate >>>>> and it'll do the right thing. >>>> >>>> Sounds good. >>>> >>>>>>> @@ -10337,6 +10373,83 @@ trees_in::fn_parms_fini (int tag, tree fn, >>>>>>> tree >>>>>>> existing, bool is_defn) >>>>>>> } >>>>>>> } >>>>>>> +/* Encode into KEY the position of the local type (class or >>>>>>> enum) >>>>>>> + 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. */ >>>>>> >>>>>> Since we already set DECL_DISCRIMINATOR for mangling, could we use >>>>>> it+name >>>>>> for >>>>>> the key as well? >>>>> >>>>> We could (and IIUc that'd be more robust to ODR violations), but >>>>> wouldn't it mean we'd have to do a linear walk over all BLOCK_VARs of >>>>> all BLOCKS in order to find the one with the matching >>>>> name+discriminator? That'd be slower than the current approach which >>>>> lets us skip to the correct BLOCK and walk only its BLOCK_VARS. >>>> >>>> Ah, good point. How about block number + name instead of the index? >>> >>> It seems DECL_DISCRIMINATOR is only set at instantiation time and so for >>> local types from a function template pattern the field is empty, which >>> means we can't use it as the key in general :/ >> >> I meant just block number and name, without DECL_DISCRIMINATOR. Just using >> the name instead of an index in BLOCK_VARS. > > Ah, I think that'd be enough for named local types, but what about > anonymous local types? IIUC without DECL_DISCRIMINATOR we wouldn't be > able to reliably distinguisth between multiple anonymous local types > defined in the same block, since their identifiers aren't stable given > that they're based off of a global counter (and so sensitive to #include > order) :( Good point. But I'd still think to merge based on name if we have one; as you said above, to be more robust to ODR violations. If the imported fn has a local class that the included header didn't, would we get the same problem? Jason ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] c++/modules: local class merging [PR99426] 2024-04-12 18:07 ` Jason Merrill @ 2024-04-12 18:39 ` Patrick Palka 2024-04-12 19:08 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Patrick Palka @ 2024-04-12 18:39 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches, nathan On Fri, 12 Apr 2024, Jason Merrill wrote: > On 4/12/24 13:48, Patrick Palka wrote: > > On Fri, 12 Apr 2024, Jason Merrill wrote: > > > > > On 4/12/24 10:35, Patrick Palka wrote: > > > > On Wed, 10 Apr 2024, Jason Merrill wrote: > > > > > > > > > On 4/10/24 14:48, Patrick Palka wrote: > > > > > > On Tue, 9 Apr 2024, Jason Merrill wrote: > > > > > > > > > > > > > On 3/5/24 10:31, Patrick Palka wrote: > > > > > > > > On Tue, 27 Feb 2024, Patrick Palka wrote: > > > > > > > > > > > > > > > > Subject: [PATCH] c++/modules: local type merging [PR99426] > > > > > > > > > > > > > > > > One known missing piece in the modules implementation is merging > > > > > > > > of > > > > > > > > a > > > > > > > > streamed-in local type (class or enum) with the corresponding > > > > > > > > in-TU > > > > > > > > version of the local type. 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 types according to their > > > > > > > > position > > > > > > > > within the containing function's definition, analogous 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_type): 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 types.. > > > > > > > > (trees_out::key_local_type): Define. > > > > > > > > (trees_in::key_local_type): Define. > > > > > > > > (trees_out::get_merge_kind) <case FUNCTION_DECL>: Return > > > > > > > > MK_local_type for a local type. > > > > > > > > (trees_out::key_mergeable) <case FUNCTION_DECL>: Use > > > > > > > > key_local_type. > > > > > > > > (trees_in::key_mergeable) <case FUNCTION_DECL>: Likewise. > > > > > > > > (trees_in::is_matching_decl): Be flexible with type mismatches > > > > > > > > for local entities. > > > > > > > > > > > > > > > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > > > > > > > > index 80b63a70a62..d9e34e9a4b9 100644 > > > > > > > > --- a/gcc/cp/module.cc > > > > > > > > +++ b/gcc/cp/module.cc > > > > > > > > @@ -6714,7 +6720,37 @@ 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 type or enumerator, chain the > > > > > > > > + duplicate decl instead of the canonical in-TU decl. > > > > > > > > Seeing > > > > > > > > + a duplicate here means the containing function whose > > > > > > > > body > > > > > > > > + we're streaming in is a duplicate too, so we'll end up > > > > > > > > + discarding this BLOCK (and the rest of the duplicate > > > > > > > > function > > > > > > > > + body) anyway. */ > > > > > > > > + 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)); > > > > > > > > + } > > > > > > > > > > > > > > This seems like a lot of generally-applicable code for finding the > > > > > > > duplicate, > > > > > > > which other calls to maybe_duplicate/odr_duplicate don't use. If > > > > > > > the > > > > > > > template > > > > > > > is a duplicate, why isn't its result? If there's a good reason > > > > > > > for > > > > > > > that, > > > > > > > should this template handling go into maybe_duplicate? > > > > > > > > > > > > Ah yeah, that makes sense. > > > > > > > > > > > > Some context: IIUC modules treats the TEMPLATE_DECL instead of the > > > > > > DECL_TEMPLATE_RESULT as the canonical decl, which in turn means > > > > > > we'll > > > > > > register_duplicate only the TEMPLATE_DECL. But BLOCK_VARS never > > > > > > contains > > > > > > a TEMPLATE_DECL, always the DECL_TEMPLATE_RESULT (i.e. a TYPE_DECL), > > > > > > hence the extra handling. > > > > > > > > > > > > Given that it's relatively more difficult to get at the > > > > > > TEMPLATE_DECL > > > > > > from the DECL_TEMPLATE_RESULT rather than vice versa, maybe we > > > > > > should > > > > > > just register both as duplicates from register_duplicate? That way > > > > > > callers can just simply pass the DECL_TEMPLATE_RESULT to > > > > > > maybe_duplicate > > > > > > and it'll do the right thing. > > > > > > > > > > Sounds good. > > > > > > > > > > > > > @@ -10337,6 +10373,83 @@ trees_in::fn_parms_fini (int tag, tree > > > > > > > > fn, > > > > > > > > tree > > > > > > > > existing, bool is_defn) > > > > > > > > } > > > > > > > > } > > > > > > > > +/* Encode into KEY the position of the local type (class > > > > > > > > or > > > > > > > > enum) > > > > > > > > + 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. */ > > > > > > > > > > > > > > Since we already set DECL_DISCRIMINATOR for mangling, could we use > > > > > > > it+name > > > > > > > for > > > > > > > the key as well? > > > > > > > > > > > > We could (and IIUc that'd be more robust to ODR violations), but > > > > > > wouldn't it mean we'd have to do a linear walk over all BLOCK_VARs > > > > > > of > > > > > > all BLOCKS in order to find the one with the matching > > > > > > name+discriminator? That'd be slower than the current approach > > > > > > which > > > > > > lets us skip to the correct BLOCK and walk only its BLOCK_VARS. > > > > > > > > > > Ah, good point. How about block number + name instead of the index? > > > > > > > > It seems DECL_DISCRIMINATOR is only set at instantiation time and so for > > > > local types from a function template pattern the field is empty, which > > > > means we can't use it as the key in general :/ > > > > > > I meant just block number and name, without DECL_DISCRIMINATOR. Just > > > using > > > the name instead of an index in BLOCK_VARS. > > > > Ah, I think that'd be enough for named local types, but what about > > anonymous local types? IIUC without DECL_DISCRIMINATOR we wouldn't be > > able to reliably distinguisth between multiple anonymous local types > > defined in the same block, since their identifiers aren't stable given > > that they're based off of a global counter (and so sensitive to #include > > order) :( > > Good point. But I'd still think to merge based on name if we have one; as you > said above, to be more robust to ODR violations. Sounds good. > > If the imported fn has a local class that the included header didn't, would we > get the same problem? IIUC yes, all the intra-block indexes would be off by one in that case and deduplicatation would fail or we'd deduplicate distinct types. Here's an incremental diff for the updated patch. The augmented testcase triggered a latent qsort checking failure in depset_cmp that was straightforwardly fixed: diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 707142531dc..ee0e5da1d37 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -2933,7 +2933,7 @@ public: unsigned binfo_mergeable (tree *); private: - tree key_local_type (const merge_key&, tree); + tree key_local_type (const merge_key&, tree, tree); uintptr_t *find_duplicate (tree existing); void register_duplicate (tree decl, tree existing); /* Mark as an already diagnosed bad duplicate. */ @@ -10437,10 +10437,10 @@ trees_out::key_local_type (merge_key& key, tree decl, tree fn) } /* Look up the local type corresponding at the position encoded by - KEY within FN. */ + KEY within FN and named NAME. */ tree -trees_in::key_local_type (const merge_key& key, tree fn) +trees_in::key_local_type (const merge_key& key, tree fn, tree name) { if (!DECL_INITIAL (fn)) return NULL_TREE; @@ -10464,7 +10464,12 @@ trees_in::key_local_type (const merge_key& key, tree fn) { if (TREE_CODE (var) != TYPE_DECL) continue; - if (decl_ix == decl_pos) + /* Prefer using the identifier as the key for more robustness + to ODR violations, except for anonymous types since their + compiler-generated identifiers aren't stable. */ + if (IDENTIFIER_ANON_P (name) + ? decl_ix == decl_pos + : DECL_NAME (var) == name) return var; ++decl_ix; } @@ -11263,7 +11268,7 @@ trees_in::key_mergeable (int tag, merge_kind mk, tree decl, tree inner, case FUNCTION_DECL: gcc_checking_assert (mk == MK_local_type); - existing = key_local_type (key, container); + existing = key_local_type (key, container, name); if (existing && inner != decl) existing = TYPE_TI_TEMPLATE (TREE_TYPE (existing)); break; @@ -13801,9 +13806,9 @@ depset_cmp (const void *a_, const void *b_) { /* Both are bindings. Order by identifier hash. */ gcc_checking_assert (a->get_name () != b->get_name ()); - return (IDENTIFIER_HASH_VALUE (a->get_name ()) - < IDENTIFIER_HASH_VALUE (b->get_name ()) - ? -1 : +1); + hashval_t ah = IDENTIFIER_HASH_VALUE (a->get_name ()); + hashval_t bh = IDENTIFIER_HASH_VALUE (b->get_name ()); + return (ah == bh ? 0 : ah < bh ? -1 : +1); } /* They are the same decl. This can happen with two using decls diff --git a/gcc/testsuite/g++.dg/modules/merge-17.h b/gcc/testsuite/g++.dg/modules/merge-17.h index a5269959702..5ce52cec3dd 100644 --- a/gcc/testsuite/g++.dg/modules/merge-17.h +++ b/gcc/testsuite/g++.dg/modules/merge-17.h @@ -22,7 +22,37 @@ auto gt() { return e; } +inline auto h0() { + struct { int m; } a0; + struct { char n; } a1; + return a0; +} + +inline auto h1() { + struct { int m; } a0; + struct { char n; } a1; + return a1; +} + +template<class T> +inline auto h0t() { + struct { int m; } a0; + struct { char n; } a1; + return a0; +} + +template<class T> +inline auto h1t() { + struct { int m; } a0; + struct { char n; } a1; + return a1; +} + using ty1 = decltype(f()); using ty2 = decltype(ft<int>()); using ty3 = decltype(g()); using ty4 = decltype(gt<int>()); +using ty5 = decltype(h0()); +using ty6 = decltype(h0t<int>()); +using ty7 = decltype(h1()); +using ty8 = decltype(h1t<int>()); Complete updated patch: -- >8 -- Subject: [PATCH] c++/modules: local type merging [PR99426] PR c++/99426 gcc/cp/ChangeLog: * module.cc (merge_kind::MK_local_type): 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 types.. (trees_out::key_local_type): Define. (trees_in::key_local_type): Define. (trees_out::get_merge_kind) <case FUNCTION_DECL>: Return MK_local_type for a local type. (trees_out::key_mergeable) <case FUNCTION_DECL>: Use key_local_type. (trees_in::key_mergeable) <case FUNCTION_DECL>: Likewise. (trees_in::is_matching_decl): Be flexible with type mismatches for local entities. (trees_in::register_duplicate): Also register the DECL_TEMPLATE_RESULT of a TEMPLATE_DECL as a duplicate. (depset_cmp): Return 0 for equal IDENTIFIER_HASH_VALUE. gcc/testsuite/ChangeLog: * g++.dg/modules/merge-17.h: New test. * g++.dg/modules/merge-17_a.H: New test. * g++.dg/modules/merge-17_b.C: New test. * g++.dg/modules/xtreme-header-7_a.H: New test. * g++.dg/modules/xtreme-header-7_b.C: New test. --- gcc/cp/module.cc | 179 +++++++++++++++--- gcc/testsuite/g++.dg/modules/merge-17.h | 58 ++++++ gcc/testsuite/g++.dg/modules/merge-17_a.H | 3 + gcc/testsuite/g++.dg/modules/merge-17_b.C | 3 + .../g++.dg/modules/xtreme-header-7_a.H | 4 + .../g++.dg/modules/xtreme-header-7_b.C | 5 + 6 files changed, 221 insertions(+), 31 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/merge-17.h create mode 100644 gcc/testsuite/g++.dg/modules/merge-17_a.H create mode 100644 gcc/testsuite/g++.dg/modules/merge-17_b.C 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 ef0280df00a..ee0e5da1d37 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -2772,6 +2772,7 @@ enum merge_kind MK_enum, /* Found by CTX, & 1stMemberNAME. */ MK_keyed, /* Found by key & index. */ + MK_local_type, /* Found by CTX, index. */ MK_friend_spec, /* Like named, but has a tmpl & args too. */ MK_local_friend, /* Found by CTX, index. */ @@ -2798,7 +2799,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 type", "friend spec", "local friend", NULL, /* 8...11 */ NULL, NULL, NULL, NULL, "type spec", "type tmpl spec", /* 16,17 type (template). */ @@ -2932,6 +2933,7 @@ public: unsigned binfo_mergeable (tree *); private: + tree key_local_type (const merge_key&, tree, tree); uintptr_t *find_duplicate (tree existing); void register_duplicate (tree decl, tree existing); /* Mark as an already diagnosed bad duplicate. */ @@ -3092,6 +3094,7 @@ public: void binfo_mergeable (tree binfo); private: + void key_local_type (merge_key&, tree, tree); bool decl_node (tree, walk_kind ref); void type_node (tree); void tree_value (tree); @@ -4959,18 +4962,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); } @@ -6244,7 +6236,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); @@ -6757,7 +6763,29 @@ 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 type or enumerator, chain the + duplicate decl instead of the canonical in-TU decl. Seeing + a duplicate here means the containing function whose body + we're streaming in is a duplicate too, so we'll end up + discarding this BLOCK (and the rest of the duplicate function + body) anyway. */ + decl = maybe_duplicate (decl); + + 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); @@ -10373,6 +10401,88 @@ trees_in::fn_parms_fini (int tag, tree fn, tree existing, bool is_defn) } } +/* Encode into KEY the position of the local type (class or enum) + 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_type (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 type corresponding at the position encoded by + KEY within FN and named NAME. */ + +tree +trees_in::key_local_type (const merge_key& key, tree fn, tree name) +{ + 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; + /* Prefer using the identifier as the key for more robustness + to ODR violations, except for anonymous types since their + compiler-generated identifiers aren't stable. */ + if (IDENTIFIER_ANON_P (name) + ? decl_ix == decl_pos + : DECL_NAME (var) == name) + 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. */ @@ -10492,17 +10602,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_type; break; case RECORD_TYPE: @@ -10804,6 +10907,10 @@ trees_out::key_mergeable (int tag, merge_kind mk, tree decl, tree inner, } break; + case MK_local_type: + key_local_type (key, STRIP_TEMPLATE (decl), container); + break; + case MK_enum: { /* Anonymous enums are located by their first identifier, @@ -11160,11 +11267,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_type); + existing = key_local_type (key, container, name); + if (existing && inner != decl) + existing = TYPE_TI_TEMPLATE (TREE_TYPE (existing)); break; case TYPE_DECL: @@ -11417,6 +11523,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, @@ -11666,6 +11777,12 @@ trees_in::register_duplicate (tree decl, tree existing) uintptr_t &slot = duplicates->get_or_insert (existing, &existed); gcc_checking_assert (!existed); slot = reinterpret_cast<uintptr_t> (decl); + if (TREE_CODE (decl) == TEMPLATE_DECL) + /* Also register the DECL_TEMPLATE_RESULT as a duplicate so + that passing the _RESULT to maybe_duplicate gives us the + existing _RESULT back. */ + register_duplicate (DECL_TEMPLATE_RESULT (decl), + DECL_TEMPLATE_RESULT (existing)); } /* We've read a definition of MAYBE_EXISTING. If not a duplicate, @@ -13689,9 +13806,9 @@ depset_cmp (const void *a_, const void *b_) { /* Both are bindings. Order by identifier hash. */ gcc_checking_assert (a->get_name () != b->get_name ()); - return (IDENTIFIER_HASH_VALUE (a->get_name ()) - < IDENTIFIER_HASH_VALUE (b->get_name ()) - ? -1 : +1); + hashval_t ah = IDENTIFIER_HASH_VALUE (a->get_name ()); + hashval_t bh = IDENTIFIER_HASH_VALUE (b->get_name ()); + return (ah == bh ? 0 : ah < bh ? -1 : +1); } /* They are the same decl. This can happen with two using decls 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..5ce52cec3dd --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/merge-17.h @@ -0,0 +1,58 @@ +// PR c++/99426 + +inline auto f() { + struct A { int m = 42; }; + return A{}; +} + +template<class T> +auto ft() { + decltype(+T()) x; + return [&x] { }; +} + +inline auto g() { + enum E { e }; + return e; +} + +template<class T> +auto gt() { + enum E : T { e }; + return e; +} + +inline auto h0() { + struct { int m; } a0; + struct { char n; } a1; + return a0; +} + +inline auto h1() { + struct { int m; } a0; + struct { char n; } a1; + return a1; +} + +template<class T> +inline auto h0t() { + struct { int m; } a0; + struct { char n; } a1; + return a0; +} + +template<class T> +inline auto h1t() { + struct { int m; } a0; + struct { char n; } a1; + return a1; +} + +using ty1 = decltype(f()); +using ty2 = decltype(ft<int>()); +using ty3 = decltype(g()); +using ty4 = decltype(gt<int>()); +using ty5 = decltype(h0()); +using ty6 = decltype(h0t<int>()); +using ty7 = decltype(h1()); +using ty8 = decltype(h1t<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"; 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..3992a24501b --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/xtreme-header-7_b.C @@ -0,0 +1,5 @@ +// A version of xtreme-header_b.C that doesn't use -fno-module-lazy. +// { dg-additional-options -fmodules-ts } + +#include "xtreme-header.h" +import "xtreme-header-7_a.H"; -- 2.44.0.568.g436d4e5b14 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] c++/modules: local class merging [PR99426] 2024-04-12 18:39 ` Patrick Palka @ 2024-04-12 19:08 ` Jason Merrill 0 siblings, 0 replies; 14+ messages in thread From: Jason Merrill @ 2024-04-12 19:08 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, nathan On 4/12/24 14:39, Patrick Palka wrote: > On Fri, 12 Apr 2024, Jason Merrill wrote: > >> On 4/12/24 13:48, Patrick Palka wrote: >>> On Fri, 12 Apr 2024, Jason Merrill wrote: >>> >>>> On 4/12/24 10:35, Patrick Palka wrote: >>>>> On Wed, 10 Apr 2024, Jason Merrill wrote: >>>>> >>>>>> On 4/10/24 14:48, Patrick Palka wrote: >>>>>>> On Tue, 9 Apr 2024, Jason Merrill wrote: >>>>>>> >>>>>>>> On 3/5/24 10:31, Patrick Palka wrote: >>>>>>>>> On Tue, 27 Feb 2024, Patrick Palka wrote: >>>>>>>>> >>>>>>>>> Subject: [PATCH] c++/modules: local type merging [PR99426] >>>>>>>>> >>>>>>>>> One known missing piece in the modules implementation is merging >>>>>>>>> of >>>>>>>>> a >>>>>>>>> streamed-in local type (class or enum) with the corresponding >>>>>>>>> in-TU >>>>>>>>> version of the local type. 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 types according to their >>>>>>>>> position >>>>>>>>> within the containing function's definition, analogous 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_type): 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 types.. >>>>>>>>> (trees_out::key_local_type): Define. >>>>>>>>> (trees_in::key_local_type): Define. >>>>>>>>> (trees_out::get_merge_kind) <case FUNCTION_DECL>: Return >>>>>>>>> MK_local_type for a local type. >>>>>>>>> (trees_out::key_mergeable) <case FUNCTION_DECL>: Use >>>>>>>>> key_local_type. >>>>>>>>> (trees_in::key_mergeable) <case FUNCTION_DECL>: Likewise. >>>>>>>>> (trees_in::is_matching_decl): Be flexible with type mismatches >>>>>>>>> for local entities. >>>>>>>>> >>>>>>>>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc >>>>>>>>> index 80b63a70a62..d9e34e9a4b9 100644 >>>>>>>>> --- a/gcc/cp/module.cc >>>>>>>>> +++ b/gcc/cp/module.cc >>>>>>>>> @@ -6714,7 +6720,37 @@ 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 type or enumerator, chain the >>>>>>>>> + duplicate decl instead of the canonical in-TU decl. >>>>>>>>> Seeing >>>>>>>>> + a duplicate here means the containing function whose >>>>>>>>> body >>>>>>>>> + we're streaming in is a duplicate too, so we'll end up >>>>>>>>> + discarding this BLOCK (and the rest of the duplicate >>>>>>>>> function >>>>>>>>> + body) anyway. */ >>>>>>>>> + 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)); >>>>>>>>> + } >>>>>>>> >>>>>>>> This seems like a lot of generally-applicable code for finding the >>>>>>>> duplicate, >>>>>>>> which other calls to maybe_duplicate/odr_duplicate don't use. If >>>>>>>> the >>>>>>>> template >>>>>>>> is a duplicate, why isn't its result? If there's a good reason >>>>>>>> for >>>>>>>> that, >>>>>>>> should this template handling go into maybe_duplicate? >>>>>>> >>>>>>> Ah yeah, that makes sense. >>>>>>> >>>>>>> Some context: IIUC modules treats the TEMPLATE_DECL instead of the >>>>>>> DECL_TEMPLATE_RESULT as the canonical decl, which in turn means >>>>>>> we'll >>>>>>> register_duplicate only the TEMPLATE_DECL. But BLOCK_VARS never >>>>>>> contains >>>>>>> a TEMPLATE_DECL, always the DECL_TEMPLATE_RESULT (i.e. a TYPE_DECL), >>>>>>> hence the extra handling. >>>>>>> >>>>>>> Given that it's relatively more difficult to get at the >>>>>>> TEMPLATE_DECL >>>>>>> from the DECL_TEMPLATE_RESULT rather than vice versa, maybe we >>>>>>> should >>>>>>> just register both as duplicates from register_duplicate? That way >>>>>>> callers can just simply pass the DECL_TEMPLATE_RESULT to >>>>>>> maybe_duplicate >>>>>>> and it'll do the right thing. >>>>>> >>>>>> Sounds good. >>>>>> >>>>>>>>> @@ -10337,6 +10373,83 @@ trees_in::fn_parms_fini (int tag, tree >>>>>>>>> fn, >>>>>>>>> tree >>>>>>>>> existing, bool is_defn) >>>>>>>>> } >>>>>>>>> } >>>>>>>>> +/* Encode into KEY the position of the local type (class >>>>>>>>> or >>>>>>>>> enum) >>>>>>>>> + 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. */ >>>>>>>> >>>>>>>> Since we already set DECL_DISCRIMINATOR for mangling, could we use >>>>>>>> it+name >>>>>>>> for >>>>>>>> the key as well? >>>>>>> >>>>>>> We could (and IIUc that'd be more robust to ODR violations), but >>>>>>> wouldn't it mean we'd have to do a linear walk over all BLOCK_VARs >>>>>>> of >>>>>>> all BLOCKS in order to find the one with the matching >>>>>>> name+discriminator? That'd be slower than the current approach >>>>>>> which >>>>>>> lets us skip to the correct BLOCK and walk only its BLOCK_VARS. >>>>>> >>>>>> Ah, good point. How about block number + name instead of the index? >>>>> >>>>> It seems DECL_DISCRIMINATOR is only set at instantiation time and so for >>>>> local types from a function template pattern the field is empty, which >>>>> means we can't use it as the key in general :/ >>>> >>>> I meant just block number and name, without DECL_DISCRIMINATOR. Just >>>> using >>>> the name instead of an index in BLOCK_VARS. >>> >>> Ah, I think that'd be enough for named local types, but what about >>> anonymous local types? IIUC without DECL_DISCRIMINATOR we wouldn't be >>> able to reliably distinguisth between multiple anonymous local types >>> defined in the same block, since their identifiers aren't stable given >>> that they're based off of a global counter (and so sensitive to #include >>> order) :( >> >> Good point. But I'd still think to merge based on name if we have one; as you >> said above, to be more robust to ODR violations. > > Sounds good. > >> >> If the imported fn has a local class that the included header didn't, would we >> get the same problem? > > IIUC yes, all the intra-block indexes would be off by one in that case > and deduplicatation would fail or we'd deduplicate distinct types. > > Here's an incremental diff for the updated patch. The augmented > testcase triggered a latent qsort checking failure in depset_cmp > that was straightforwardly fixed: OK. Jason ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-04-12 19:08 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-02-27 2:37 [PATCH] c++/modules: local class merging [PR99426] Patrick Palka 2024-02-27 18:10 ` Patrick Palka 2024-03-05 15:31 ` Patrick Palka 2024-03-26 14:24 ` Patrick Palka 2024-04-09 20:27 ` Patrick Palka 2024-04-09 21:57 ` Jason Merrill 2024-04-10 18:48 ` Patrick Palka 2024-04-10 22:55 ` Jason Merrill 2024-04-12 14:35 ` Patrick Palka 2024-04-12 17:18 ` Jason Merrill 2024-04-12 17:48 ` Patrick Palka 2024-04-12 18:07 ` Jason Merrill 2024-04-12 18:39 ` Patrick Palka 2024-04-12 19:08 ` 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).