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. > > 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 > >
