r217852 - Reject a slightly-sneaky way to perform a read of mutable state from within a

[Richard Smith]

Author: rsmith
Date: Mon Sep 15 20:24:02 2014
New Revision: 217852

URL: http://llvm.org/viewvc/llvm-project?rev=217852&view=rev
Log:
Reject a slightly-sneaky way to perform a read of mutable state from within a
constexpr function. Part of this fix is a tentative fix for an as-yet-unfiled
core issue (we're missing a prohibition against reading mutable members from
unions via a trivial constructor/assignment, since that doesn't perform an
lvalue-to-rvalue conversion on the members).

Modified:
    cfe/trunk/lib/AST/ExprConstant.cpp
    cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp

Modified: cfe/trunk/lib/AST/ExprConstant.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=217852&r1=217851&r2=217852&view=diff
==============================================================================
--- cfe/trunk/lib/AST/ExprConstant.cpp (original)
+++ cfe/trunk/lib/AST/ExprConstant.cpp Mon Sep 15 20:24:02 2014
@@ -2080,6 +2080,64 @@ static void expandArray(APValue &Array,
   Array.swap(NewValue);
 }
 
+/// Determine whether a type would actually be read by an lvalue-to-rvalue
+/// conversion. If it's of class type, we may assume that the copy operation
+/// is trivial. Note that this is never true for a union type with fields
+/// (because the copy always "reads" the active member) and always true for
+/// a non-class type.
+static bool isReadByLvalueToRvalueConversion(QualType T) {
+  CXXRecordDecl *RD = T->getBaseElementTypeUnsafe()->getAsCXXRecordDecl();
+  if (!RD || (RD->isUnion() && !RD->field_empty()))
+    return true;
+  if (RD->isEmpty())
+    return false;
+
+  for (auto *Field : RD->fields())
+    if (isReadByLvalueToRvalueConversion(Field->getType()))
+      return true;
+
+  for (auto &BaseSpec : RD->bases())
+    if (isReadByLvalueToRvalueConversion(BaseSpec.getType()))
+      return true;
+
+  return false;
+}
+
+/// Diagnose an attempt to read from any unreadable field within the specified
+/// type, which might be a class type.
+static bool diagnoseUnreadableFields(EvalInfo &Info, const Expr *E,
+                                     QualType T) {
+  CXXRecordDecl *RD = T->getBaseElementTypeUnsafe()->getAsCXXRecordDecl();
+  if (!RD)
+    return false;
+
+  if (!RD->hasMutableFields())
+    return false;
+
+  for (auto *Field : RD->fields()) {
+    // If we're actually going to read this field in some way, then it can't
+    // be mutable. If we're in a union, then assigning to a mutable field
+    // (even an empty one) can change the active member, so that's not OK.
+    // FIXME: Add core issue number for the union case.
+    if (Field->isMutable() &&
+        (RD->isUnion() || isReadByLvalueToRvalueConversion(Field->getType()))) {
+      Info.Diag(E, diag::note_constexpr_ltor_mutable, 1) << Field;
+      Info.Note(Field->getLocation(), diag::note_declared_at);
+      return true;
+    }
+
+    if (diagnoseUnreadableFields(Info, E, Field->getType()))
+      return true;
+  }
+
+  for (auto &BaseSpec : RD->bases())
+    if (diagnoseUnreadableFields(Info, E, BaseSpec.getType()))
+      return true;
+
+  // All mutable fields were empty, and thus not actually read.
+  return false;
+}
+
 /// Kinds of access we can perform on an object, for diagnostics.
 enum AccessKinds {
   AK_Read,
@@ -2135,6 +2193,14 @@ findSubobject(EvalInfo &Info, const Expr
     }
 
     if (I == N) {
+      // If we are reading an object of class type, there may still be more
+      // things we need to check: if there are any mutable subobjects, we
+      // cannot perform this read. (This only happens when performing a trivial
+      // copy or assignment.)
+      if (ObjType->isRecordType() && handler.AccessKind == AK_Read &&
+          diagnoseUnreadableFields(Info, E, ObjType))
+        return handler.failed();
+
       if (!handler.found(*O, ObjType))
         return false;
 

Modified: cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp?rev=217852&r1=217851&r2=217852&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp (original)
+++ cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp Mon Sep 15 20:24:02 2014
@@ -1326,6 +1326,49 @@ namespace MutableMembers {
   struct C { B b; };
   constexpr C c[3] = {};
   constexpr int k = c[1].b.a.n; // expected-error {{constant expression}} expected-note {{mutable}}
+
+  struct D { int x; mutable int y; }; // expected-note {{here}}
+  constexpr D d1 = { 1, 2 };
+  int l = ++d1.y;
+  constexpr D d2 = d1; // expected-error {{constant}} expected-note {{mutable}} expected-note {{in call}}
+
+  struct E {
+    union {
+      int a;
+      mutable int b; // expected-note {{here}}
+    };
+  };
+  constexpr E e1 = {{1}};
+  constexpr E e2 = e1; // expected-error {{constant}} expected-note {{mutable}} expected-note {{in call}}
+
+  struct F {
+    union U { };
+    mutable U u;
+    struct X { };
+    mutable X x;
+    struct Y : X { X x; U u; };
+    mutable Y y;
+    int n;
+  };
+  // This is OK; we don't actually read any mutable state here.
+  constexpr F f1 = {};
+  constexpr F f2 = f1;
+
+  struct G {
+    struct X {};
+    union U { X a; };
+    mutable U u; // expected-note {{here}}
+  };
+  constexpr G g1 = {};
+  constexpr G g2 = g1; // expected-error {{constant}} expected-note {{mutable}} expected-note {{in call}}
+  constexpr G::U gu1 = {};
+  constexpr G::U gu2 = gu1;
+
+  union H {
+    mutable G::X gx; // expected-note {{here}}
+  };
+  constexpr H h1 = {};
+  constexpr H h2 = h1; // expected-error {{constant}} expected-note {{mutable}} expected-note {{in call}}
 }
 
 namespace Fold {