[PATCH] D127487: [Sema] Fix assertion failure when instantiating requires expression

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D127487#3573667 <https://reviews.llvm.org/D127487#3573667>, @ilya-biryukov wrote:

> I've checked that the change works fine on top of D126907 <https://reviews.llvm.org/D126907>. The bug is still there after D126907 <https://reviews.llvm.org/D126907> and gets fixed by this.
> Also, the merge conflict is actually minimal, no code changes intersect.

Thanks for looking into this!  I DID see it was still in place after the fact, but wanted to make sure this wouldn't break any more of the tests.



================
Comment at: clang/lib/Sema/SemaConcept.cpp:352
+      [this](const Expr *AtomicExpr) -> ExprResult {
+        // We only do this to immitate lvalue-to-rvalue conversion.
+        return PerformContextuallyConvertToBool(const_cast<Expr*>(AtomicExpr));
----------------
ilya-biryukov wrote:
> royjacobson wrote:
> > erichkeane wrote:
> > > ilya-biryukov wrote:
> > > > erichkeane wrote:
> > > > > Can you explain this more?  How does this work, and why don't we do that directly instead?
> > > > That's entangled with `calculateConstraintSatisfaction`. I actually tried to do it directly, but before passing expressions to this function `calculateConstraintSatisfaction` calls `IgnoreParenImpCasts()`, which strips away the lvalue-to-rvalue conversion.
> > > > And we need this conversion so that the evaluation that runs after this callback returns actually produces an r-value.
> > > > 
> > > > Note that the other call to `calculateConstraintSatisfaction` also calls `PerformContextuallyConvertToBool` after doing template substitution into the constraint expression.
> > > > 
> > > > I don't have full context on why it's the way it is, maybe there is a more fundamental change that helps with both cases.
> > > Hmm... my understanding is we DO need these to be a boolean expression eventually, since we have to test them as a bool, so that is why the other attempts the converesion.  If you think of any generalizations of this, it would be appreciated, I'll think it through as well.
> > Note we already have a related bug about this https://github.com/llvm/llvm-project/issues/54524
> Yeah, they have to be bool and we actually check for that in `CheckConstraintExpression`. The standard explicitly mentions only the lvalue-to-rvalue conversion should be performed.
> ```
> [temp.constr.atomic]p3 If substitution results in an invalid type or expression, the constraint is
> not satisfied. Otherwise, the lvalue-to-rvalue conversion (7.3.1) is performed if necessary, and E shall be a constant expression of type bool.
> ```
> 
> However, in the calls to `calculateConstraintSatisfaction` we do a more generic boolean conversion, but the comment in the other call site suggests this probably accidental and we actually want a less generic conversion:
> ```
>           // Substitution might have stripped off a contextual conversion to
>           // bool if this is the operand of an '&&' or '||'. For example, we
>           // might lose an lvalue-to-rvalue conversion here. If so, put it back
>           // before we try to evaluate.
>           if (!SubstitutedExpression.isInvalid())
>             SubstitutedExpression =
>                 S.PerformContextuallyConvertToBool(SubstitutedExpression.get());
> ```
> 
> I am happy to take a look at fixing the mentioned bug.
Hmm... yeah, I find myself wondering if there is a better lval/rval conversion function here, and I'm guessing the contextually convert to bool is the wrong one.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2042
+        !SemaRef.CheckConstraintExpression(TransConstraint.get())) {
+      assert(Trap.hasErrorOccurred() && "CheckConstraintExpression failed, but "
+                                        "did not produce a SFINAE error");
----------------
ilya-biryukov wrote:
> erichkeane wrote:
> > ilya-biryukov wrote:
> > > erichkeane wrote:
> > > > This branch ends up being empty if asserts are off.  Also, it results in CheckConstraintExpression happening 2x, which ends up being more expensive after https://reviews.llvm.org/D126907
> > > > This branch ends up being empty if asserts are off.  Also, it results in CheckConstraintExpression happening 2x, which ends up being more expensive after https://reviews.llvm.org/D126907
> > > 
> > > Yeah, good point, I have update it.
> > > 
> > > I am not sure why would `CheckConstraintExpression` be called twice, could you elaborate? Note that we do not call `BuildNestedRequirement` anymore and use placement new directly to avoid extra template instantiations. Instead we call `CheckConstraintExpression` directly to account for any errors.
> > This check does not seem to cause a 'return' to the function, but then falls through to the version on 2052, right?  
> > 
> > `CheckConstraintExpression`/`CheckConstraintSatisfaction`(i think?) ends up now having to re-instantiate every time it is called, so any ability to cache results ends up being beneficial here.
> The number of calls to these functions is actually the same.
> 
> `CheckConstraintExpression` used to be called during `CheckConstraintSatisfaction` (that does instantiations) for every atomic constraint after the substitution. It merely checks that each constraint have a bool type and does not do any substitutions, so it's pretty cheap anyway.
> 
> `CheckConstraintSatisfaction` was called inside `BuildNestedRequirement`, we now call a different overload here directly that does not do any extra template instantiations directly.
> 
> That way we end up doing the same checks without running recursive template instantiations.
Hmm... I guess what I'm saying is: I would like it if we could minimize/reduce the calls to calcuateConstraintSatisfaction (in SemaConcept.cpp) that does the call to SubstConstraintExpr (or substExpr).  

That ends up being more expensive thanks to my other patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127487