[clang] fixing diagnostics of constexpr callstack

[Tyker]

Summary

this is a bugfix for this

added a constant copy of argument to CallStackFrame.
and use it in describeCall instead of the modifiable version od arguments.
added test for the diagnostic

[• Quuxplusone]

clang/lib/AST/ExprConstant.cpp

486

Please don't use a typedef for this. Follow the style of the surrounding lines.

const APValue *ConstantArgs;

(The pointer member itself should not be const-qualified. Compare to const LValue *This; two declarations above.)

4520

const APValue *ConstantArgs,

4684

Surely you meant ConstantArgs (plural) here.

clang/test/SemaCXX/constant-expression-cxx1y.cpp

1183

Personally, I would prefer to see something as simple as my original example; I don't think the extra 6 levels of recursion here makes the test any easier to understand.

constexpr int f(int i) {
    i = -i;
    return 1 << i;  // expected-note{{negative shift count -1}}
}
static_assert(f(1));
// expected-error@-1{{static_assert expression is not an integral constant expression}}
// expected-note@-2{{in call to 'f(1)'}}

Also, procedural note: I expect you'll be asked to reupload this diff with full context (git diff -U999 ...).

[Tyker]

@Quuxplusone edited based of feedback

i simplified the test but didn't put the original because this one test arguments for variable and literal and there may be corner case differences

[rsmith]

This seems liable to be moderately expensive, especially for large argument values, and it's a cost that is unnecessary in the common case where evaluation succeeds.

I wonder if we'd be better off speculatively trying the entire evaluation without storing any such values, and rerunning the evaluation to collect diagnostics only when we find that evaluation would fail (and we're in a context where the caller actually wants the diagnostics).

[Tyker]

@rsmith i don't think collecting theses values is expansive compared to evaluating the expression.
but i agree that we can disable the collection of these values when no diagnostics can be emited.

[riccibruno]

In D60561#1464740, @Tyker wrote:

@rsmith i don't think collecting theses values is expansive compared to evaluating the expression.
but i agree that we can disable the collection of these values when no diagnostics can be emited.

To help make an informed decision you can grab a big TU (I usually use SemaDecl.cpp or all of Boost) and then run -fsyntax-only on it. With a tool like perf stat you can get pretty good results.

[Tyker]

the impact was much higher than i expected, around 1% slower in average on 50 compilation of SemaDecl with -fsyntax-only.

[riccibruno]

In D60561#1465373, @Tyker wrote:

the impact was much higher than i expected, around 1% slower in average on 50 compilation of SemaDecl with -fsyntax-only.

Good to know, thanks for doing the measurement !

[Tyker]

i changed the way arguments are stored. to make it more controllable.
added an argument call stack where it is needed.
this version slows down compilation by around 0.5% in average over 200 run for SemaDecl -fsyntax-only

clang/lib/AST/ExprConstant.cpp

struct CallStackFrame {
     /// Arguments - Parameter bindings for this function call, indexed by
     /// parameters' function scope indices.
     APValue *Arguments;
 
     // 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;
 
     /// Index - The call index of this call.
     unsigned Index;
 
struct EvalInfo {
     ASTContext &Ctx;
 
     /// EvalStatus - Contains information about the evaluation.
     Expr::EvalStatus &EvalStatus;
 
     /// CurrentCall - The top of the constexpr call stack.
     CallStackFrame *CurrentCall;
 
+    /// ArgumentCallStack - Used to store Arguments to function calls
+    /// only required if diagnostics should produce a callstack
+    SmallVectorImpl<APValue> *ArgumentCallStack;
+
     /// CallStackDepth - The number of calls in the call stack right now.
     unsigned CallStackDepth;
 
     /// NextCallIndex - The next call index to assign.
     unsigned NextCallIndex;
 
     /// StepsLeft - The remaining number of evaluation steps we're permitted
     /// to perform. This is essentially a limit for the number of statements
struct EvalInfo {
     }
 
     /// Are we checking an expression for overflow?
     // FIXME: We should check for any kind of undefined or suspicious behavior
     // in such constructs, not just overflow.
     bool checkingForOverflow() { return EvalMode == EM_EvaluateForOverflow; }
 
