On 5/6/24 18:53, Patrick Palka wrote:
On Mon, 6 May 2024, Jason Merrill wrote:
On 5/3/24 07:17, Nathaniel Shead wrote:
On Thu, May 02, 2024 at 02:05:38PM -0400, Jason Merrill wrote:
On 5/1/24 21:34, Nathaniel Shead wrote:
On Thu, May 02, 2024 at 12:15:44AM +1000, Nathaniel Shead wrote:
On Wed, May 01, 2024 at 09:57:38AM -0400, Patrick Palka wrote:
On Wed, 1 May 2024, Nathaniel Shead wrote:
Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk
(and
later 14.2)? I don't think making it a GTY root is necessary but
I felt
perhaps better to be safe than sorry.
Potentially another approach would be to use DECL_UID instead like
how
entity_map does; would that be preferable?
-- >8 --
I got notified by Linaro CI and by checking testresults that there
seems
to be some occasional failures in tpl-friend-4_b.C on some
architectures
and standards modes since r15-59-gb5f6a56940e708. I haven't been
able
to reproduce but looking at the backtrace I suspect the issue is
that
we're adding to the 'imported_temploid_friend' map a decl that is
ultimately discarded, which then has its address reused by a later
decl
causing a failure in the assert in 'set_originating_module'.
This patch attempts to fix the issue in two ways: by ensuring that
we
only store the decl if we know it's a new decl (and hence won't be
discarded), and by making the imported_temploid_friends map a GTY
root
so that even if the decl does get discarded later the address
isn't
reused.
gcc/cp/ChangeLog:
* module.cc (imported_temploid_friends): Mark GTY, and...
(init_modules): ...allocate from GGC.
(trees_in::decl_value): Only write to
imported_temploid_friends
for new decls.
Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
---
gcc/cp/module.cc | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 5b8ff5bc483..37d38bb9654 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -2731,7 +2731,7 @@ static keyed_map_t *keyed_table;
need to be attached to the same module as the temploid.
This maps
these decls to the temploid they are instantiated them, as
there is
no other easy way to get this information. */
-static hash_map<tree, tree> *imported_temploid_friends;
+static GTY(()) hash_map<tree, tree> *imported_temploid_friends;
/********************************************************************/
/* Tree streaming. The tree streaming is very specific to the
tree
@@ -8327,7 +8327,8 @@ trees_in::decl_value ()
if (TREE_CODE (inner) == FUNCTION_DECL
|| TREE_CODE (inner) == TYPE_DECL)
if (tree owner = tree_node ())
- imported_temploid_friends->put (decl, owner);
+ if (is_new)
+ imported_temploid_friends->put (decl, owner);
Hmm, I'm not seeing this code path getting reached for
tpl-friend-4_b.C.
It seems we're instead adding to imported_temploid_friends from
propagate_defining_module, during tsubst_friend_function.
What seems to be happening is that we we first
tsubst_friend_function
'foo' from TPL<int>, and then we tsubst_friend_function 'foo' from
DEF<int>,
which ends up calling duplicate_decls, which ggc_frees this 'foo'
redeclaration that is still present in the imported_temploid_friends
map.
So I don't think marking imported_temploid_friends as a GC root
would
help with this situation. If we want to keep
imported_temploid_friends
as a tree -> tree map, I think we just need to ensure that a decl
is removed from the map upon getting ggc_free'd from e.g.
duplicate_decls.
Could we instead move the call to propagate_defining_module down a few
lines, after the pushdecl?
Doing that for tsubst_friend_class seems to work OK with my current test
cases, but for tsubst_friend_function doing so causes ICEs in
'module_may_redeclare' within duplicate_decls because the function is
already marked as attached but the originating module information hasn't
been setup yet.
It's unfortunate that we need to add a hash table entry in order to make it
through duplicate_decls, at which point it becomes dead. Ah, well.
I suppose with tsubst_friend_class it works though because we can't ever
take the pushdecl branch if an existing type exists that we would call
duplicate_decls on.
But it seems simpler to use DECL_UID as the key instead, since those
never get reused even after the decl gets ggc_free'd IIUC.
It still means garbage entries in the hash_map, which is undesirable even
if
it doesn't cause the same kind of breakage.
Incidentally, decl_tree_map is preferable to hash_map<tree,tree> when the
key is always a decl.
Ah thanks, didn't know about decl_tree_map. I feel that I prefer using
DECL_UIDs explicitly here though; it's also consistent with the existing
usage in entity_map_t, and it looks like decl_tree_map is still perhaps
vulnerable to the original issue here (since DECL_UID is only used for
hashing and not for equality, it looks like?).
Though that said, 'decl_constraints' in constraints.cc seems to be using
it fine (well, with a GTY marking) by using 'remove_constraints' within
duplicate_decls to clear this: Here's a version of the patch that
implements in the same way, is something like this preferable?
Bootstrapped and regtested on x86_64-pc-linux-gnu.
-- >8 --
I got notified by Linaro CI and by checking testresults that there seems
to be some occasional failures in tpl-friend-4_b.C on some architectures
and standards modes since r15-59-gb5f6a56940e708. I haven't been able
to reproduce but looking at the backtrace I suspect the issue is that
we're adding to the 'imported_temploid_friend' map a decl that is
ultimately discarded, which then has its address reused by a later decl
causing a failure in the assert in 'set_originating_module'.
This patch fixes the problem by ensuring 'imported_temploid_friends' is
correctly marked as a GTY root, and that 'duplicate_decls' properly
removes entries from the map for declarations that it frees.
Hmm, I think only one or the other of those is necessary: if duplicate_decls
removes entries, there won't be garbage in the map. Alternatively, if the map
is marked GTY((cache)), garbage entries will be collected.
If the map is only marked GTY((cache)), isn't there a chance that the
ggc_free'd memory corresponding to a garbage entry gets reused, perhaps
for another decl, before the entry got collected? And then the garbage
entry would appear live, and incorrectly refer to an unrelated decl.
Hmm, good point; sounds extremely unlikely but possible. OK, let's just
go with this patch.
Jason
The same should be true of decl_constraints.
The other reason to mark something GTY is so that it's properly dumped to PCH,
but using PCH with modules would be weird.
Does using only one or the other affect compilation time measurably?
IIRC in practice with a release compiler the GC doesn't seem to run at
all unless the heap grows to like over 1GB, at least on my machine, so
adding a GC root should be negligible in practice. The GC runs much
more often with a checking compiler a new GC root might be measurable
there. But I'd expect the size of imported_temploid_friends to be
pretty small?
Jason
