Re: [PATCH] c++/modules: local class merging [PR99426]
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) : ... here. Stream > BLOCK_VARS manually. > (trees_in::core_vals) : 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) : Return > MK_local_class for a local class. > (trees_out::key_mergeable) : Use > key_local_class. > (trees_in::key_mergeable) : 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); > + } > +
Re: [PATCH] c++/modules: local class merging [PR99426]
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) : ... here. Stream > > BLOCK_VARS manually. > > (trees_in::core_vals) : 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) : Return > > MK_local_class for a local class. > > (trees_out::key_mergeable) : Use > > key_local_class. > > (trees_in::key_mergeable) : 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. *
Re: [PATCH] c++/modules: local class merging [PR99426]
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) : ... here. Stream > > > BLOCK_VARS manually. > > > (trees_in::core_vals) : 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) : Return > > > MK_local_class for a local class. > > > (trees_out::key_mergeable) : Use > > > key_local_class. > > > (trees_in::key_mergeable) : 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; de
Re: [PATCH] c++/modules: local class merging [PR99426]
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) : ... here. Stream > > > > BLOCK_VARS manually. > > > > (trees_in::core_vals) : 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) : Return > > > > MK_local_class for a local class. > > > > (trees_out::key_mergeable) : Use > > > > key_local_class. > > > > (trees_in::key_mergeable) : 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 (dec
Re: [PATCH] c++/modules: local class merging [PR99426]
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) : ... here. Stream BLOCK_VARS manually. (trees_in::core_vals) : 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) : Return MK_local_type for a local type. (trees_out::key_mergeable) : Use key_local_type. (trees_in::key_mergeable) : 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
Re: [PATCH] c++/modules: local class merging [PR99426]
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) : ... here. Stream > > BLOCK_VARS manually. > > (trees_in::core_vals) : 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) : Return > > MK_local_type for a local type. > > (trees_out::key_mergeable) : Use > > key_local_type. > > (trees_in::key_mergeable) : 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 simpl
Re: [PATCH] c++/modules: local class merging [PR99426]
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) : ... here. Stream BLOCK_VARS manually. (trees_in::core_vals) : 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) : Return MK_local_type for a local type. (trees_out::key_mergeable) : Use key_local_type. (trees_in::key_mergeable) : 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
Re: [PATCH] c++/modules: local class merging [PR99426]
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) : ... here. Stream > > > > BLOCK_VARS manually. > > > > (trees_in::core_vals) : 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) : Return > > > > MK_local_type for a local type. > > > > (trees_out::key_mergeable) : Use > > > > key_local_type. > > > > (trees_in::key_mergeable) : 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 wi
Re: [PATCH] c++/modules: local class merging [PR99426]
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) : ... here. Stream BLOCK_VARS manually. (trees_in::core_vals) : 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) : Return MK_local_type for a local type. (trees_out::key_mergeable) : Use key_local_type. (trees_in::key_mergeable) : 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 o
Re: [PATCH] c++/modules: local class merging [PR99426]
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) : ... here. Stream > > > > > > BLOCK_VARS manually. > > > > > > (trees_in::core_vals) : 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) : Return > > > > > > MK_local_type for a local type. > > > > > > (trees_out::key_mergeable) : Use > > > > > > key_local_type. > > > > > > (trees_in::key_mergeable) : 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 > > > >
Re: [PATCH] c++/modules: local class merging [PR99426]
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) : ... here. Stream BLOCK_VARS manually. (trees_in::core_vals) : 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) : Return MK_local_type for a local type. (trees_out::key_mergeable) : Use key_local_type. (trees_in::key_mergeable) : 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.
Re: [PATCH] c++/modules: local class merging [PR99426]
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) : ... here. Stream > > > > > > > > BLOCK_VARS manually. > > > > > > > > (trees_in::core_vals) : 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) : Return > > > > > > > > MK_local_type for a local type. > > > > > > > > (trees_out::key_mergeable) : Use > > > > > > > > key_local_type. > > > > > > > > (trees_in::key_mergeable) : > > > > > > > > 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 c
Re: [PATCH] c++/modules: local class merging [PR99426]
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) : ... here. Stream BLOCK_VARS manually. (trees_in::core_vals) : 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) : Return MK_local_type for a local type. (trees_out::key_mergeable) : Use key_local_type. (trees_in::key_mergeable) : 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 approa