[clang] Fix crash when handling nested immediate invocations

[Fznamznon]

Summary

Before this patch it was expected that if there was several immediate
invocations they all belong to the same expression evaluation context.
During parsing of non local variable initializer a new evaluation context is
pushed, so code like this

namespace scope {
struct channel {
    consteval channel(const char* name) noexcept { }
};
consteval const char* make_channel_name(const char* name) { return name;}

channel rsx_log(make_channel_name("rsx_log"));
}

produced a nested immediate invocation whose subexpressions are attached
to different expression evaluation contexts. The constructor call
belongs to TU context and make_channel_name call to context of
variable initializer.

This patch removes this assumption and adds tracking of previously
failed immediate invocations, so it is possible when handling an
immediate invocation th check that its subexpressions from possibly another
evaluation context contains errors and not produce duplicate
diagnostics.

Fixes https://github.com/llvm/llvm-project/issues/58207

[shafik]

Thank you for the fix.

clang/lib/Sema/SemaExpr.cpp

17897

It is not obvious to me why this is correct, can you please explain.

[Fznamznon]

clang/lib/Sema/SemaExpr.cpp

17897

The visitor which this function is a part of traverses all subexpressions of a given ImmediateInvocationCandidate, it is saved to CurrentII member. Each ImmediateIvocationCandidate is a pair of integer and a pointer to ConstantExpr. The non-zero integer indicates that we should not try to evaluate particular immediate invocation. So, this patch, once found out that current expression contains a subexpression from another evaluation context whose evaluation failed before, marks that we shouldn't evaluate it to avoid double diagnosing. Otherwise, if some subexpression was found in the same context that current ImmediateInvocationCandidate belongs to, mark that we should not evaluate it instead, again to avoid double diagnosing. Hope that helps.

[Fznamznon]

Ping?

[cor3ntin]

Sorry for the late review.
I think this makes sense generally, i just have one question

clang/lib/Sema/SemaExpr.cpp

17979

Shouln't we clear FailedImmediateInvocations at the end of a full expression to avoid going there if there was an error in another expression

[Fznamznon]

clang/lib/Sema/SemaExpr.cpp

17979

The idea sounds reasonable, but I'm not sure it can work with current algorithm of handling immediate invocations. Immediate invocations are handled when expression evaluation context is popped, so we need to remember previously failed immediate invocations until then, even though they could happen in two different full expressions.

consteval foo() { ... }
void bar() {
  int a = foo(/* there might be a failed immediate invocation attached to initializer context */); // this is a full-expression
  int b = foo(); // this is another full-expression
} // This is where immediate invocations that appear in function context are processed

[aaron.ballman]

I think this LGTM, but I'm holding off on signing off until @shafik is satisfied with the part he was asking about. You should add a release note for the fix, too.

clang/lib/Sema/SemaExpr.cpp

17891–17894

Minor grammar nits.

17973

17979

[Fznamznon]

Rebase, add release note, fix grammar

[Fznamznon]

Fix formatting

[Fznamznon]

Ping @shafik ?

[shafik]

LGTM

[Fznamznon]

@aaron.ballman , @cor3ntin are you ok with the patch?

[aaron.ballman]

LGTM!

clang/include/clang/Sema/Sema.h

bool isDiscardedStatementContext() const {
                   ExpressionEvaluationContext::ImmediateFunctionContext &&
               InDiscardedStatement);
     }
   };
 
   /// A stack of expression evaluation contexts.
   SmallVector<ExpressionEvaluationContextRecord, 8> ExprEvalContexts;
 
+  // Set of failed immediate invocations to avoid double diagnosing.
+  llvm::SmallPtrSet<ConstantExpr*, 4> FailedImmediateInvocations;
+
   /// Emit a warning for all pending noderef expressions that we recorded.
   void WarnOnPendingNoDerefs(ExpressionEvaluationContextRecord &Rec);
 
   /// Compute the mangling number context for a lambda expression or
   /// block literal. Also return the extra mangling decl if any.
   ///
   /// \param DC - The DeclContext containing the lambda expression or
   /// block literal.

clang/lib/Sema/SemaExpr.cpp

