NoQ created this revision.
Herald added subscribers: cfe-commits, rnkovacs.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D40841

Files:
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  test/Analysis/initializer.cpp

Index: test/Analysis/initializer.cpp
===================================================================
--- test/Analysis/initializer.cpp
+++ test/Analysis/initializer.cpp
@@ -1,4 +1,5 @@
 // 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);
 
@@ -220,3 +221,40 @@
   C c{x}; // no-warning
 }
 }
+
+namespace CXX17_default_brace_syntax {
+struct A {
+  A();
+};
+
+struct B: public A {
+};
+
+struct C: public B {
+};
+
+struct D: public virtual A {
+};
+
+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 = {};
+
+#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
+}
+}
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -14,6 +14,7 @@
 #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"
@@ -277,6 +278,19 @@
     }
     // FALLTHROUGH
   case CXXConstructExpr::CK_NonVirtualBase:
+    // FIXME: support C++17 construction into initializer lists. Instead of the
+    // incorrect temporary region we use as a placeholder here, we should figure
+    // out what region is being initialized by the surrounding InitListExpr and
+    // construct into the relevant part of it. We might as well model the
+    // trivial constructor call that's missing in the AST and CFG in this case
+    // (which is why we cannot simply get the target region by sub-regioning the
+    // this-region of the parent stack frame).
+    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,
Index: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
+++ lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
@@ -22,6 +22,7 @@
 //===----------------------------------------------------------------------===//
 
 #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"
@@ -262,8 +263,14 @@
       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: Support C++17 constructors into initializer lists.
+        if (dyn_cast_or_null<InitListExpr>(
+                LCtx->getParentMap().getParent(Ctor->getOriginExpr())))
+          return;
+
+        recordFixedType(Target, cast<CXXConstructorDecl>(LCtx->getDecl()), C);
       }
       return;
     }
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to