[C++] Fix concepts vs. PCH (PR c++/92458)
diff mbox series

Message ID 20191112000230.GC4650@tucnak
State New
Headers show
Series
  • [C++] Fix concepts vs. PCH (PR c++/92458)
Related show

Commit Message

Jakub Jelinek Nov. 12, 2019, 12:02 a.m. UTC
Hi!

PCH remaps object addresses, so hash tables that use pointer hashing
don't work well across PCH, because the hash values can change and without
rehashing the hash tbales values might not be found.

The following patch fixes it by using DECL_UID as hash function instead,
which is stable across PCH save/restore.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2019-11-11  Jakub Jelinek  <jakub@redhat.com>

	PR c++/92458
	* constraint.cc: Include tree-hash-traits.h.
	(decl_tree_cache_traits): New type.
	(decl_tree_cache_map): New typedef.
	(decl_constraints): Change type to decl_tree_cache_map *
	from tree_cache_map *.

	* g++.dg/pch/pr92458.C: New test.
	* g++.dg/pch/pr92458.Hs: New test.


	Jakub

Comments

Jason Merrill Nov. 18, 2019, 7:41 p.m. UTC | #1
On 11/12/19 12:02 AM, Jakub Jelinek wrote:
> Hi!
> 
> PCH remaps object addresses, so hash tables that use pointer hashing
> don't work well across PCH, because the hash values can change and without
> rehashing the hash table's values might not be found.
> 
> The following patch fixes it by using DECL_UID as hash function instead,
> which is stable across PCH save/restore.
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2019-11-11  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/92458
> 	* constraint.cc: Include tree-hash-traits.h.
> 	(decl_tree_cache_traits): New type.
> 	(decl_tree_cache_map): New typedef.
> 	(decl_constraints): Change type to decl_tree_cache_map *
> 	from tree_cache_map *.

Do we need to change other tree_cache_map uses, too?  It looks like many 
of them map from decls.

> 	* g++.dg/pch/pr92458.C: New test.
> 	* g++.dg/pch/pr92458.Hs: New test.
> 
> --- gcc/cp/constraint.cc.jj	2019-11-07 09:50:51.755239234 +0100
> +++ gcc/cp/constraint.cc	2019-11-11 19:04:03.231862024 +0100
> @@ -45,6 +45,7 @@ along with GCC; see the file COPYING3.
>   #include "decl.h"
>   #include "toplev.h"
>   #include "type-utils.h"
> +#include "tree-hash-traits.h"
>   
>   static tree satisfaction_value (tree t);
>   
> @@ -1113,7 +1114,10 @@ build_constraints (tree tr, tree dr)
>   
>   /* A mapping from declarations to constraint information.  */
>   
> -static GTY ((cache)) tree_cache_map *decl_constraints;
> +struct decl_tree_cache_traits
> +  : simple_cache_map_traits<tree_decl_hash, tree> { };
> +typedef hash_map<tree,tree,decl_tree_cache_traits> decl_tree_cache_map;
> +static GTY ((cache)) decl_tree_cache_map *decl_constraints;
>   
>   /* Returns the template constraints of declaration T. If T is not
>      constrained, return NULL_TREE. Note that T must be non-null. */
> --- gcc/testsuite/g++.dg/pch/pr92458.C.jj	2019-11-11 19:09:51.547614022 +0100
> +++ gcc/testsuite/g++.dg/pch/pr92458.C	2019-11-11 19:12:39.164088578 +0100
> @@ -0,0 +1,5 @@
> +// PR c++/92458
> +// { dg-options "-std=c++2a" }
> +
> +#include "pr92458.H"
> +S<int> s;
> --- gcc/testsuite/g++.dg/pch/pr92458.Hs.jj	2019-11-11 19:09:25.091012637 +0100
> +++ gcc/testsuite/g++.dg/pch/pr92458.Hs	2019-11-11 19:12:47.626961060 +0100
> @@ -0,0 +1,7 @@
> +// PR c++/92458
> +// { dg-options "-std=c++2a" }
> +
> +template<typename T> concept C = sizeof(T) > 1;
> +template<typename T> struct S { };
> +template<typename T> requires C<T> struct S<T> { };
> +template<typename T> requires (!C<T>) struct S<T> { };
> 
> 	Jakub
>
Jakub Jelinek Nov. 20, 2019, 7:21 p.m. UTC | #2
On Mon, Nov 18, 2019 at 02:41:48PM -0500, Jason Merrill wrote:
> > 2019-11-11  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR c++/92458
> > 	* constraint.cc: Include tree-hash-traits.h.
> > 	(decl_tree_cache_traits): New type.
> > 	(decl_tree_cache_map): New typedef.
> > 	(decl_constraints): Change type to decl_tree_cache_map *
> > 	from tree_cache_map *.
> 
> Do we need to change other tree_cache_map uses, too?  It looks like many of
> them map from decls.

I guess it depends on whether those hash tables can have data in them across
PCH save/restore.
As an experiment, I've built stdc++.h.gch with -std=c++2a and put a
breakpoint in c_common_read_pch after gt_pch_restore.
Besides decl_constraints I've changed, I see also defarg_inst table with
data on it, which means that defarg_inst lookups after PCH read might not
find saved instantiations in the table.
So, defarg_inst might be another candidate for decl_tree_cache_map,
especially because PARM_DECLs are the keys in it.
All the other C++ FE tree_cache_map hash tables are empty, so no idea if it
is needed or not.