static void EvaluateAndDiagnoseImmediateInvocation(
     Sema &SemaRef, Sema::ImmediateInvocationCandidate Candidate) {
   llvm::SmallVector<PartialDiagnosticAt, 8> Notes;
   Expr::EvalResult Eval;
   Eval.Diag = &Notes;
   ConstantExpr *CE = Candidate.getPointer();
   bool Result = CE->EvaluateAsConstantExpr(
       Eval, SemaRef.getASTContext(), ConstantExprKind::ImmediateInvocation);
   if (!Result || !Notes.empty()) {
+    SemaRef.FailedImmediateInvocations.insert(CE);
     Expr *InnerExpr = CE->getSubExpr()->IgnoreImplicit();
     if (auto *FunctionalCast = dyn_cast<CXXFunctionalCastExpr>(InnerExpr))
       InnerExpr = FunctionalCast->getSubExpr();
     FunctionDecl *FD = nullptr;
     if (auto *Call = dyn_cast<CallExpr>(InnerExpr))
       FD = cast<FunctionDecl>(Call->getCalleeDecl());
     else if (auto *Call = dyn_cast<CXXConstructExpr>(InnerExpr))
       FD = Call->getConstructor();
ComplexRemove(Sema &SemaRef, llvm::SmallPtrSetImpl<DeclRefExpr *> &DR,
                   SmallVector<Sema::ImmediateInvocationCandidate,
                               4>::reverse_iterator Current)
         : Base(SemaRef), DRSet(DR), IISet(II), CurrentII(Current) {}
     void RemoveImmediateInvocation(ConstantExpr* E) {
       auto It = std::find_if(CurrentII, IISet.rend(),
                              [E](Sema::ImmediateInvocationCandidate Elem) {
                                return Elem.getPointer() == E;
                              });
-      assert(It != IISet.rend() &&
-             "ConstantExpr marked IsImmediateInvocation should "
-             "be present");
+      // It is possible that some subexpression of current immediate invocation
+      // was handled from another expression evaluation context. Do not handle
+      // current immediate invocation if some of its subexpressions failed
+      // before.
+      if (It == IISet.rend()) {
+        if (SemaRef.FailedImmediateInvocations.contains(E))
+          CurrentII->setInt(1);
+      } else {
         It->setInt(1); // Mark as deleted
       }
+    }
     ExprResult TransformConstantExpr(ConstantExpr *E) {
       if (!E->isImmediateInvocation())
         return Base::TransformConstantExpr(E);
       RemoveImmediateInvocation(E);
       return Base::TransformExpr(E->getSubExpr());
     }
     /// Base::TransfromCXXOperatorCallExpr doesn't traverse the callee so
     /// we need to remove its DeclRefExpr from the DRSet.

 static void
 HandleImmediateInvocations(Sema &SemaRef,
                            Sema::ExpressionEvaluationContextRecord &Rec) {
   if ((Rec.ImmediateInvocationCandidates.size() == 0 &&
        Rec.ReferenceToConsteval.size() == 0) ||
       SemaRef.RebuildingImmediateInvocation)
     return;
 
-  /// When we have more then 1 ImmediateInvocationCandidates we need to check
-  /// for nested ImmediateInvocationCandidates. when we have only 1 we only
-  /// need to remove ReferenceToConsteval in the immediate invocation.
-  if (Rec.ImmediateInvocationCandidates.size() > 1) {
+  /// When we have more then 1 ImmediateInvocationCandidates or previously
+  /// failed immediate invocations, we need to check for nested
+  /// ImmediateInvocationCandidates in order to avoid duplicate diagnostics.
+  /// Otherwise we only need to remove ReferenceToConsteval in the immediate
+  /// invocation.
+  if (Rec.ImmediateInvocationCandidates.size() > 1 ||
+      SemaRef.FailedImmediateInvocations.size()) {
 
     /// Prevent sema calls during the tree transform from adding pointers that
     /// are already in the sets.
     llvm::SaveAndRestore DisableIITracking(
         SemaRef.RebuildingImmediateInvocation, true);
 
     /// Prevent diagnostic during tree transfrom as they are duplicates
     Sema::TentativeAnalysisScope DisableDiag(SemaRef);

clang/test/SemaCXX/cxx2a-consteval.cpp

constexpr auto lstatic = []() static consteval { // expected-error {{cannot take address of consteval call operator of '(lambda at}} \
                                        // expected-warning {{extension}}
         return 3;
   };
   constexpr int (*f2)(void) = lstatic; // expected-error {{constexpr variable 'f2' must be initialized by a constant expression}} \
                                        // expected-note  {{pointer to a consteval declaration is not a constant expression}}
 
 }
 }
+
+namespace GH58207 {
+struct tester {
+    consteval tester(const char* name) noexcept { }
+};
+consteval const char* make_name(const char* name) { return name;}
+consteval const char* pad(int P) { return "thestring"; }
+
+int bad = 10; // expected-note 6{{declared here}}
+
+tester glob1(make_name("glob1"));
+tester glob2(make_name("glob2"));
+constexpr tester cglob(make_name("cglob"));
+tester paddedglob(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+                                        // expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+constexpr tester glob3 = { make_name("glob3") };
+constexpr tester glob4 = { make_name(pad(bad)) }; // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+                                                  // expected-error {{constexpr variable 'glob4' must be initialized by a constant expression}} \
+                                                  // expected-note 2{{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+auto V = make_name(pad(3));
+auto V1 = make_name(pad(bad)); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+                               // expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+
+void foo() {
+  static tester loc1(make_name("loc1"));
+  static constexpr tester loc2(make_name("loc2"));
+  static tester paddedloc(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+                                                // expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+}
+
+void bar() {
+  static tester paddedloc(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+                                                // expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+}
+}