c++: spec_hasher and TYPENAME_TYPE resolution [PR95223]

After enabling sanitization of the specialization tables, we are
triggering one of the hash table sanity checks in the below testcase.

The reason is that when looking up the specialization j<int> in the
type_specializations table, the sanity check finds that the existing
entry j<n<t>::m> compares equal to j<int> but hashes differently.

The discrepancy is due to structural_comptypes looking through
TYPENAME_TYPEs (via resolve_typename_type), something which
iterative_hash_template_arg doesn't do.  So the TYPENAME_TYPE n<t>::m is
considered equal to int, but the hashes of these template arguments are
different.

It seems wrong for the result of a specialization table lookup to depend
on the current scope, so this patch makes structural_comptypes avoid
calling resolve_typename_type when comparing_specializations.

In order for the below testcase to deterministically trigger the
sanitization error without this patch, we also need to fix the location
of the call to hash_table::verify within hash_table::find_with_hash.

Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK to
commit?

gcc/ChangeLog:

	PR c++/95223
	* hash-table.h (hash_table::find_with_hash): Move up the call to
	hash_table::verify.

gcc/cp/ChangeLog:

	PR c++/95223
	* gcc/cp/typeck.c (structural_comptypes): Don't perform
	context-dependent resolution of TYPENAME_TYPEs when
	comparing_specializations.

gcc/testsuite/ChangeLog:

	PR c++/95223
	* g++.dg/template/typename23.C: New test.
---
 gcc/cp/typeck.c                            | 15 +++++++++------
 gcc/hash-table.h                           | 14 +++++++-------
 gcc/testsuite/g++.dg/template/typename23.C | 10 ++++++++++
 3 files changed, 26 insertions(+), 13 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/template/typename23.C

On 5/19/20 11:38 PM, Patrick Palka via Gcc-patches wrote:
> After enabling sanitization of the specialization tables, we are
> triggering one of the hash table sanity checks in the below testcase.
> 
> The reason is that when looking up the specialization j<int> in the
> type_specializations table, the sanity check finds that the existing
> entry j<n<t>::m> compares equal to j<int> but hashes differently.
> 
> The discrepancy is due to structural_comptypes looking through
> TYPENAME_TYPEs (via resolve_typename_type), something which
> iterative_hash_template_arg doesn't do.  So the TYPENAME_TYPE n<t>::m is
> considered equal to int, but the hashes of these template arguments are
> different.
> 
> It seems wrong for the result of a specialization table lookup to depend
> on the current scope, so this patch makes structural_comptypes avoid
> calling resolve_typename_type when comparing_specializations.
> 
> In order for the below testcase to deterministically trigger the
> sanitization error without this patch, we also need to fix the location
> of the call to hash_table::verify within hash_table::find_with_hash.

Yes please!  I have this problem on the modules branch, but didn't 
notice I can press comparing_specializations into service, so added a 
module-specific global just there.

> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK to
> commit?
> 
> gcc/ChangeLog:
> 
> 	PR c++/95223
> 	* hash-table.h (hash_table::find_with_hash): Move up the call to
> 	hash_table::verify.
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/95223
> 	* gcc/cp/typeck.c (structural_comptypes): Don't perform
> 	context-dependent resolution of TYPENAME_TYPEs when
> 	comparing_specializations.

ok.

nathan

On 5/19/20 11:38 PM, Patrick Palka wrote:
> After enabling sanitization of the specialization tables, we are
> triggering one of the hash table sanity checks in the below testcase.
> 
> The reason is that when looking up the specialization j<int> in the
> type_specializations table, the sanity check finds that the existing
> entry j<n<t>::m> compares equal to j<int> but hashes differently.
> 
> The discrepancy is due to structural_comptypes looking through
> TYPENAME_TYPEs (via resolve_typename_type), something which
> iterative_hash_template_arg doesn't do.  So the TYPENAME_TYPE n<t>::m is
> considered equal to int, but the hashes of these template arguments are
> different.
> 
> It seems wrong for the result of a specialization table lookup to depend
> on the current scope, so this patch makes structural_comptypes avoid
> calling resolve_typename_type when comparing_specializations.
> 
> In order for the below testcase to deterministically trigger the
> sanitization error without this patch, we also need to fix the location
> of the call to hash_table::verify within hash_table::find_with_hash.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK to
> commit?

