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 = >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
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 = >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
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 = >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 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 = >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 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 = >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
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 = >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
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 = >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 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 = >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
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 = >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, 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
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;
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 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);
> + }
> +
