* [PATCH] c++/modules: Fix dangling pointer with imported_temploid_friends @ 2024-05-01 4:30 Nathaniel Shead 2024-05-01 13:57 ` Patrick Palka 0 siblings, 1 reply; 10+ messages in thread From: Nathaniel Shead @ 2024-05-01 4:30 UTC (permalink / raw) To: gcc-patches; +Cc: Jason Merrill, Nathan Sidwell 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 <nathanieloshead@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); /* Regular typedefs will have a NULL TREE_TYPE at this point. */ unsigned tdef_flags = 0; @@ -20523,7 +20524,7 @@ init_modules (cpp_reader *reader) entity_map = new entity_map_t (EXPERIMENT (1, 400)); vec_safe_reserve (entity_ary, EXPERIMENT (1, 400)); imported_temploid_friends - = new hash_map<tree,tree> (EXPERIMENT (1, 400)); + = hash_map<tree,tree>::create_ggc (EXPERIMENT (1, 400)); } #if CHECKING_P -- 2.43.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++/modules: Fix dangling pointer with imported_temploid_friends 2024-05-01 4:30 [PATCH] c++/modules: Fix dangling pointer with imported_temploid_friends Nathaniel Shead @ 2024-05-01 13:57 ` Patrick Palka 2024-05-01 14:15 ` Nathaniel Shead 0 siblings, 1 reply; 10+ messages in thread From: Patrick Palka @ 2024-05-01 13:57 UTC (permalink / raw) To: Nathaniel Shead; +Cc: gcc-patches, Jason Merrill, Nathan Sidwell 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 <nathanieloshead@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. 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. > > /* Regular typedefs will have a NULL TREE_TYPE at this point. */ > unsigned tdef_flags = 0; > @@ -20523,7 +20524,7 @@ init_modules (cpp_reader *reader) > entity_map = new entity_map_t (EXPERIMENT (1, 400)); > vec_safe_reserve (entity_ary, EXPERIMENT (1, 400)); > imported_temploid_friends > - = new hash_map<tree,tree> (EXPERIMENT (1, 400)); > + = hash_map<tree,tree>::create_ggc (EXPERIMENT (1, 400)); > } > > #if CHECKING_P > -- > 2.43.2 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] c++/modules: Fix dangling pointer with imported_temploid_friends 2024-05-01 13:57 ` Patrick Palka @ 2024-05-01 14:15 ` Nathaniel Shead 2024-05-02 1:34 ` [PATCH v2] " Nathaniel Shead 0 siblings, 1 reply; 10+ messages in thread From: Nathaniel Shead @ 2024-05-01 14:15 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, Jason Merrill, Nathan Sidwell 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 <nathanieloshead@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. > > 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. > Ah right, thanks for digging into that further. Yup OK, I think probably the DECL_UID route feels safer to me then in case there are other places where a decl might be explicitly freed. Looking at tree.cc it looks like the relevant function is 'allocate_decl_uid' which shouldn't reuse UIDs until 2^32 decls have been created, so we should be safe on the reuse front. I'll draft and test a patch for that tomorrow morning. > > > > /* Regular typedefs will have a NULL TREE_TYPE at this point. */ > > unsigned tdef_flags = 0; > > @@ -20523,7 +20524,7 @@ init_modules (cpp_reader *reader) > > entity_map = new entity_map_t (EXPERIMENT (1, 400)); > > vec_safe_reserve (entity_ary, EXPERIMENT (1, 400)); > > imported_temploid_friends > > - = new hash_map<tree,tree> (EXPERIMENT (1, 400)); > > + = hash_map<tree,tree>::create_ggc (EXPERIMENT (1, 400)); > > } > > > > #if CHECKING_P > > -- > > 2.43.2 > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] c++/modules: Fix dangling pointer with imported_temploid_friends 2024-05-01 14:15 ` Nathaniel Shead @ 2024-05-02 1:34 ` Nathaniel Shead 2024-05-02 14:16 ` Patrick Palka 2024-05-02 18:05 ` Jason Merrill 0 siblings, 2 replies; 10+ messages in thread From: Nathaniel Shead @ 2024-05-02 1:34 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches, Jason Merrill, Nathan Sidwell 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 <nathanieloshead@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. > > > > 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. > > > > Ah right, thanks for digging into that further. Yup OK, I think > probably the DECL_UID route feels safer to me then in case there are > other places where a decl might be explicitly freed. > > Looking at tree.cc it looks like the relevant function is > 'allocate_decl_uid' which shouldn't reuse UIDs until 2^32 decls have > been created, so we should be safe on the reuse front. > > I'll draft and test a patch for that tomorrow morning. > Here's that patch. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/14.2? -- >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 using DECL_UID as the map key instead of the tree directly, much like with entity_map, since even if a declaration gets deallocated the DECL_UID should not be reused by a later declaration. gcc/cp/ChangeLog: * module.cc (imported_temploid_friends): Map from DECL_UID instead of tree. (get_originating_module_decl): Likewise. (propagate_defining_module): Likewise. (trees_out::decl_value): Likewise. (trees_in::decl_value): Likewise. And only enter into the map for new declarations. (init_modules): Update type of imported_temploid_friends. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/module.cc | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index fac0301d80e..ceb383ba509 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -2730,8 +2730,13 @@ static keyed_map_t *keyed_table; /* Instantiations of temploid friends imported from another module 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; + no other easy way to get this information. We map the DECL_UID + instead of the tree directly to handle keys that get freed and + reused. */ +typedef hash_map<unsigned, tree, + simple_hashmap_traits<int_hash<unsigned, 0>, tree> + > temploid_map_t; +static temploid_map_t *imported_temploid_friends; /********************************************************************/ /* Tree streaming. The tree streaming is very specific to the tree @@ -7829,7 +7834,7 @@ trees_out::decl_value (tree decl, depset *dep) /* But don't consider imported temploid friends as attached, since importers will need to merge this decl even if it was attached to a different module. */ - if (imported_temploid_friends->get (decl)) + if (imported_temploid_friends->get (DECL_UID (decl))) is_attached = false; bits.b (is_attached); @@ -8014,7 +8019,7 @@ trees_out::decl_value (tree decl, depset *dep) { /* Write imported temploid friends so that importers can reconstruct this information on stream-in. */ - tree* slot = imported_temploid_friends->get (decl); + tree* slot = imported_temploid_friends->get (DECL_UID (decl)); tree_node (slot ? *slot : NULL_TREE); } @@ -8327,7 +8332,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_UID (decl), owner); /* Regular typedefs will have a NULL TREE_TYPE at this point. */ unsigned tdef_flags = 0; @@ -18976,7 +18982,7 @@ get_originating_module_decl (tree decl) /* An imported temploid friend is attached to the same module the befriending class was. */ if (imported_temploid_friends) - if (tree *slot = imported_temploid_friends->get (decl)) + if (tree *slot = imported_temploid_friends->get (DECL_UID (decl))) decl = *slot; int use; @@ -19306,7 +19312,7 @@ propagate_defining_module (tree decl, tree orig) if (DECL_LANG_SPECIFIC (not_tmpl) && DECL_MODULE_IMPORT_P (not_tmpl)) { - bool exists = imported_temploid_friends->put (decl, orig); + bool exists = imported_temploid_friends->put (DECL_UID (decl), orig); /* We should only be called if lookup for an existing decl failed, in which case there shouldn't already be an entry @@ -20528,8 +20534,7 @@ init_modules (cpp_reader *reader) pending_table = new pending_map_t (EXPERIMENT (1, 400)); entity_map = new entity_map_t (EXPERIMENT (1, 400)); vec_safe_reserve (entity_ary, EXPERIMENT (1, 400)); - imported_temploid_friends - = new hash_map<tree,tree> (EXPERIMENT (1, 400)); + imported_temploid_friends = new temploid_map_t (EXPERIMENT (1, 400)); } #if CHECKING_P -- 2.43.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] c++/modules: Fix dangling pointer with imported_temploid_friends 2024-05-02 1:34 ` [PATCH v2] " Nathaniel Shead @ 2024-05-02 14:16 ` Patrick Palka 2024-05-02 18:05 ` Jason Merrill 1 sibling, 0 replies; 10+ messages in thread From: Patrick Palka @ 2024-05-02 14:16 UTC (permalink / raw) To: Nathaniel Shead; +Cc: gcc-patches, Jason Merrill, Nathan Sidwell On Wed, May 1, 2024 at 9:35 PM Nathaniel Shead <nathanieloshead@gmail.com> 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 <nathanieloshead@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. > > > > > > 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. > > > > > > > Ah right, thanks for digging into that further. Yup OK, I think > > probably the DECL_UID route feels safer to me then in case there are > > other places where a decl might be explicitly freed. > > > > Looking at tree.cc it looks like the relevant function is > > 'allocate_decl_uid' which shouldn't reuse UIDs until 2^32 decls have > > been created, so we should be safe on the reuse front. > > > > I'll draft and test a patch for that tomorrow morning. > > > > Here's that patch. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK > for trunk/14.2? LGTM > > -- >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 using DECL_UID as the map key instead of > the tree directly, much like with entity_map, since even if a > declaration gets deallocated the DECL_UID should not be reused by a > later declaration. > > gcc/cp/ChangeLog: > > * module.cc (imported_temploid_friends): Map from DECL_UID > instead of tree. > (get_originating_module_decl): Likewise. > (propagate_defining_module): Likewise. > (trees_out::decl_value): Likewise. > (trees_in::decl_value): Likewise. And only enter into the map > for new declarations. > (init_modules): Update type of imported_temploid_friends. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/module.cc | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index fac0301d80e..ceb383ba509 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -2730,8 +2730,13 @@ static keyed_map_t *keyed_table; > /* Instantiations of temploid friends imported from another module > 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; > + no other easy way to get this information. We map the DECL_UID > + instead of the tree directly to handle keys that get freed and > + reused. */ > +typedef hash_map<unsigned, tree, > + simple_hashmap_traits<int_hash<unsigned, 0>, tree> > + > temploid_map_t; > +static temploid_map_t *imported_temploid_friends; > > /********************************************************************/ > /* Tree streaming. The tree streaming is very specific to the tree > @@ -7829,7 +7834,7 @@ trees_out::decl_value (tree decl, depset *dep) > /* But don't consider imported temploid friends as attached, > since importers will need to merge this decl even if it was > attached to a different module. */ > - if (imported_temploid_friends->get (decl)) > + if (imported_temploid_friends->get (DECL_UID (decl))) > is_attached = false; > > bits.b (is_attached); > @@ -8014,7 +8019,7 @@ trees_out::decl_value (tree decl, depset *dep) > { > /* Write imported temploid friends so that importers can reconstruct > this information on stream-in. */ > - tree* slot = imported_temploid_friends->get (decl); > + tree* slot = imported_temploid_friends->get (DECL_UID (decl)); > tree_node (slot ? *slot : NULL_TREE); > } > > @@ -8327,7 +8332,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_UID (decl), owner); > > /* Regular typedefs will have a NULL TREE_TYPE at this point. */ > unsigned tdef_flags = 0; > @@ -18976,7 +18982,7 @@ get_originating_module_decl (tree decl) > /* An imported temploid friend is attached to the same module the > befriending class was. */ > if (imported_temploid_friends) > - if (tree *slot = imported_temploid_friends->get (decl)) > + if (tree *slot = imported_temploid_friends->get (DECL_UID (decl))) > decl = *slot; > > int use; > @@ -19306,7 +19312,7 @@ propagate_defining_module (tree decl, tree orig) > > if (DECL_LANG_SPECIFIC (not_tmpl) && DECL_MODULE_IMPORT_P (not_tmpl)) > { > - bool exists = imported_temploid_friends->put (decl, orig); > + bool exists = imported_temploid_friends->put (DECL_UID (decl), orig); > > /* We should only be called if lookup for an existing decl > failed, in which case there shouldn't already be an entry > @@ -20528,8 +20534,7 @@ init_modules (cpp_reader *reader) > pending_table = new pending_map_t (EXPERIMENT (1, 400)); > entity_map = new entity_map_t (EXPERIMENT (1, 400)); > vec_safe_reserve (entity_ary, EXPERIMENT (1, 400)); > - imported_temploid_friends > - = new hash_map<tree,tree> (EXPERIMENT (1, 400)); > + imported_temploid_friends = new temploid_map_t (EXPERIMENT (1, 400)); > } > > #if CHECKING_P > -- > 2.43.2 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] c++/modules: Fix dangling pointer with imported_temploid_friends 2024-05-02 1:34 ` [PATCH v2] " Nathaniel Shead 2024-05-02 14:16 ` Patrick Palka @ 2024-05-02 18:05 ` Jason Merrill 2024-05-03 11:17 ` [PATCH v3] " Nathaniel Shead 1 sibling, 1 reply; 10+ messages in thread From: Jason Merrill @ 2024-05-02 18:05 UTC (permalink / raw) To: Nathaniel Shead, Patrick Palka; +Cc: gcc-patches, Nathan Sidwell 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 <nathanieloshead@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? >>> 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. Jason ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3] c++/modules: Fix dangling pointer with imported_temploid_friends 2024-05-02 18:05 ` Jason Merrill @ 2024-05-03 11:17 ` Nathaniel Shead 2024-05-06 22:19 ` Jason Merrill 0 siblings, 1 reply; 10+ messages in thread From: Nathaniel Shead @ 2024-05-03 11:17 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches, Nathan Sidwell 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 <nathanieloshead@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. 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. > > Jason > 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. gcc/cp/ChangeLog: * cp-tree.h (remove_defining_module): Declare. * decl.cc (duplicate_decls): Call remove_defining_module on to-be-freed newdecl. * module.cc (imported_temploid_friends): Mark as GTY root... (init_modules): ...and allocate from ggc. (trees_in::decl_value): Only track for declarations that won't be discarded. (remove_defining_module): New function. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/cp-tree.h | 1 + gcc/cp/decl.cc | 4 ++++ gcc/cp/module.cc | 19 ++++++++++++++++--- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 933504b4821..b7c09bf78c4 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7420,6 +7420,7 @@ extern void set_instantiating_module (tree); extern void set_defining_module (tree); extern void maybe_key_decl (tree ctx, tree decl); extern void propagate_defining_module (tree decl, tree orig); +extern void remove_defining_module (tree decl); extern void mangle_module (int m, bool include_partition); extern void mangle_module_fini (); diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc index 378311c0f04..4af3ec48b42 100644 --- a/gcc/cp/decl.cc +++ b/gcc/cp/decl.cc @@ -3340,6 +3340,10 @@ duplicate_decls (tree newdecl, tree olddecl, bool hiding, bool was_hidden) if (flag_concepts) remove_constraints (newdecl); + /* And similarly for any module tracking data. */ + if (modules_p ()) + remove_defining_module (newdecl); + ggc_free (newdecl); return olddecl; diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 44dc81eed3e..520dd710549 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((cache)) decl_tree_cache_map *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); /* Regular typedefs will have a NULL TREE_TYPE at this point. */ unsigned tdef_flags = 0; @@ -19336,6 +19337,18 @@ propagate_defining_module (tree decl, tree orig) } } +/* DECL is being freed, clear data we don't need anymore. */ + +void +remove_defining_module (tree decl) +{ + if (!modules_p ()) + return; + + if (imported_temploid_friends) + imported_temploid_friends->remove (decl); +} + /* Create the flat name string. It is simplest to have it handy. */ void @@ -20550,7 +20563,7 @@ init_modules (cpp_reader *reader) entity_map = new entity_map_t (EXPERIMENT (1, 400)); vec_safe_reserve (entity_ary, EXPERIMENT (1, 400)); imported_temploid_friends - = new hash_map<tree,tree> (EXPERIMENT (1, 400)); + = decl_tree_cache_map::create_ggc (EXPERIMENT (1, 400)); } #if CHECKING_P -- 2.43.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] c++/modules: Fix dangling pointer with imported_temploid_friends 2024-05-03 11:17 ` [PATCH v3] " Nathaniel Shead @ 2024-05-06 22:19 ` Jason Merrill 2024-05-06 22:53 ` Patrick Palka 0 siblings, 1 reply; 10+ messages in thread From: Jason Merrill @ 2024-05-06 22:19 UTC (permalink / raw) To: Nathaniel Shead; +Cc: Patrick Palka, gcc-patches, Nathan Sidwell 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 <nathanieloshead@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. 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? Jason ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] c++/modules: Fix dangling pointer with imported_temploid_friends 2024-05-06 22:19 ` Jason Merrill @ 2024-05-06 22:53 ` Patrick Palka 2024-05-07 0:28 ` Jason Merrill 0 siblings, 1 reply; 10+ messages in thread From: Patrick Palka @ 2024-05-06 22:53 UTC (permalink / raw) To: Jason Merrill; +Cc: Nathaniel Shead, Patrick Palka, gcc-patches, Nathan Sidwell 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 <nathanieloshead@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 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] c++/modules: Fix dangling pointer with imported_temploid_friends 2024-05-06 22:53 ` Patrick Palka @ 2024-05-07 0:28 ` Jason Merrill 0 siblings, 0 replies; 10+ messages in thread From: Jason Merrill @ 2024-05-07 0:28 UTC (permalink / raw) To: Patrick Palka; +Cc: Nathaniel Shead, gcc-patches, Nathan Sidwell 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 <nathanieloshead@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 >> >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-05-07 0:28 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-05-01 4:30 [PATCH] c++/modules: Fix dangling pointer with imported_temploid_friends Nathaniel Shead 2024-05-01 13:57 ` Patrick Palka 2024-05-01 14:15 ` Nathaniel Shead 2024-05-02 1:34 ` [PATCH v2] " Nathaniel Shead 2024-05-02 14:16 ` Patrick Palka 2024-05-02 18:05 ` Jason Merrill 2024-05-03 11:17 ` [PATCH v3] " Nathaniel Shead 2024-05-06 22:19 ` Jason Merrill 2024-05-06 22:53 ` Patrick Palka 2024-05-07 0:28 ` Jason Merrill
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).