[ExprConstant] Allow constexpr ctor to modify non static data members in body

[erik.pilkington]

Summary

Previously, clang rejected the following (copied from PR19741):

struct A {
  int a = 0;
  constexpr A() { a = 1; }
};
constexpr bool f() {
  constexpr A a;
  static_assert(a.a == 1, "");
  return a.a == 1;
}
static_assert(f(), "");

Clang didn't like the assignment to a in A() because it doesn't know that A is being constructed and therefore considers the subobject A.a to be const. The fix in this patch (which @rsmith suggested in the PR) is just to keep track of the set of objects that are currently being constructed, and strip away the const whenever we encounter a subobject of an object under construction.

https://bugs.llvm.org/show_bug.cgi?id=19741

Thanks for taking a look!
Erik

[rsmith]

lib/AST/ExprConstant.cpp

576 ↗

(On Diff #117445)

Please add a comment explaining what the two fields mean.

588 ↗

(On Diff #117445)

Can the DidInsert == false case actually happen?

596 ↗

(On Diff #117445)

Hmm, should we check the CallIndex is 0 in this case? I think we technically don't need to, but only because we happen to only evaluate the initializer of local variables before the enclosing function has its body attached.

Perhaps we could insert EvaluatingDecl into the EvaluatingConstructors list and simplify this check to just the DenseSet lookup?

[erik.pilkington]

Thanks for the feedback, in this new patch:

• insert EvaluatingDecl into EvaluatingConstructors instead of checking it in isEvaluatingDecl()
• Add a comment to the typedef

lib/AST/ExprConstant.cpp

588 ↗

(On Diff #117445)

Yep, if you have the following:

struct Outer {
  struct Inner {
    constexpr Inner() {}
  };
  Inner i;
  constexpr Outer() {}
};
constexpr Outer o;

There are two ctor calls for the LValueBase constexpr Outer o; at call index 0.

[rsmith]

LGTM, thank you!

lib/AST/ExprConstant.cpp

3127 ↗

(On Diff #117529)

The first half of this FIXME is now fixed, and should be removed. (But the second half -- const subobjects remain mutable past the end of their period of construction -- is still an open problem.)

cfe/trunk/lib/AST/ExprConstant.cpp

struct EvalInfo {
     /// EvaluatingDecl - This is the declaration whose initializer is being
     /// evaluated, if any.
     APValue::LValueBase EvaluatingDecl;
 
     /// EvaluatingDeclValue - This is the value being constructed for the
     /// declaration whose initializer is being evaluated, if any.
     APValue *EvaluatingDeclValue;
 
+    /// EvaluatingObject - Pair of the AST node that an lvalue represents and
+    /// the call index that that lvalue was allocated in.
+    typedef std::pair<APValue::LValueBase, unsigned> EvaluatingObject;
+
+    /// EvaluatingConstructors - Set of objects that are currently being
+    /// constructed.
+    llvm::DenseSet<EvaluatingObject> EvaluatingConstructors;
+
+    struct EvaluatingConstructorRAII {
+      EvalInfo &EI;
+      EvaluatingObject Object;
+      bool DidInsert;
+      EvaluatingConstructorRAII(EvalInfo &EI, EvaluatingObject Object)
+          : EI(EI), Object(Object) {
+        DidInsert = EI.EvaluatingConstructors.insert(Object).second;
+      }
+      ~EvaluatingConstructorRAII() {
+        if (DidInsert) EI.EvaluatingConstructors.erase(Object);
+      }
+    };
+
+    bool isEvaluatingConstructor(APValue::LValueBase Decl, unsigned CallIndex) {
+      return EvaluatingConstructors.count(EvaluatingObject(Decl, CallIndex));
+    }
+
     /// The current array initialization index, if we're performing array
     /// initialization.
     uint64_t ArrayInitIndex = -1;
 
     /// HasActiveDiagnostic - Was the previous diagnostic stored? If so, further
     /// notes attached to it will also be stored, otherwise they will not be.
     bool HasActiveDiagnostic;
 
EvalInfo(const ASTContext &C, Expr::EvalStatus &S, EvaluationMode Mode)
         EvaluatingDecl((const ValueDecl *)nullptr),
         EvaluatingDeclValue(nullptr), HasActiveDiagnostic(false),
         HasFoldFailureDiagnostic(false), IsSpeculativelyEvaluating(false),
         EvalMode(Mode) {}
 
     void setEvaluatingDecl(APValue::LValueBase Base, APValue &Value) {
       EvaluatingDecl = Base;
       EvaluatingDeclValue = &Value;
+      EvaluatingConstructors.insert({Base, 0});
     }
 
     const LangOptions &getLangOpts() const { return Ctx.getLangOpts(); }
 
     bool CheckCallLimit(SourceLocation Loc) {
       // Don't perform any constexpr calls (other than the call we're checking)
       // when checking a potential constant expression.
       if (checkingPotentialConstantExpression() && CallStackDepth > 1)
if (BaseType.isVolatileQualified()) {
       } else {
         Info.FFDiag(E);
       }
       return CompleteObject();
     }
   }
 
   // During the construction of an object, it is not yet 'const'.
-  // FIXME: We don't set up EvaluatingDecl for local variables or temporaries,
-  // and this doesn't do quite the right thing for const subobjects of the
+  // FIXME: This doesn't do quite the right thing for const subobjects of the
   // object under construction.
-  if (LVal.getLValueBase() == Info.EvaluatingDecl) {
+  if (Info.isEvaluatingConstructor(LVal.getLValueBase(), LVal.CallIndex)) {
     BaseType = Info.Ctx.getCanonicalType(BaseType);
     BaseType.removeLocalConst();
   }
 
   // In C++1y, we can't safely access any mutable state when we might be
   // evaluating after an unmodeled side effect.
   //
   // FIXME: Not all local state is mutable. Allow local constant subobjects
if (!Info.CheckCallLimit(CallLoc))
     return false;
 
   const CXXRecordDecl *RD = Definition->getParent();
   if (RD->getNumVBases()) {
     Info.FFDiag(CallLoc, diag::note_constexpr_virtual_base) << RD;
     return false;
   }
 
+  EvalInfo::EvaluatingConstructorRAII EvalObj(
+      Info, {This.getLValueBase(), This.CallIndex});
   CallStackFrame Frame(Info, CallLoc, Definition, &This, ArgValues);
 
   // 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.

cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp

constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
 }
 
 constexpr void Void(int n) {
   void(n + 1);
   void();
 }
 constexpr int void_test = (Void(0), 1);
+
+namespace PR19741 {
+constexpr void addone(int &m) { m++; }
+
+struct S {
+  int m = 0;
+  constexpr S() { addone(m); }
+};
+constexpr bool evalS() {
+  constexpr S s;
+  return s.m == 1;
+}
+static_assert(evalS(), "");
+
+struct Nested {
+  struct First { int x = 42; };
+  union {
+    First first;
+    int second;
+  };
+  int x;
+  constexpr Nested(int x) : first(), x(x) { x = 4; }
+  constexpr Nested() : Nested(42) {
+    addone(first.x);
+    x = 3;
+  }
+};
+constexpr bool evalNested() {
+  constexpr Nested N;
+  return N.first.x == 43;
+}
+static_assert(evalNested(), "");
+} // namespace PR19741