On 4/12/24 14:39, Patrick Palka wrote:
On Fri, 12 Apr 2024, Jason Merrill wrote:
On 4/12/24 13:48, Patrick Palka wrote:
On Fri, 12 Apr 2024, Jason Merrill wrote:
On 4/12/24 10:35, Patrick Palka wrote:
On Wed, 10 Apr 2024, Jason Merrill wrote:
On 4/10/24 14:48, Patrick Palka wrote:
On Tue, 9 Apr 2024, Jason Merrill wrote:
On 3/5/24 10:31, Patrick Palka wrote:
On Tue, 27 Feb 2024, Patrick Palka wrote:
Subject: [PATCH] c++/modules: local type merging [PR99426]
One known missing piece in the modules implementation is merging
of
a
streamed-in local type (class or enum) with the corresponding
in-TU
version of the local type. This missing piece turns out to
cause a
hard-to-reduce use-after-free GC issue due to the entity_ary not
being
marked as a GC root (deliberately), and manifests as a
serialization
error on stream-in as in PR99426 (see comment #6 for a
reduction).
It's
also reproducible on trunk when running the xtreme-header tests
without
-fno-module-lazy.
This patch makes us merge such local types according to their
position
within the containing function's definition, analogous to how we
merge
FIELD_DECLs of a class according to their index in the
TYPE_FIELDS
list.
PR c++/99426
gcc/cp/ChangeLog:
* module.cc (merge_kind::MK_local_type): New enumerator.
(merge_kind_name): Update.
(trees_out::chained_decls): Move BLOCK-specific handling
of DECL_LOCAL_DECL_P decls to ...
(trees_out::core_vals) <case BLOCK>: ... here. Stream
BLOCK_VARS manually.
(trees_in::core_vals) <case BLOCK>: Stream BLOCK_VARS
manually. Handle deduplicated local types..
(trees_out::key_local_type): Define.
(trees_in::key_local_type): Define.
(trees_out::get_merge_kind) <case FUNCTION_DECL>: Return
MK_local_type for a local type.
(trees_out::key_mergeable) <case FUNCTION_DECL>: Use
key_local_type.
(trees_in::key_mergeable) <case FUNCTION_DECL>: Likewise.
(trees_in::is_matching_decl): Be flexible with type mismatches
for local entities.
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 80b63a70a62..d9e34e9a4b9 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -6714,7 +6720,37 @@ trees_in::core_vals (tree t)
case BLOCK:
t->block.locus = state->read_location (*this);
t->block.end_locus = state->read_location (*this);
- t->block.vars = chained_decls ();
+
+ for (tree *chain = &t->block.vars;;)
+ if (tree decl = tree_node ())
+ {
+ /* For a deduplicated local type or enumerator, chain the
+ duplicate decl instead of the canonical in-TU decl.
Seeing
+ a duplicate here means the containing function whose
body
+ we're streaming in is a duplicate too, so we'll end up
+ discarding this BLOCK (and the rest of the duplicate
function
+ body) anyway. */
+ if (is_duplicate (decl))
+ decl = maybe_duplicate (decl);
+ else if (DECL_IMPLICIT_TYPEDEF_P (decl)
+ && TYPE_TEMPLATE_INFO (TREE_TYPE (decl)))
+ {
+ tree tmpl = TYPE_TI_TEMPLATE (TREE_TYPE (decl));
+ if (DECL_TEMPLATE_RESULT (tmpl) == decl &&
is_duplicate
(tmpl))
+ decl = DECL_TEMPLATE_RESULT (maybe_duplicate
(tmpl));
+ }
This seems like a lot of generally-applicable code for finding the
duplicate,
which other calls to maybe_duplicate/odr_duplicate don't use. If
the
template
is a duplicate, why isn't its result? If there's a good reason
for
that,
should this template handling go into maybe_duplicate?
Ah yeah, that makes sense.
Some context: IIUC modules treats the TEMPLATE_DECL instead of the
DECL_TEMPLATE_RESULT as the canonical decl, which in turn means
we'll
register_duplicate only the TEMPLATE_DECL. But BLOCK_VARS never
contains
a TEMPLATE_DECL, always the DECL_TEMPLATE_RESULT (i.e. a TYPE_DECL),
hence the extra handling.
Given that it's relatively more difficult to get at the
TEMPLATE_DECL
from the DECL_TEMPLATE_RESULT rather than vice versa, maybe we
should
just register both as duplicates from register_duplicate? That way
callers can just simply pass the DECL_TEMPLATE_RESULT to
maybe_duplicate
and it'll do the right thing.
Sounds good.
@@ -10337,6 +10373,83 @@ trees_in::fn_parms_fini (int tag, tree
fn,
tree
existing, bool is_defn)
}
}
+/* Encode into KEY the position of the local type (class
or
enum)
+ declaration DECL within FN. The position is encoded as the
+ index of the innermost BLOCK (numbered in BFS order) along
with
+ the index within its BLOCK_VARS list. */
Since we already set DECL_DISCRIMINATOR for mangling, could we use
it+name
for
the key as well?
We could (and IIUc that'd be more robust to ODR violations), but
wouldn't it mean we'd have to do a linear walk over all BLOCK_VARs
of
all BLOCKS in order to find the one with the matching
name+discriminator? That'd be slower than the current approach
which
lets us skip to the correct BLOCK and walk only its BLOCK_VARS.
Ah, good point. How about block number + name instead of the index?
It seems DECL_DISCRIMINATOR is only set at instantiation time and so for
local types from a function template pattern the field is empty, which
means we can't use it as the key in general :/
I meant just block number and name, without DECL_DISCRIMINATOR. Just
using
the name instead of an index in BLOCK_VARS.
Ah, I think that'd be enough for named local types, but what about
anonymous local types? IIUC without DECL_DISCRIMINATOR we wouldn't be
able to reliably distinguisth between multiple anonymous local types
defined in the same block, since their identifiers aren't stable given
that they're based off of a global counter (and so sensitive to #include
order) :(
Good point. But I'd still think to merge based on name if we have one; as you
said above, to be more robust to ODR violations.
Sounds good.
If the imported fn has a local class that the included header didn't, would we
get the same problem?
IIUC yes, all the intra-block indexes would be off by one in that case
and deduplicatation would fail or we'd deduplicate distinct types.
Here's an incremental diff for the updated patch. The augmented
testcase triggered a latent qsort checking failure in depset_cmp
that was straightforwardly fixed:
OK.
Jason
