[clang][ExprConst] Use call source range for 'in call to' diags

[tbaeder]

Summary

Before:

array.cpp:74:15: error: static assertion expression is not an integral constant expression
   74 | static_assert(ints(1, div(true, false), 2, div(false, true)) == 1, "");
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
array.cpp:68:12: note: division by zero
   68 |   return 1 / (int)b;
      |            ^
array.cpp:74:23: note: in call to 'div(true, false)'
   74 | static_assert(ints(1, div(true, false), 2, div(false, true)) == 1, "");
      |                       ^
1 error generated.

After:

array.cpp:74:15: error: static assertion expression is not an integral constant expression
   74 | static_assert(ints(1, div(true, false), 2, div(false, true)) == 1, "");
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
array.cpp:68:12: note: division by zero
   68 |   return 1 / (int)b;
      |            ^
array.cpp:74:23: note: in call to 'div(true, false)'
   74 | static_assert(ints(1, div(true, false), 2, div(false, true)) == 1, "");
      |                       ^~~~~~~~~~~~~~~~

As you can see I have a test case, but I didn't attach it in test/Misc/constexpr-source-ranges.cpp because I can't get -fdiagnostics-print-source-range-info to actually print anything for them.

[aaron.ballman]

As you can see I have a test case, but I didn't attach it in test/Misc/constexpr-source-ranges.cpp because I can't get -fdiagnostics-print-source-range-info to actually print anything for them.

That's odd; have you investigated what's going on/filed an issue?

The changes LG, but the lack of test coverage gives me pause.

[tbaeder]

In D156604#4546650, @aaron.ballman wrote:

As you can see I have a test case, but I didn't attach it in test/Misc/constexpr-source-ranges.cpp because I can't get -fdiagnostics-print-source-range-info to actually print anything for them.

That's odd; have you investigated what's going on/filed an issue?

I have thoroughly investigated the aforementioned issue and found the cause: I am blind.

[tbaeder]

@hazohelet I changed this to just return CallExpr->getSourceRange() and to not save a source range separately. Can you give this a look please?

[hazohelet]

In D156604#4549524, @tbaeder wrote:

@hazohelet I changed this to just return CallExpr->getSourceRange() and to not save a source range separately. Can you give this a look please?

