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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
