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

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

[Patrick Palka]

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;
+    }
 }
 
 /* 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

Re: [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
> 
> 

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

[Jason Merrill]

On 5/9/23 12:35, 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?

OK.

> 	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;
> +    }
>   }
>   
>   /* 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" }