     EvalInfo(const ASTContext &C, Expr::EvalStatus &S, EvaluationMode Mode)
         : Ctx(const_cast<ASTContext &>(C)), EvalStatus(S), CurrentCall(nullptr),
-        CallStackDepth(0), NextCallIndex(1),
+          ArgumentCallStack(nullptr), CallStackDepth(0), NextCallIndex(1),
           StepsLeft(getLangOpts().ConstexprStepLimit),
           BottomFrame(*this, SourceLocation(), nullptr, nullptr, nullptr),
           EvaluatingDecl((const ValueDecl *)nullptr),
           EvaluatingDeclValue(nullptr), HasActiveDiagnostic(false),
           HasFoldFailureDiagnostic(false), IsSpeculativelyEvaluating(false),
           InConstantContext(false), EvalMode(Mode) {}
 
     void setEvaluatingDecl(APValue::LValueBase Base, APValue &Value) {
       EvaluatingDecl = Base;
       EvaluatingDeclValue = &Value;
       EvaluatingConstructors.insert({Base, {0, 0}});
     }
 
     const LangOptions &getLangOpts() const { return Ctx.getLangOpts(); }

 
 CallStackFrame::CallStackFrame(EvalInfo &Info, SourceLocation CallLoc,
                                const FunctionDecl *Callee, const LValue *This,
                                APValue *Arguments)
     : Info(Info), Caller(Info.CurrentCall), Callee(Callee), This(This),
       Arguments(Arguments), CallLoc(CallLoc), Index(Info.NextCallIndex++) {
   Info.CurrentCall = this;
   ++Info.CallStackDepth;
+  if (Info.ArgumentCallStack) {
+    Info.ArgumentCallStack->reserve(Info.ArgumentCallStack->size() +
+                                    Callee->param_size());
+    std::copy(Arguments, Arguments + Callee->param_size(),
+              std::back_inserter(*Info.ArgumentCallStack));
+  }
 }
 
 CallStackFrame::~CallStackFrame() {
   assert(Info.CurrentCall == this && "calls retired out of order");
+  if (Info.ArgumentCallStack && Caller) {
+    Info.ArgumentCallStack->resize(Info.ArgumentCallStack->size() -
+                                   Callee->param_size());
+  }
   --Info.CallStackDepth;
   Info.CurrentCall = Caller;
 }
 
 APValue &CallStackFrame::createTemporary(const void *Key,
                                          bool IsLifetimeExtended) {
   unsigned Version = Info.CurrentCall->getTempVersion();
   APValue &Result = Temporaries[MapKeyTy(Key, Version)];
   assert(Result.isUninit() && "temporary created multiple times");
   Info.CleanupStack.push_back(Cleanup(&Result, IsLifetimeExtended));
   return Result;
 }
 
-static void describeCall(CallStackFrame *Frame, raw_ostream &Out);
+static void describeCall(CallStackFrame *Frame, raw_ostream &Out,
+                         SmallVectorImpl<APValue> *ArgumentCallStack,
+                         unsigned Pos);
 