I was thinking about removing CallLoc some time ago, and concluded that we shouldn't.
Destructor calls are usually not explicitly written by the user, so CallExpr is set to null. CallLoc is often set to the declaration location (https://github.com/llvm/llvm-project/blob/97cddb78502eee583b5f4ee02c59b7156398587f/clang/lib/AST/ExprConstant.cpp#L703-L708).
So we'll lose this information if we are to remove CallLoc from the stack frame, and it seems to be the reason for the test failures.

About getting source range from CallExpr->getSourceRange, I don't see any obvious problems. All explicitly-written function calls should have its AST in CallExpr field.
So using the CallLoc for the diagnostics location and CallExpr->getSourceRange for the source range would do the trick.

[tbaeder]

Ping

[aaron.ballman]

LGTM but please give a day or two for @hazohelet to weigh in.

[hazohelet]

We need some tests for dtors because they are handled differently from other functions.
I think the current ExprConstant part would not cover the explicitly-called dtors because the HandleDestructorImpl only has access to CallLoc and not source range.
Also new interpreter seems to point to the wrong source location on implicitly-called dtor.
Link: https://godbolt.org/z/1a6GW47eG
I think it's good to have a fix for it in this patch if it's not too complicated.

My previous comment "All explicitly-written function calls should have its AST in CallExpr field." was wrong for destructors. Sorry for the confusion.

[tbaeder]

In D156604#4572018, @hazohelet wrote:

We need some tests for dtors because they are handled differently from other functions.
I think the current ExprConstant part would not cover the explicitly-called dtors because the HandleDestructorImpl only has access to CallLoc and not source range.
Also new interpreter seems to point to the wrong source location on implicitly-called dtor.
Link: https://godbolt.org/z/1a6GW47eG
I think it's good to have a fix for it in this patch if it's not too complicated.

Yeah I know, there's a FIXME comment in the tests somewhere and it's on my list.

[hazohelet]

I think it would be helpful to point to the subobject source range when diagnosing errors in subobject dtors, so I left some suggestions.
Otherwise this LGTM. Thanks!

clang/lib/AST/ExprConstant.cpp

6653

6672

It's not the problem of this patch, but it might be nice to have CXXBaseSpecifier::getTypeSourceRange or alike because this is an interesting source range.

[tbaeder]

While both of those suggestions are probably improvements (I haven't checked), they also seem out of scope for this patch. This is just adding source ranges. We can improve them later (and add better tests for them at that point).

[hazohelet]

In D156604#4572994, @tbaeder wrote:

While both of those suggestions are probably improvements (I haven't checked), they also seem out of scope for this patch. This is just adding source ranges. We can improve them later (and add better tests for them at that point).

That makes sense to me.

clang/lib/AST/ExprConstant.cpp

public:
 
     // Note that we intentionally use std::map here so that references to
     // values are stable.
     typedef std::pair<const void *, unsigned> MapKeyTy;
     typedef std::map<MapKeyTy, APValue> MapTy;
     /// Temporaries - Temporary lvalues materialized within this stack frame.
     MapTy Temporaries;
 
-    /// CallLoc - The location of the call expression for this call.
-    SourceLocation CallLoc;
+    /// CallRange - The source range of the call expression for this call.
+    SourceRange CallRange;
 
     /// Index - The call index of this call.
     unsigned Index;
 
     /// The stack of integers for tracking version numbers for temporaries.
     SmallVector<unsigned, 2> TempVersionStack = {1};
     unsigned CurTempVersion = TempVersionStack.back();
 
public:
     // But let's try this and see how it goes; we can look into caching the map
     // as a later change.
 
     /// LambdaCaptureFields - Mapping from captured variables/this to
     /// corresponding data members in the closure class.
     llvm::DenseMap<const ValueDecl *, FieldDecl *> LambdaCaptureFields;
     FieldDecl *LambdaThisCaptureField = nullptr;
 
-    CallStackFrame(EvalInfo &Info, SourceLocation CallLoc,
+    CallStackFrame(EvalInfo &Info, SourceRange CallRange,
                    const FunctionDecl *Callee, const LValue *This,
                    const Expr *CallExpr, CallRef Arguments);
     ~CallStackFrame();
 
     // Return the temporary for Key whose version number is Version.
     APValue *getTemporary(const void *Key, unsigned Version) {
       MapKeyTy KV(Key, Version);
       auto LB = Temporaries.lower_bound(KV);
APValue &createTemporary(const KeyT *Key, QualType T,
                              ScopeKind Scope, LValue &LV);
 
     /// Allocate storage for a parameter of a function call made in this frame.
     APValue &createParam(CallRef Args, const ParmVarDecl *PVD, LValue &LV);
 
     void describe(llvm::raw_ostream &OS) const override;
 
     Frame *getCaller() const override { return Caller; }
-    SourceLocation getCallLocation() const override { return CallLoc; }
+    SourceRange getCallRange() const override { return CallRange; }
     const FunctionDecl *getCallee() const override { return Callee; }
 
     bool isStdFunction() const {
       for (const DeclContext *DC = Callee; DC; DC = DC->getParent())
         if (DC->isStdNamespace())
           return true;
       return false;
     }
Info.CCEDiag(E, diag::note_constexpr_array_index)
       << N << /*array*/ 0
       << static_cast<unsigned>(getMostDerivedArraySize());
   else
     Info.CCEDiag(E, diag::note_constexpr_array_index)
       << N << /*non-array*/ 1;
   setInvalid();
 }
 
-CallStackFrame::CallStackFrame(EvalInfo &Info, SourceLocation CallLoc,
+CallStackFrame::CallStackFrame(EvalInfo &Info, SourceRange CallRange,
                                const FunctionDecl *Callee, const LValue *This,
                                const Expr *CallExpr, CallRef Call)
     : Info(Info), Caller(Info.CurrentCall), Callee(Callee), This(This),
-      CallExpr(CallExpr), Arguments(Call), CallLoc(CallLoc),
+      CallExpr(CallExpr), Arguments(Call), CallRange(CallRange),
       Index(Info.NextCallIndex++) {
   Info.CurrentCall = this;
   ++Info.CallStackDepth;
 }
 
 CallStackFrame::~CallStackFrame() {
   assert(Info.CurrentCall == this && "calls retired out of order");
   --Info.CallStackDepth;

 static bool HandleFunctionCall(SourceLocation CallLoc,
                                const FunctionDecl *Callee, const LValue *This,
                                const Expr *E, ArrayRef<const Expr *> Args,
                                CallRef Call, const Stmt *Body, EvalInfo &Info,
                                APValue &Result, const LValue *ResultSlot) {
   if (!Info.CheckCallLimit(CallLoc))
     return false;
 
-  CallStackFrame Frame(Info, CallLoc, Callee, This, E, Call);
+  CallStackFrame Frame(Info, E->getSourceRange(), Callee, This, E, Call);
 
   // For a trivial copy or move assignment, perform an APValue copy. This is
   // essential for unions, where the operations performed by the assignment
   // operator cannot be represented as statements.
   //
   // Skip this for non-union classes with no fields; in that case, the defaulted
   // copy/move does not actually read the object.
   const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(Callee);
if (RD->getNumVBases()) {
     Info.FFDiag(CallLoc, diag::note_constexpr_virtual_base) << RD;
     return false;
   }
 
   EvalInfo::EvaluatingConstructorRAII EvalObj(
       Info,
       ObjectUnderConstruction{This.getLValueBase(), This.Designator.Entries},
       RD->getNumBases());
-  CallStackFrame Frame(Info, CallLoc, Definition, &This, E, Call);
+  CallStackFrame Frame(Info, E->getSourceRange(), Definition, &This, E, Call);
 
   // FIXME: Creating an APValue just to hold a nonexistent return value is
   // wasteful.
   APValue RetVal;
   StmtResult Ret = {RetVal, nullptr};
 
   // If it's a delegating constructor, delegate.
   if (Definition->isDelegatingConstructor()) {
for (const FieldDecl *FD : llvm::reverse(Fields)) {
     if (FD->isUnnamedBitfield())
       continue;
 
     LValue Subobject = This;
     if (!HandleLValueMember(Info, &LocE, Subobject, FD, &Layout))
       return false;
 
     APValue *SubobjectValue = &Value.getStructField(FD->getFieldIndex());
     if (!HandleDestructionImpl(Info, CallLoc, Subobject, *SubobjectValue,
                                FD->getType()))
       return false;
   }
 
   if (BasesLeft != 0)
     EvalObj.startedDestroyingBases();
 
   // Destroy base classes in reverse order.
   for (const CXXBaseSpecifier &Base : llvm::reverse(RD->bases())) {
     --BasesLeft;
 
     QualType BaseType = Base.getType();
     LValue Subobject = This;
     if (!HandleLValueDirectBase(Info, &LocE, Subobject, RD,
                                 BaseType->getAsCXXRecordDecl(), &Layout))
       return false;
 
     APValue *SubobjectValue = &Value.getStructBase(BasesLeft);
     if (!HandleDestructionImpl(Info, CallLoc, Subobject, *SubobjectValue,
                                BaseType))
       return false;
   }
   assert(BasesLeft == 0 && "NumBases was wrong?");
 
   // The period of destruction ends now. The object is gone.
   Value = APValue();
   return true;
case Builtin::BI__builtin_is_constant_evaluated: {
     const auto *Callee = Info.CurrentCall->getCallee();
     if (Info.InConstantContext && !Info.CheckingPotentialConstantExpression &&
         (Info.CallStackDepth == 1 ||
          (Info.CallStackDepth == 2 && Callee->isInStdNamespace() &&
           Callee->getIdentifier() &&
           Callee->getIdentifier()->isStr("is_constant_evaluated")))) {
       // FIXME: Find a better way to avoid duplicated diagnostics.
       if (Info.EvalStatus.Diag)
-        Info.report((Info.CallStackDepth == 1) ? E->getExprLoc()
-                                               : Info.CurrentCall->CallLoc,
+        Info.report((Info.CallStackDepth == 1)
+                        ? E->getExprLoc()
+                        : Info.CurrentCall->getCallRange().getBegin(),
                     diag::warn_is_constant_evaluated_always_true_constexpr)
             << (Info.CallStackDepth == 1 ? "__builtin_is_constant_evaluated"
                                          : "std::is_constant_evaluated");
     }
 
     return Success(Info.InConstantContext, E);
   }
 

clang/lib/AST/Interp/Frame.h

public:
 
   /// Generates a human-readable description of the call site.
   virtual void describe(llvm::raw_ostream &OS) const = 0;
 
   /// Returns a pointer to the caller frame.
   virtual Frame *getCaller() const = 0;
 
   /// Returns the location of the call site.
-  virtual SourceLocation getCallLocation() const = 0;
+  virtual SourceRange getCallRange() const = 0;
 
   /// Returns the called function's declaration.
   virtual const FunctionDecl *getCallee() const = 0;
 };
 
 } // namespace interp
 } // namespace clang
 
 #endif

clang/lib/AST/Interp/InterpFrame.h

public:
 
   /// Describes the frame with arguments for diagnostic purposes.
   void describe(llvm::raw_ostream &OS) const override;
 
   /// Returns the parent frame object.
   Frame *getCaller() const override;
 
   /// Returns the location of the call to the frame.
-  SourceLocation getCallLocation() const override;
+  SourceRange getCallRange() const override;
 
   /// Returns the caller.
   const FunctionDecl *getCallee() const override;
 
   /// Returns the current function.
   const Function *getFunction() const { return Func; }
 
   /// Returns the offset on the stack at which the frame starts.

clang/lib/AST/Interp/InterpFrame.cpp

 }
 
 Frame *InterpFrame::getCaller() const {
   if (Caller->Caller)
     return Caller;
   return S.getSplitFrame();
 }
 
-SourceLocation InterpFrame::getCallLocation() const {
+SourceRange InterpFrame::getCallRange() const {
   if (!Caller->Func)
-    return S.getLocation(nullptr, {});
-  return S.getLocation(Caller->Func, RetPC - sizeof(uintptr_t));
+    return S.getRange(nullptr, {});
+  return S.getRange(Caller->Func, RetPC - sizeof(uintptr_t));
 }
 
 const FunctionDecl *InterpFrame::getCallee() const {
   return Func->getDecl();
 }
 
 Pointer InterpFrame::getLocalPointer(unsigned Offset) const {
   assert(Offset < Func->getFrameSize() && "Invalid local offset.");

clang/lib/AST/Interp/State.cpp

if (Limit && Limit < ActiveCalls) {
     SkipEnd = ActiveCalls - Limit / 2;
   }
 
   // Walk the call stack and add the diagnostics.
   unsigned CallIdx = 0;
   const Frame *Top = getCurrentFrame();
   const Frame *Bottom = getBottomFrame();
   for (const Frame *F = Top; F != Bottom; F = F->getCaller(), ++CallIdx) {
-    SourceLocation CallLocation = F->getCallLocation();
+    SourceRange CallRange = F->getCallRange();
 
     // Skip this call?
     if (CallIdx >= SkipStart && CallIdx < SkipEnd) {
       if (CallIdx == SkipStart) {
         // Note that we're skipping calls.
-        addDiag(CallLocation, diag::note_constexpr_calls_suppressed)
+        addDiag(CallRange.getBegin(), diag::note_constexpr_calls_suppressed)
             << unsigned(ActiveCalls - Limit);
       }
       continue;
     }
 
     // Use a different note for an inheriting constructor, because from the
     // user's perspective it's not really a function at all.
     if (const auto *CD =
             dyn_cast_if_present<CXXConstructorDecl>(F->getCallee());
         CD && CD->isInheritingConstructor()) {
-      addDiag(CallLocation, diag::note_constexpr_inherited_ctor_call_here)
+      addDiag(CallRange.getBegin(),
+              diag::note_constexpr_inherited_ctor_call_here)
           << CD->getParent();
       continue;
     }
 
     SmallString<128> Buffer;
     llvm::raw_svector_ostream Out(Buffer);
     F->describe(Out);
-    addDiag(CallLocation, diag::note_constexpr_call_here) << Out.str();
+    addDiag(CallRange.getBegin(), diag::note_constexpr_call_here)
+        << Out.str() << CallRange;
   }
 }

clang/test/Misc/constexpr-source-ranges.cpp

 // RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-print-source-range-info -fcxx-exceptions %s 2>&1 | FileCheck %s
 // RUN: not %clang_cc1 -fsyntax-only -fexperimental-new-constant-interpreter -fdiagnostics-print-source-range-info -fcxx-exceptions %s 2>&1 | FileCheck %s
 
 constexpr int f() {
   throw 1;
   return 0;
 }
 
 // CHECK: constexpr-source-ranges.cpp:5:3:{5:3-5:10}
 
 
 constexpr int I = 12;
 constexpr const int *P = &I;
 constexpr long L = (long)P;
 // CHECK: constexpr-source-ranges.cpp:14:20:{14:20-14:27}
+
+
+constexpr int div(bool a, bool b) {
+  return 1 / (int)b;
+}
+constexpr int ints(int a, int b, int c, int d) {
+  return 1;
+}
+static_assert(ints(1, div(true, false), 2, div(false, true)) == 1, "");
+// CHECK: constexpr-source-ranges.cpp:24:23:{24:23-24:39}