If decl_tree_cache_map will be needed in more than one spot, I'll probably
need to move it to some generic header.

> > --- gcc/cp/constraint.cc.jj	2019-11-07 09:50:51.755239234 +0100
> > +++ gcc/cp/constraint.cc	2019-11-11 19:04:03.231862024 +0100
> > @@ -45,6 +45,7 @@ along with GCC; see the file COPYING3.
> >   #include "decl.h"
> >   #include "toplev.h"
> >   #include "type-utils.h"
> > +#include "tree-hash-traits.h"
> >   static tree satisfaction_value (tree t);
> > @@ -1113,7 +1114,10 @@ build_constraints (tree tr, tree dr)
> >   /* A mapping from declarations to constraint information.  */
> > -static GTY ((cache)) tree_cache_map *decl_constraints;
> > +struct decl_tree_cache_traits
> > +  : simple_cache_map_traits<tree_decl_hash, tree> { };
> > +typedef hash_map<tree,tree,decl_tree_cache_traits> decl_tree_cache_map;
> > +static GTY ((cache)) decl_tree_cache_map *decl_constraints;
> >   /* Returns the template constraints of declaration T. If T is not
> >      constrained, return NULL_TREE. Note that T must be non-null. */

	Jakub
Jason Merrill Nov. 21, 2019, 12:46 a.m. UTC | #3
On 11/20/19 7:21 PM, Jakub Jelinek wrote:
> On Mon, Nov 18, 2019 at 02:41:48PM -0500, Jason Merrill wrote:
>>> 2019-11-11  Jakub Jelinek  <jakub@redhat.com>
>>>
>>> 	PR c++/92458
>>> 	* constraint.cc: Include tree-hash-traits.h.
>>> 	(decl_tree_cache_traits): New type.
>>> 	(decl_tree_cache_map): New typedef.
>>> 	(decl_constraints): Change type to decl_tree_cache_map *
>>> 	from tree_cache_map *.
>>
>> Do we need to change other tree_cache_map uses, too?  It looks like many of
>> them map from decls.
> 
> I guess it depends on whether those hash tables can have data in them across
> PCH save/restore.
> As an experiment, I've built stdc++.h.gch with -std=c++2a and put a
> breakpoint in c_common_read_pch after gt_pch_restore.
> Besides decl_constraints I've changed, I see also defarg_inst table with
> data on it, which means that defarg_inst lookups after PCH read might not
> find saved instantiations in the table.
> So, defarg_inst might be another candidate for decl_tree_cache_map,
> especially because PARM_DECLs are the keys in it.
> All the other C++ FE tree_cache_map hash tables are empty, so no idea if it
> is needed or not.

> If decl_tree_cache_map will be needed in more than one spot, I'll probably
> need to move it to some generic header.

Most of them probably need it, for code that uses the relevant features. 
  Except debug_type_map, which probably needs to use TYPE_UID.

Or we might make default_hash_traits<tree> use DECL_UID for decls and 
TYPE_UID for types even if it doesn't do the more complex analysis of 
tree_operand_hash.

Jason

Patch
diff mbox series

--- gcc/cp/constraint.cc.jj	2019-11-07 09:50:51.755239234 +0100
+++ gcc/cp/constraint.cc	2019-11-11 19:04:03.231862024 +0100
@@ -45,6 +45,7 @@  along with GCC; see the file COPYING3.
 #include "decl.h"
 #include "toplev.h"
 #include "type-utils.h"
+#include "tree-hash-traits.h"
 
 static tree satisfaction_value (tree t);
 
@@ -1113,7 +1114,10 @@  build_constraints (tree tr, tree dr)
 
 /* A mapping from declarations to constraint information.  */
 
-static GTY ((cache)) tree_cache_map *decl_constraints;
+struct decl_tree_cache_traits
+  : simple_cache_map_traits<tree_decl_hash, tree> { };
+typedef hash_map<tree,tree,decl_tree_cache_traits> decl_tree_cache_map;
+static GTY ((cache)) decl_tree_cache_map *decl_constraints;
 
 /* Returns the template constraints of declaration T. If T is not
    constrained, return NULL_TREE. Note that T must be non-null. */
--- gcc/testsuite/g++.dg/pch/pr92458.C.jj	2019-11-11 19:09:51.547614022 +0100
+++ gcc/testsuite/g++.dg/pch/pr92458.C	2019-11-11 19:12:39.164088578 +0100
@@ -0,0 +1,5 @@ 
+// PR c++/92458
+// { dg-options "-std=c++2a" }
+
+#include "pr92458.H"
+S<int> s;
--- gcc/testsuite/g++.dg/pch/pr92458.Hs.jj	2019-11-11 19:09:25.091012637 +0100
+++ gcc/testsuite/g++.dg/pch/pr92458.Hs	2019-11-11 19:12:47.626961060 +0100
@@ -0,0 +1,7 @@ 
+// PR c++/92458
+// { dg-options "-std=c++2a" }
+
+template<typename T> concept C = sizeof(T) > 1;
+template<typename T> struct S { };
+template<typename T> requires C<T> struct S<T> { };
+template<typename T> requires (!C<T>) struct S<T> { };