 void EvalInfo::addCallStack(unsigned Limit) {
+  assert(ArgumentCallStack && "needed to produce a call stack");
   // Determine which calls to skip, if any.
   unsigned ActiveCalls = CallStackDepth - 1;
   unsigned SkipStart = ActiveCalls, SkipEnd = SkipStart;
   if (Limit && Limit < ActiveCalls) {
     SkipStart = Limit / 2 + Limit % 2;
     SkipEnd = ActiveCalls - Limit / 2;
   }
 
   // Walk the call stack and add the diagnostics.
   unsigned CallIdx = 0;
+  unsigned Pos = ArgumentCallStack->size();
   for (CallStackFrame *Frame = CurrentCall; Frame != &BottomFrame;
        Frame = Frame->Caller, ++CallIdx) {
+    Pos -= Frame->Callee->param_size();
     // Skip this call?
     if (CallIdx >= SkipStart && CallIdx < SkipEnd) {
       if (CallIdx == SkipStart) {
         // Note that we're skipping calls.
         addDiag(Frame->CallLoc, 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 (auto *CD = dyn_cast_or_null<CXXConstructorDecl>(Frame->Callee)) {
       if (CD->isInheritingConstructor()) {
         addDiag(Frame->CallLoc, diag::note_constexpr_inherited_ctor_call_here)
           << CD->getParent();
         continue;
       }
     }
 
     SmallVector<char, 128> Buffer;
     llvm::raw_svector_ostream Out(Buffer);
-    describeCall(Frame, Out);
+    describeCall(Frame, Out, ArgumentCallStack, Pos);
     addDiag(Frame->CallLoc, diag::note_constexpr_call_here) << Out.str();
   }
 }
 
 /// Kinds of access we can perform on an object, for diagnostics.
 enum AccessKinds {
   AK_Read,
   AK_Assign,
static void negateAsSigned(APSInt &Int) {
   if (Int.isUnsigned() || Int.isMinSignedValue()) {
     Int = Int.extend(Int.getBitWidth() + 1);
     Int.setIsSigned(true);
   }
   Int = -Int;
 }
 
 /// Produce a string describing the given constexpr call.
-static void describeCall(CallStackFrame *Frame, raw_ostream &Out) {
+static void describeCall(CallStackFrame *Frame, raw_ostream &Out,
+                         SmallVectorImpl<APValue> *ArgumentCallStack,
+                         unsigned Pos) {
   unsigned ArgIndex = 0;
   bool IsMemberCall = isa<CXXMethodDecl>(Frame->Callee) &&
                       !isa<CXXConstructorDecl>(Frame->Callee) &&
                       cast<CXXMethodDecl>(Frame->Callee)->isInstance();
 
   if (!IsMemberCall)
     Out << *Frame->Callee << '(';
 
   if (Frame->This && IsMemberCall) {
     APValue Val;
     Frame->This->moveInto(Val);
     Val.printPretty(Out, Frame->Info.Ctx,
                     Frame->This->Designator.MostDerivedType);
     // FIXME: Add parens around Val if needed.
     Out << "->" << *Frame->Callee << '(';
     IsMemberCall = false;
   }
 
   for (FunctionDecl::param_const_iterator I = Frame->Callee->param_begin(),
        E = Frame->Callee->param_end(); I != E; ++I, ++ArgIndex) {
     if (ArgIndex > (unsigned)IsMemberCall)
       Out << ", ";
 
     const ParmVarDecl *Param = *I;
-    const APValue &Arg = Frame->Arguments[ArgIndex];
+    const APValue &Arg = (*ArgumentCallStack)[Pos + ArgIndex];
     Arg.printPretty(Out, Frame->Info.Ctx, Param->getType());
 
     if (ArgIndex == 0 && IsMemberCall)
       Out << "->" << *Frame->Callee << '(';
   }
 
   Out << ')';
 }
if (!Info.checkingPotentialConstantExpression())
       MD->getParent()->getCaptureFields(Frame.LambdaCaptureFields,
                                         Frame.LambdaThisCaptureField);
   }
 
   StmtResult Ret = {Result, ResultSlot};
   EvalStmtResult ESR = EvaluateStmt(Ret, Info, Body);
   if (ESR == ESR_Succeeded) {
     if (Callee->getReturnType()->isVoidType())
       return true;
     Info.FFDiag(Callee->getEndLoc(), diag::note_constexpr_no_return);
   }
   return ESR == ESR_Returned;
 }
 
 /// Evaluate a constructor call.
 static bool HandleConstructorCall(const Expr *E, const LValue &This,
                                   APValue *ArgValues,
#endif
     }
   }
 
   return Success &&
          EvaluateStmt(Ret, Info, Definition->getBody()) != ESR_Failed;
 }
 
 static bool HandleConstructorCall(const Expr *E, const LValue &This,
                                   ArrayRef<const Expr*> Args,
                                   const CXXConstructorDecl *Definition,
                                   EvalInfo &Info, APValue &Result) {
   ArgVector ArgValues(Args.size());
   if (!EvaluateArgs(Args, ArgValues, Info))
     return false;
 
   return HandleConstructorCall(E, This, ArgValues.data(), Definition,
                                Info, Result);

 /// EvaluateAsRValue - Return true if this is a constant which we can fold using
 /// any crazy technique (that has nothing to do with language standards) that
 /// we want to.  If this function returns true, it returns the folded constant
 /// in Result. If this expression is a glvalue, an lvalue-to-rvalue conversion
 /// will be applied to the result.
 bool Expr::EvaluateAsRValue(EvalResult &Result, const ASTContext &Ctx,
                             bool InConstantContext) const {
   EvalInfo Info(Ctx, Result, EvalInfo::EM_IgnoreSideEffects);
+  SmallVector<APValue, 24> ArgumentCallStack;
+  Info.ArgumentCallStack = &ArgumentCallStack;
   Info.InConstantContext = InConstantContext;
   return ::EvaluateAsRValue(this, Result, Ctx, Info);
 }
 
 bool Expr::EvaluateAsBooleanCondition(bool &Result,
                                       const ASTContext &Ctx) const {
   EvalResult Scratch;
   return EvaluateAsRValue(Scratch, Ctx) &&
if (!EvaluateAsRValue(ExprResult, Ctx) || !ExprResult.Val.isFloat() ||
     return false;
 
   Result = ExprResult.Val.getFloat();
   return true;
 }
 
 bool Expr::EvaluateAsLValue(EvalResult &Result, const ASTContext &Ctx) const {
   EvalInfo Info(Ctx, Result, EvalInfo::EM_ConstantFold);
-
   LValue LV;
   if (!EvaluateLValue(this, LV, Info) || Result.HasSideEffects ||
       !CheckLValueConstantExpression(Info, getExprLoc(),
                                      Ctx.getLValueReferenceType(getType()), LV,
                                      Expr::EvaluateForCodeGen))
     return false;
 
   LV.moveInto(Result.Val);
   return true;
 }
 
 bool Expr::EvaluateAsConstantExpr(EvalResult &Result, ConstExprUsage Usage,
                                   const ASTContext &Ctx) const {
   EvalInfo::EvaluationMode EM = EvalInfo::EM_ConstantExpression;
   EvalInfo Info(Ctx, Result, EM);
+  SmallVector<APValue, 24> ArgumentCallStack;
+  Info.ArgumentCallStack = &ArgumentCallStack;
   Info.InConstantContext = true;
   if (!::Evaluate(Result.Val, Info, this))
     return false;
 
   return CheckConstantExpression(Info, getExprLoc(), getType(), Result.Val,
                                  Usage);
 }
 
bool Expr::EvaluateAsInitializer(APValue &Value, const ASTContext &Ctx,
   Expr::EvalStatus EStatus;
   EStatus.Diag = &Notes;
 
   EvalInfo InitInfo(Ctx, EStatus, VD->isConstexpr()
                                       ? EvalInfo::EM_ConstantExpression
                                       : EvalInfo::EM_ConstantFold);
   InitInfo.setEvaluatingDecl(VD, Value);
   InitInfo.InConstantContext = true;
-
+  SmallVector<APValue, 8> ArgumentCallStack;
+  InitInfo.ArgumentCallStack = &ArgumentCallStack;
   LValue LVal;
   LVal.set(VD);
 
   // C++11 [basic.start.init]p2:
   //  Variables with static storage duration or thread storage duration shall be
   //  zero-initialized before any other initialization takes place.
   // This behavior is not present in C.
   if (Ctx.getLangOpts().CPlusPlus && !VD->hasLocalStorage() &&
bool Expr::isCXX11ConstantExpr(const ASTContext &Ctx, APValue *Result,
   // issues.
   assert(Ctx.getLangOpts().CPlusPlus);
 
   // Build evaluation settings.
   Expr::EvalStatus Status;
   SmallVector<PartialDiagnosticAt, 8> Diags;
   Status.Diag = &Diags;
   EvalInfo Info(Ctx, Status, EvalInfo::EM_ConstantExpression);
+  SmallVector<APValue, 24> ArgumentCallStack;
+  Info.ArgumentCallStack = &ArgumentCallStack;
 
   APValue Scratch;
   bool IsConstExpr = ::EvaluateAsRValue(Info, this, Result ? *Result : Scratch);
 
   if (!Diags.empty()) {
     IsConstExpr = false;
     if (Loc) *Loc = Diags[0].first;
   } else if (!IsConstExpr) {

clang/test/SemaCXX/constant-expression-cxx1y.cpp

enum InEnum1 {
   ONE = indirect_builtin_constant_p("abc")
 };
 enum InEnum2 : int {
   TWO = indirect_builtin_constant_p("abc")
 };
 enum class InEnum3 {
   THREE = indirect_builtin_constant_p("abc")
 };
+
+constexpr int f1(int i, int b) {
+  i += 4;
+  b -= 4;
+  // expected-note@+1 {{negative shift count -7}}
+  return i << b;
+}
+
+constexpr int f2(int i) {
+  int b = 0;
+  i += 1;
+  b -= 1;
+  // expected-note@+1 {{in call to 'f1(3, -3)'}}
+  return f1(i + 2, b -= 2);
+}
+
+constexpr int f(int i) {
+  i -= 1;
+  //expected-note@+1 {{negative shift count -1}}
+  return 1 << i;
+}
+
+// expected-error@+2 {{constant expression}}
+// expected-note@+1 {{in call to 'f2(0)'}}
+constexpr int val = f2(0);
+
+// expected-error@+2 {{constant expression}}
+// expected-note@+1 {{in call to 'f(0)'}}
+static_assert(f(0), "");
 No newline at end of file