[ExprConstant] Fix crash when initialize an indirect field with another field.

[vsapsai]

Summary

When indirect field is initialized with another field, you have
MemberExpr with CXXThisExpr that corresponds to the field's immediate
anonymous parent. But 'this' was referring to the non-anonymous parent.
So when we were building LValue Designator, it was incorrect as it had
wrong starting point. Usage of such designator would cause unexpected
APValue changes and crashes.

The fix is in adjusting 'this' for indirect fields from non-anonymous
parent to the field's immediate parent.

Discovered by OSS-Fuzz:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=4985

rdar://problem/36359187

[vsapsai]

Ping.

[rsmith]

Please handle this by temporarily updating the This pointer appropriately when evaluating the CXXDefaultInitExpr rather than trying to work around the This value being wrong later (your approach will still go wrong if the This pointer is used in other ways, such as an explicit mention of this or a member function call). You can refer to how we do this in CodeGen: look for CodeGenFunction::CXXDefaultInitExprScope and CodeGenFunction::FieldConstructionScope.

[vsapsai]

Thanks for response, Richard. I'll look into using CXXDefaultInitExpr.

As for

In D42498#1007028, @rsmith wrote:

[…] your approach will still go wrong if the This pointer is used in other ways, such as an explicit mention of this or a member function call.

I wasn't able to find a failing test case. Maybe I misunderstood you but in my testing functions declared in named structs had correct This pointer and in anonymous structs functions cannot be declared.

[vsapsai]

• Override This for indirect field initializer more like it is done for InitListExpr.

I rebased my changes, so the diff between different versions can be noisy.

[rsmith]

Looks good, thanks!

In D42498#1015419, @vsapsai wrote:

In D42498#1007028, @rsmith wrote:

[…] your approach will still go wrong if the This pointer is used in other ways, such as an explicit mention of this or a member function call.

I wasn't able to find a failing test case. Maybe I misunderstood you but in my testing functions declared in named structs had correct This pointer and in anonymous structs functions cannot be declared.

Here's a testcase that fails today:

struct A { int n = 0; struct { void *p = this; }; void *q = this; };
constexpr A a = A();
static_assert(a.p != a.q, ""); // fails today, should pass

constexpr A b = A{0};
static_assert(b.p != b.q, ""); // passes today

With the previous approach of fixing up the This value only in class member access, the above testcase would presumably still fail. I expect it to pass with your new approach.

[vsapsai]

Thanks for the review. Explicit this usage is a good test, added it.

Checked that it happened to work with my previous approach because initializer was CXXDefaultInitExpr with expression

ImplicitCastExpr 0x7fe966808c40 'void *' <BitCast>
`-CXXThisExpr 0x7fe966808c28 'struct A::(anonymous at file.cc:3:5) *' this

cfe/trunk/lib/AST/ExprConstant.cpp

static bool HandleConstructorCall(const Expr *E, const LValue &This,
 
   bool Success = true;
   unsigned BasesSeen = 0;
 #ifndef NDEBUG
   CXXRecordDecl::base_class_const_iterator BaseIt = RD->bases_begin();
 #endif
   for (const auto *I : Definition->inits()) {
     LValue Subobject = This;
+    LValue SubobjectParent = This;
     APValue *Value = &Result;
 
     // Determine the subobject to initialize.
     FieldDecl *FD = nullptr;
     if (I->isBaseInitializer()) {
       QualType BaseType(I->getBaseClass(), 0);
 #ifndef NDEBUG
       // Non-virtual base classes are initialized in the order in the class
#endif
         Result = APValue(FD);
         Value = &Result.getUnionValue();
       } else {
         Value = &Result.getStructField(FD->getFieldIndex());
       }
     } else if (IndirectFieldDecl *IFD = I->getIndirectMember()) {
       // Walk the indirect field decl's chain to find the object to initialize,
       // and make sure we've initialized every step along it.
-      for (auto *C : IFD->chain()) {
+      auto IndirectFieldChain = IFD->chain();
+      for (auto *C : IndirectFieldChain) {
         FD = cast<FieldDecl>(C);
         CXXRecordDecl *CD = cast<CXXRecordDecl>(FD->getParent());
         // Switch the union field if it differs. This happens if we had
         // preceding zero-initialization, and we're now initializing a union
         // subobject other than the first.
         // FIXME: In this case, the values of the other subobjects are
         // specified, since zero-initialization sets all padding bits to zero.
         if (Value->isUninit() ||
             (Value->isUnion() && Value->getUnionField() != FD)) {
           if (CD->isUnion())
             *Value = APValue(FD);
           else
             *Value = APValue(APValue::UninitStruct(), CD->getNumBases(),
                              std::distance(CD->field_begin(), CD->field_end()));
         }
+        // Store Subobject as its parent before updating it for the last element
+        // in the chain.
+        if (C == IndirectFieldChain.back())
+          SubobjectParent = Subobject;
         if (!HandleLValueMember(Info, I->getInit(), Subobject, FD))
           return false;
         if (CD->isUnion())
           Value = &Value->getUnionValue();
         else
           Value = &Value->getStructField(FD->getFieldIndex());
       }
     } else {
       llvm_unreachable("unknown base initializer kind");
     }
 
+    // Need to override This for implicit field initializers as in this case
+    // This refers to innermost anonymous struct/union containing initializer,
+    // not to currently constructed class.
+    const Expr *Init = I->getInit();
+    ThisOverrideRAII ThisOverride(*Info.CurrentCall, &SubobjectParent,
+                                  isa<CXXDefaultInitExpr>(Init));
     FullExpressionRAII InitScope(Info);
-    if (!EvaluateInPlace(*Value, Info, Subobject, I->getInit()) ||
-        (FD && FD->isBitField() && !truncateBitfieldValue(Info, I->getInit(),
-                                                          *Value, FD))) {
+    if (!EvaluateInPlace(*Value, Info, Subobject, Init) ||
+        (FD && FD->isBitField() &&
+         !truncateBitfieldValue(Info, Init, *Value, FD))) {
       // If we're checking for a potential constant expression, evaluate all
       // initializers even if some of them fail.
       if (!Info.noteFailure())
         return false;
       Success = false;
     }
   }
 

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

namespace Mutable {
   struct B {
     mutable int n; // expected-note {{here}}
     int m;
     constexpr B() : n(1), m(n) {} // ok
   };
   constexpr B b;
   constexpr int p = b.n; // expected-error {{constant expression}} expected-note {{mutable}}
 }
+
+namespace IndirectFields {
+
+// Reference indirect field.
+struct A {
+  struct {
+    union {
+      int x = x = 3; // expected-note {{outside its lifetime}}
+    };
+  };
+  constexpr A() {}
+};
+static_assert(A().x == 3, ""); // expected-error{{not an integral constant expression}} expected-note{{in call to 'A()'}}
+
+// Reference another indirect field, with different 'this'.
+struct B {
+  struct {
+    union {
+      int x = 3;
+    };
+    int y = x;
+  };
+  constexpr B() {}
+};
+static_assert(B().y == 3, "");
+
+// Nested evaluation of indirect field initializers.
+struct C {
+  union {
+    int x = 1;
+  };
+};
+struct D {
+  struct {
+    C c;
+    int y = c.x + 1;
+  };
+};
+static_assert(D().y == 2, "");
+
+// Explicit 'this'.
+struct E {
+  int n = 0;
+  struct {
+    void *x = this;
+  };
+  void *y = this;
+};
+constexpr E e1 = E();
+static_assert(e1.x != e1.y, "");
+constexpr E e2 = E{0};
+static_assert(e2.x != e2.y, "");
+
+} // namespace IndirectFields