[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

Fri Oct 28 12:17:02 PDT 2022

erichkeane added a comment.

In D133052#3892650 <https://reviews.llvm.org/D133052#3892650>, @erichkeane wrote:

> In D133052#3891768 <https://reviews.llvm.org/D133052#3891768>, @usaxena95 wrote:
>
>> My suggestion would be to properly handle cycles in `CheckConstraintSatisfaction`. This problem goes beyond cycles introduced by conversion. See #53213 <https://github.com/llvm/llvm-project/issues/53213>, #44304 <https://github.com/llvm/llvm-project/issues/44304> and #45736 <https://github.com/llvm/llvm-project/issues/45736> https://godbolt.org/z/hzM6f3qnW. Most of the bugs are due to an assertion failure while inserting entry into SatisfactionCache.
>>
>> IIUC, failing on cycles would solve all of these issues.
>>
>> My suggestion would be detect cycles in `CheckConstraintSatisfaction`. We already have a way to check "whether we have seen this evaluation before ?" in `SatisfactionCache` using `FoldingSet` for `ConstraintSatisfaction`. We could use a similar set to track the constraints being evaluated. Stop evaluation when we detect a cycle. Attach appropriate details to the Satisfaction and fail the constraint.
>
> We cant use the SatisfactionCache 'just yet', we have to have a different stack, but we should fail there. We should NOT fail the constraint, it needs to be a hard-error based on discussion on the reflector. I'm intending to commit a patch to that effect today/monday.
>
> In D133052#3892527 <https://reviews.llvm.org/D133052#3892527>, @usaxena95 wrote:
>
>> Coincidentally 2 of the bugs were just fixed in b9a77b56d8 <https://github.com/llvm/llvm-project/commit/b9a77b56d83ac788beb7b1743e510ef8534354ca>. I do not completely agree with the approach there though. We should be fixing the root cause instead of circumventing the assert in `InsertNode` by insert-if-not-present. This is why we have 44304 and 50891 still persistent.
>
> So the problem that I solved was forming a recovery expression during evaluation, this wasn't a 'circumventing' the assert, this was making sure we dont try to double-cache a legitimate recursive-lookup.

Interestingly, just doing it in CheckConstraintSatisfaction (~SemaConcept.cpp:385) doesn't seem right, we end up catching the failure 'too early', since a FunctionDecl itself goes through there and has its constraints checked.  I suspect we actually need to check at the constraint-expr level instead (so down a few levels).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133052/new/

https://reviews.llvm.org/D133052