OK.

> gcc/ChangeLog:
> 
> 	PR c++/95223
> 	* hash-table.h (hash_table::find_with_hash): Move up the call to
> 	hash_table::verify.
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/95223
> 	* gcc/cp/typeck.c (structural_comptypes): Don't perform
> 	context-dependent resolution of TYPENAME_TYPEs when
> 	comparing_specializations.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/95223
> 	* g++.dg/template/typename23.C: New test.
> ---
>   gcc/cp/typeck.c                            | 15 +++++++++------
>   gcc/hash-table.h                           | 14 +++++++-------
>   gcc/testsuite/g++.dg/template/typename23.C | 10 ++++++++++
>   3 files changed, 26 insertions(+), 13 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/template/typename23.C
> 
> diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
> index d2e6c907622..0181984bb99 100644
> --- a/gcc/cp/typeck.c
> +++ b/gcc/cp/typeck.c
> @@ -1256,13 +1256,16 @@ structural_comptypes (tree t1, tree t2, int strict)
>   
>     gcc_assert (TYPE_P (t1) && TYPE_P (t2));
>   
> -  /* TYPENAME_TYPEs should be resolved if the qualifying scope is the
> -     current instantiation.  */
> -  if (TREE_CODE (t1) == TYPENAME_TYPE)
> -    t1 = resolve_typename_type (t1, /*only_current_p=*/true);
> +  if (!comparing_specializations)
> +    {
> +      /* TYPENAME_TYPEs should be resolved if the qualifying scope is the
> +	 current instantiation.  */
> +      if (TREE_CODE (t1) == TYPENAME_TYPE)
> +	t1 = resolve_typename_type (t1, /*only_current_p=*/true);
>   
> -  if (TREE_CODE (t2) == TYPENAME_TYPE)
> -    t2 = resolve_typename_type (t2, /*only_current_p=*/true);
> +      if (TREE_CODE (t2) == TYPENAME_TYPE)
> +	t2 = resolve_typename_type (t2, /*only_current_p=*/true);
> +    }
>   
>     if (TYPE_PTRMEMFUNC_P (t1))
>       t1 = TYPE_PTRMEMFUNC_FN_TYPE (t1);
> diff --git a/gcc/hash-table.h b/gcc/hash-table.h
> index a1423c78112..32f3a634e1e 100644
> --- a/gcc/hash-table.h
> +++ b/gcc/hash-table.h
> @@ -912,6 +912,12 @@ hash_table<Descriptor, Lazy, Allocator>
>   
>     if (Lazy && m_entries == NULL)
>       m_entries = alloc_entries (size);
> +
> +#if CHECKING_P
> +  if (m_sanitize_eq_and_hash)
> +    verify (comparable, hash);
> +#endif
> +
>     value_type *entry = &m_entries[index];
>     if (is_empty (*entry)
>         || (!is_deleted (*entry) && Descriptor::equal (*entry, comparable)))
> @@ -928,13 +934,7 @@ hash_table<Descriptor, Lazy, Allocator>
>         entry = &m_entries[index];
>         if (is_empty (*entry)
>             || (!is_deleted (*entry) && Descriptor::equal (*entry, comparable)))
> -	{
> -#if CHECKING_P
> -	  if (m_sanitize_eq_and_hash)
> -	    verify (comparable, hash);
> -#endif
> -	  return *entry;
> -	}
> +	return *entry;
>       }
>   }
>   
> diff --git a/gcc/testsuite/g++.dg/template/typename23.C b/gcc/testsuite/g++.dg/template/typename23.C
> new file mode 100644
> index 00000000000..d2fb0ca72f5
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/typename23.C
> @@ -0,0 +1,10 @@
> +// PR c++/95223
> +// { dg-do compile }
> +// { dg-additional-options "--param=hash-table-verification-limit=10000" }
> +
> +template <typename> struct j {};
> +template <typename t> struct n {
> +  typedef int m;
> +  j<n<t>::m> p();
> +};
> +template <typename o> j<typename n<o>::m> n<o>::p() { return o::f(); }
>