[PATCH] D43659: [analyzer] Don't crash when dynamic type of a concrete region is hard-set with placement new.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.

This is assertion removal that i find valid. With placement new (which isn't even need to be inlined, we used to model it conservatively well enough), anything of any type can have any dynamic type. Even if we have a concrete region of a variable, its dynamic type and its static type can be completely unrelated. This might be UB due to strict aliasing rules, but we shouldn't crash. This patch introduces a relatively sane behavior in this scenario that consists of `evalCast()`ing this-region to the assumed dynamic type during virtual calls.

In the common case when the dynamic type is a sub-class of the static type, this is worse than calling `attemptDownCast()` because it adds element region instead of removing base regions (which is not incorrect but produces a non-canonical representation of the SVal). But when the common-case approach is known to have failed, there doesn't seem to be a better option.

A lot of these crashes have suddenly shown up when i was testing temporaries. They have nothing to do with temporaries though, but with a weird implementation detail of `std::function` that suddenly got some if its methods inlined.


Repository:
  rC Clang

https://reviews.llvm.org/D43659

Files:
  lib/StaticAnalyzer/Core/CallEvent.cpp
  test/Analysis/new-dynamic-types.cpp


Index: test/Analysis/new-dynamic-types.cpp
===================================================================
--- /dev/null
+++ test/Analysis/new-dynamic-types.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -std=c++11 -verify %s
+
+// expected-no-diagnostics
+
+typedef __typeof(sizeof(int)) size_t;
+
+void *operator new(size_t size, void *ptr);
+
+struct B {
+  virtual void foo();
+};
+
+struct D : public B {
+  virtual void foo() override {}
+};
+
+void test() {
+  // FIXME: Potentially warn because this code is pretty weird.
+  B b;
+  new (&b) D;
+  b.foo(); // no-crash
+}
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===================================================================
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -583,11 +583,18 @@
       ASTContext &Ctx = SVB.getContext();
       const CXXRecordDecl *Class = MD->getParent();
       QualType Ty = Ctx.getPointerType(Ctx.getRecordType(Class));
-
       // FIXME: CallEvent maybe shouldn't be directly accessing StoreManager.
       bool Failed;
       ThisVal = StateMgr.getStoreManager().attemptDownCast(ThisVal, Ty, Failed);
-      assert(!Failed && "Calling an incorrectly devirtualized method");
+      if (Failed) {
+        // We might have suffered some sort of placement new earlier, so
+        // we're constructing in a completely unexpected storage.
+        // Fall back to a generic pointer cast for this-value.
+        const CXXMethodDecl *StaticMD = cast<CXXMethodDecl>(getDecl());
+        const CXXRecordDecl *StaticClass = StaticMD->getParent();
+        QualType StaticTy = Ctx.getPointerType(Ctx.getRecordType(StaticClass));
+        ThisVal = SVB.evalCast(ThisVal, Ty, StaticTy);
+      }
     }
 
     if (!ThisVal.isUnknown())


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D43659.135563.patch
Type: text/x-patch
Size: 1799 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180223/f06bfee9/attachment-0001.bin>