[analyzer] Fix a crash on C++17 AST for non-trivial construction into a trivial brace initializer.

[NoQ]

Summary

Yet another situation where we cannot easily guess, by looking at the CFG, the target region for C++ constructor call.

Consider the following brace initialization:

struct A {
  A();
 };

struct B : public A {
  int x;
};

void foo() {
  B b = {};
}

AST in C++14:

`-VarDecl 0x7ff07884eab8 <col:3, col:10> col:5 b 'struct B' cinit
  `-CXXConstructExpr 0x7ff07887a940 <col:9, col:10> 'struct B' 'void (void) noexcept(false)' list zeroing

AST in C++17:

`-VarDecl 0x7fb1cf012b18 <col:3, col:10> col:5 b 'struct B' cinit
  `-InitListExpr 0x7fb1cf012f38 <col:9, col:10> 'struct B'
    |-CXXConstructExpr 0x7fb1cf012fd0 <col:10> 'struct A' 'void (void)' list
    `-ImplicitValueInitExpr 0x7fb1cf013000 <<invalid sloc>> 'int'

CFG in C++14:

[B1]
  1: {} (CXXConstructExpr, struct B)
  2: B b = {};

CFG in C++17:

[B1]
  1: {} (CXXConstructExpr, struct A)
  2: /*implicit*/(int)0
  3: {}
  4: B b = {};

So, essentially, in C++17 we don't have the trivial constructor call for B present anywhere at all, neither in AST, nor in CFG.

This causes a crash in the analyzer because he tries to find the target region for CK_NonVirtualBase constructor of A by looking at the parent stack frame, expecting to find the derived class constructor (for B) here, and then taking a base region within its this-object.

This fix is a quick-and-dirty suppression for the crash. It follows the tradition of "when unsure, make up a temporary and construct into that, then discard the result" - which is wrong, but at least it gets some invalidation done.

In our example it would be correct to construct into base{A, b}. There can also be situations when such initializer-list is instead lifetime-extended by a const& or && reference (see the tests), so in this case we'd need to construct into a respective sub-region of the temporary (but not into the temporary itself, of course).

Finally, from the perspective of checker development, i believe it would be useful to actually revive the constructor call, so that PreCall/PostCall event occured.

[NoQ]

Add a forgotten //no-warning.

[a.sidorin]

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

288

Could we try to make another lookup to see if we're initializing a variable of non-reference type? If so, we can make MemRegionManager use the region of the variable; otherwise, we can always fallback to temporary (it seems like sometimes this situation can happen out of DeclStmts). Or am I missing something?

[NoQ]

A reply from @rsmith was accidentally lost:

http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20171204/211785.html

(seems that phabricator cuts away all text below the first "On ..., Such-and-such wrote:" line)

[NoQ]

Note that there is no constructor call here. This is aggregate initialization. And there's not really any part of this that's new, except that a class with base classes is now an a aggregate. You'll see the same kind of AST formed in all C++ language modes with a slightly modified example:

struct A {
  A();
};

struct B {
  A a;
  int x;
};

void foo() {
  B b = {};
}

The analyzer should presumably treat the new example the exact same way it treats that case.

Thank you Richard! This sheds light on what situations do we need to cover.

For now it seems that with the patch we'd indeed be treating the base class case the exact same way as the field case: by bailing out. So both cases would need to be fixed. And the field case is very important because it's not C++17-specific.

I guess i'd finish the operator new adventure first, but this issue is definitely on the list of important-to-fix C++ stuff. And for now I guess i'd just stick something in there so that it didn't crash.

Updated comments to explain the situation.

[NoQ]

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

288

Yeah, that's what we do in other cases, i guess that's the shortest way to go. As usual, it would be way better if we just knew this stuff from, like, CFG. Maybe transform the constructor CFGElement from StmtElement to a better element that has a pointer to the parent expression that represents the object that's being constructed.

[dcoughlin]

This looks good to me for a quick fix that we plan to address in a more principled fashion later.

However, I'd like you to add a comment at each of the places where you use the parent map to note something like "This is a quick dirty fix and we really shouldn't be using the parent map to model behavior." I understand why you're doing it, but I'd like to make sure that someone reading this code doesn't think that it is a good pattern to use the parent map and decide to use it elsewhere!

Using the parent map is slow and is a sign that our reasoning isn't compositional. The analyzer should be able to figure out the correct behavior based on the expression and its state/context alone. The fact that we need the parent map here is an indication that we don't have the right representation for constructors in the analyzer. (I know you're planning on tackling this in the future!)

lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp

 // Generics Checker for Objective-C:
 // This checker tries to find type errors that the compiler is not able to catch
 // due to the implicit conversions that were introduced for backward
 // compatibility.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangSACheckers.h"
+#include "clang/AST/ParentMap.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
case CXXConstructExpr::CK_Delegating:
       // means the dynamic type info will be correct even for objects
       // constructed with operator new.
       return;
     case CXXConstructExpr::CK_NonVirtualBase:
     case CXXConstructExpr::CK_VirtualBase:
       if (const MemRegion *Target = Ctor->getCXXThisVal().getAsRegion()) {
         // We just finished a base constructor. Now we can use the subclass's
         // type when resolving virtual calls.
-        const Decl *D = C.getLocationContext()->getDecl();
-        recordFixedType(Target, cast<CXXConstructorDecl>(D), C);
+        const LocationContext *LCtx = C.getLocationContext();
+
+        // FIXME: In C++17 classes with non-virtual bases may be treated as
+        // aggregates, and in such case no top-frame constructor will be called.
+        // Figure out if we need to do anything in this case.
+        if (dyn_cast_or_null<InitListExpr>(
+                LCtx->getParentMap().getParent(Ctor->getOriginExpr())))
+          return;
+
+        recordFixedType(Target, cast<CXXConstructorDecl>(LCtx->getDecl()), C);
       }
       return;
     }
   }
 }
 
 /// TODO: Handle explicit casts.
 ///       Handle C++ casts.

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

 //===- ExprEngineCXX.cpp - ExprEngine support for C++ -----------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines the C++ expression evaluation engine.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/StmtCXX.h"
+#include "clang/AST/ParentMap.h"
 #include "clang/Basic/PrettyStackTrace.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 
 using namespace clang;
 using namespace ento;
 
if (const Stmt *Outer = LCtx->getCurrentStackFrame()->getCallSite()) {
         case CXXConstructExpr::CK_Complete:
         case CXXConstructExpr::CK_Delegating:
           break;
         }
       }
     }
     // FALLTHROUGH
   case CXXConstructExpr::CK_NonVirtualBase:
+    // In C++17, classes with non-virtual bases may be aggregates, so they would
+    // be initialized as aggregates without a constructor call, so we may have
+    // a base class constructed directly into an initializer list without
+    // having the derived-class constructor call on the previous stack frame.
+    // Initializer lists may be nested into more initializer lists that
+    // correspond to surrounding aggregate initializations.
+    // FIXME: For now this code essentially bails out. We need to find the
+    // correct target region and set it.
+    if (dyn_cast_or_null<InitListExpr>(LCtx->getParentMap().getParent(CE))) {
+      MemRegionManager &MRMgr = getSValBuilder().getRegionManager();
+      Target = MRMgr.getCXXTempObjectRegion(CE, LCtx);
+      break;
+    }
+    // FALLTHROUGH
   case CXXConstructExpr::CK_Delegating: {
     const CXXMethodDecl *CurCtor = cast<CXXMethodDecl>(LCtx->getDecl());
     Loc ThisPtr = getSValBuilder().getCXXThis(CurCtor,
                                               LCtx->getCurrentStackFrame());
     SVal ThisVal = State->getSVal(ThisPtr);
 
     if (CE->getConstructionKind() == CXXConstructExpr::CK_Delegating) {
       Target = ThisVal.getAsRegion();

test/Analysis/initializer.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,cplusplus.NewDeleteLeaks,debug.ExprInspection -analyzer-config c++-inlining=constructors -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,cplusplus.NewDeleteLeaks,debug.ExprInspection -analyzer-config c++-inlining=constructors -std=c++17 -DCPLUSPLUS17 -verify %s
 
 void clang_analyzer_eval(bool);
 
 #include "Inputs/system-header-simulator-cxx.h"
 
 class A {
   int x;
 public:
void foo() {
   C empty{}; // no-crash
 
   // Do not warn that 'x' leaks. It might have been deleted by
   // the destructor of 'c'.
   int *x = new int;
   C c{x}; // no-warning
 }
 }
+
+namespace CXX17_aggregate_construction {
+struct A {
+  A();
+};
+
+struct B: public A {
+};
+
+struct C: public B {
+};
+
+struct D: public virtual A {
+};
+
+// In C++17, classes B and C are aggregates, so they will be constructed
+// without actually calling their trivial constructor. Used to crash.
+void foo() {
+  B b = {}; // no-crash
+  const B &bl = {}; // no-crash
+  B &&br = {}; // no-crash
+
+  C c = {}; // no-crash
+  const C &cl = {}; // no-crash
+  C &&cr = {}; // no-crash
+
+  D d = {}; // no-crash
+
+#ifdef CPLUSPLUS17
+  C cd = {{}}; // no-crash
+  const C &cdl = {{}}; // no-crash
+  C &&cdr = {{}}; // no-crash
+
+  const B &bll = {{}}; // no-crash
+  const B &bcl = C({{}}); // no-crash
+  B &&bcr = C({{}}); // no-crash
+#endif
+}
+}