[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 ↗

(On Diff #66315)

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 ↗

(On Diff #66315)

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

[xazax.hun]

lib/StaticAnalyzer/Core/ExprEngineC.cpp

423 ↗

(On Diff #66315)

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 ↗

(On Diff #66315)

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 ↗

(On Diff #66315)

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?

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h

public:
 
   /// Evaluates a chain of derived-to-base casts through the specified path.
   SVal evalDerivedToBase(SVal Derived, const CXXBasePath &CastPath);
 
   /// Evaluates a derived-to-base cast through a single level of derivation.
   SVal evalDerivedToBase(SVal Derived, QualType DerivedPtrType,
                          bool IsVirtual);
 
-  /// \brief Evaluates C++ dynamic_cast cast.
+  /// \brief Attempts to do a down cast. Used to model BaseToDerived and C++
+  ///        dynamic_cast.
   /// The callback may result in the following 3 scenarios:
   ///  - Successful cast (ex: derived is subclass of base).
   ///  - Failed cast (ex: derived is definitely not a subclass of base).
+  ///    The distinction of this case from the next one is necessary to model
+  ///    dynamic_cast. 
   ///  - We don't know (base is a symbolic region and we don't have 
   ///    enough info to determine if the cast will succeed at run time).
   /// The function returns an SVal representing the derived class; it's
   /// valid only if Failed flag is set to false.
-  SVal evalDynamicCast(SVal Base, QualType DerivedPtrType, bool &Failed);
+  SVal attemptDownCast(SVal Base, QualType DerivedPtrType, bool &Failed);
 
   const ElementRegion *GetElementZeroRegion(const MemRegion *R, QualType T);
 
   /// castRegion - Used by ExprEngine::VisitCast to handle casts from
   ///  a MemRegion* to a specific location type.  'R' is the region being
   ///  casted and 'CastToTy' the result type of the cast.
   const MemRegion *castRegion(const MemRegion *region, QualType CastToTy);
 

cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp

if (!ThisVal.isUnknown()) {
     // we have the proper layering of CXXBaseObjectRegions.
     if (MD->getCanonicalDecl() != getDecl()->getCanonicalDecl()) {
       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().evalDynamicCast(ThisVal, Ty, Failed);
+      ThisVal = StateMgr.getStoreManager().attemptDownCast(ThisVal, Ty, Failed);
       assert(!Failed && "Calling an incorrectly devirtualized method");
     }
 
     if (!ThisVal.isUnknown())
       Bindings.push_back(std::make_pair(ThisLoc, ThisVal));
   }
 }
 

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp

switch (CastE->getCastKind()) {
 
         bool Failed = false;
 
         // Check if the value being cast evaluates to 0.
         if (val.isZeroConstant())
           Failed = true;
         // Else, evaluate the cast.
         else
-          val = getStoreManager().evalDynamicCast(val, T, Failed);
+          val = getStoreManager().attemptDownCast(val, T, Failed);
 
         if (Failed) {
           if (T->isReferenceType()) {
             // A bad_cast exception is thrown if input value is a reference.
             // Currently, we model this, by generating a sink.
             Bldr.generateSink(CastE, Pred, state);
             continue;
           } else {
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.isConstant()) {
+          val = getStoreManager().attemptDownCast(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);

cfe/trunk/lib/StaticAnalyzer/Core/Store.cpp

 static const CXXRecordDecl *getCXXRecordType(const MemRegion *MR) {
   if (const TypedValueRegion *TVR = dyn_cast<TypedValueRegion>(MR))
     return TVR->getValueType()->getAsCXXRecordDecl();
   if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(MR))
     return SR->getSymbol()->getType()->getPointeeCXXRecordDecl();
   return nullptr;
 }
 
-SVal StoreManager::evalDynamicCast(SVal Base, QualType TargetType,
+SVal StoreManager::attemptDownCast(SVal Base, QualType TargetType,
                                    bool &Failed) {
   Failed = false;
 
   const MemRegion *MR = Base.getAsRegion();
   if (!MR)
     return UnknownVal();
 
   // Assume the derived class is a pointer or a reference to a CXX record.

cfe/trunk/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;
+}