erichkeane added a comment. 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. 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
