[PATCH] c++: error-recovery ICE with unstable satisfaction [PR109752]

[Patrick Palka]

On Tue, 9 May 2023, Patrick Palka wrote:

> After diagnosing and recovering from unstable satisfaction, it's
> possible to evaluate an atom for the first time noisily rather than
> quietly.  The satisfaction cache tries to handle this situation
> gracefully, but apparently not gracefully enough: we inserted an empty
> slot in the cache, and left it empty, which later makes
> hash_table::check_complete_insertion unhappy.  This patch fixes this by
> removing the empty slot in this case.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?
> 
> 	PR c++/109752
> 
> gcc/cp/ChangeLog:
> 
> 	* constraint.cc (satisfaction_cache::satisfaction_cache): In the
> 	unexpected case of evaluating an atom for the first time noisily,
> 	clear the cache slot that we inserted.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp2a/concepts-pr109752.C: New test.
> ---
>  gcc/cp/constraint.cc                          | 14 +++++++---
>  .../g++.dg/cpp2a/concepts-pr109752.C          | 26 +++++++++++++++++++
>  2 files changed, 37 insertions(+), 3 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-pr109752.C
> 
> diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
> index 675299aa4cd..bdaa82c8741 100644
> --- a/gcc/cp/constraint.cc
> +++ b/gcc/cp/constraint.cc
> @@ -2687,9 +2687,17 @@ satisfaction_cache
>        *slot = entry;
>      }
>    else
> -    /* We shouldn't get here, but if we do, let's just leave 'entry'
> -       empty, effectively disabling the cache.  */
> -    return;
> +    {
> +      /* We're evaluating this atom for the first time, and doing so noisily.
> +	 This shouldn't happen outside of error recovery situations involving
> +	 unstable satisfaction.  Let's just leave 'entry' empty, effectively
> +	 disabling the cache, and remove the empty slot.  */
> +      gcc_checking_assert (seen_error ());
> +      /* To appease hash_table::check_complete_insertion.  */
> +      *slot = ggc_alloc<sat_entry> ();
> +      sat_cache->clear_slot (slot);
> +      return;

Whoops, this 'return;' is rather unnecessary, so consider it removed.

> +    }
>  }
>  
>  /* Returns the cached satisfaction result if we have one and we're not
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-pr109752.C b/gcc/testsuite/g++.dg/cpp2a/concepts-pr109752.C
> new file mode 100644
> index 00000000000..d54ce295e50
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-pr109752.C
> @@ -0,0 +1,26 @@
> +// PR c++/109752
> +// { dg-do compile { target c++20 } }
> +
> +template <typename _Tp, typename... _Args>
> +  inline constexpr bool is_constructible_v = __is_constructible(_Tp, _Args...);
> +    template<typename _Tp, typename _Up>
> +      concept __weakly_eq_cmp_with
> + = requires(_Tp __t, _Up __u) {    { __u != __t } ; // { dg-error "changed from" }
> + };
> +  template<typename _Tp>
> +    concept regular =  is_constructible_v<_Tp>  && __weakly_eq_cmp_with<_Tp, _Tp>;
> +  template<typename _Iter> concept incrementable = true
> +&& regular<_Iter>
> +&& requires(_Iter __i) { { __i++ } ;}
> +;
> +template<typename D>
> +struct iterator_interface
> +{
> +  friend constexpr bool operator>=(D lhs, D rhs) requires __weakly_eq_cmp_with<D, D> { return true; }
> +};
> +template<typename T>
> +struct iterator : iterator_interface<iterator<T>>
> +{
> +    bool operator==(iterator) const;
> +};
> +static_assert(incrementable<iterator<int>>); // { dg-error "assert" }
> -- 
> 2.40.1.476.g69c786637d
> 
>