[analyzer] Model base to derived casts more precisely.

[xazax.hun]

Summary

Dynamic casts are handled relatively well by the static analyzer. BaseToDerived casts however are handled conservatively. This can cause some false positives with the NewDeleteLeaks checker.

This patch alters the behavior of BaseToDerived casts. In case a dynamic cast would succeed use the same semantics. Otherwise fall back to the conservative approach.

A slightly related question: in case a cast on a pointer can not be modelled precisely, maybe that should be handled as a pointer escape to avoid false positives in some cases?

[xazax.hun]

test/Analysis/NewDelete-checker-test.cpp

394

Before the modification the analyzer reports a leak here, since the symbol returned by the BaseToDerived cast is independent of the original symbol.

[NoQ]

lib/StaticAnalyzer/Core/ExprEngineC.cpp

423

I guess if val is a non-zero constant, it wouldn't make much difference.

[xazax.hun]

lib/StaticAnalyzer/Core/ExprEngineC.cpp

423

I might be wrong, but isn't the only valid constant value for a pointer the zero constant?

[NoQ]

lib/StaticAnalyzer/Core/ExprEngineC.cpp

423

Even if forbidden by the Standard in well-formed programs, we'd have to expect it here - after all, it's great if we analyze a program that has a bug :)

We also have this FixedAddressChecker thing, so even in our simplified model, there actually do exist non-zero constant pointers. That said, there's some bug i stepped into in explain-svals.cpp, which produces more zero constant pointers that one would expect, didn't have time to investigate yet...

[dcoughlin]

Other than a naming/documentation suggestion, looks good to me. Thanks Gábor!

lib/StaticAnalyzer/Core/ExprEngineC.cpp

424

It seems a bit weird to call a method named "evalDynamicCast" when handling a static cast. What do you think about renaming evalDynamicCast() to to attemptDowncast() and updating its documentation to reflect its now-dual use?

[xazax.hun]

• Improvements according to review comments.

[xazax.hun]

What do you think about escaping pointers that gone through conservatively evaluated casts?

lib/StaticAnalyzer/Core/ExprEngineC.cpp

switch (CastE->getCastKind()) {
             state = state->BindExpr(CastE, LCtx, NewSym);
           } else
             // Else, bind to the derived region value.
             state = state->BindExpr(CastE, LCtx, val);
         }
         Bldr.generateNode(CastE, Pred, state);
         continue;
       }
+      case CK_BaseToDerived: {
+        SVal val = state->getSVal(Ex, LCtx);
+        QualType resultType = CastE->getType();
+        if (CastE->isGLValue())
+          resultType = getContext().getPointerType(resultType);
+
+        bool Failed = false;
+
+        if (!val.isZeroConstant()) {
+          val = getStoreManager().evalDynamicCast(val, T, Failed);
+        }
+
+        // Failed to cast or the result is unknown, fall back to conservative.
+        if (Failed || val.isUnknown()) {
+          val =
+            svalBuilder.conjureSymbolVal(nullptr, CastE, LCtx, resultType,
+                                         currBldrCtx->blockCount());
+        }
+        state = state->BindExpr(CastE, LCtx, val);
+        Bldr.generateNode(CastE, Pred, state);
+        continue;
+      }
       case CK_NullToMemberPointer: {
         // FIXME: For now, member pointers are represented by void *.
         SVal V = svalBuilder.makeNull();
         state = state->BindExpr(CastE, LCtx, V);
         Bldr.generateNode(CastE, Pred, state);
         continue;
       }
       // Various C++ casts that are not handled yet.
       case CK_ToUnion:
-      case CK_BaseToDerived:
       case CK_BaseToDerivedMemberPointer:
       case CK_DerivedToBaseMemberPointer:
       case CK_ReinterpretMemberPointer:
       case CK_VectorSplat: {
         // Recover some path-sensitivty by conjuring a new value.
         QualType resultType = CastE->getType();
         if (CastE->isGLValue())
           resultType = getContext().getPointerType(resultType);

test/Analysis/NewDelete-checker-test.cpp

public:
   ~EmptyClass() {}
 };
 
 void testDoubleDeleteEmptyClass() {
   EmptyClass *foo = new EmptyClass();
   delete foo;
   delete foo;  // expected-warning {{Attempt to delete released memory}}
 }
+
+struct Base {
+  virtual ~Base() {}
+};
+
+struct Derived : Base {
+};
+
+Base *allocate() {
+  return new Derived;
+}
+
+void shouldNotReportLeak() {
+  Derived *p = (Derived *)allocate();
+  delete p;
+}