Jordan, I have reverted a test, in r165088, to unbreak builds. error: 'warning' diagnostics expected but not seen: Line 94: types are incompatible 1 error generated.
...Takumi 2012/10/3 Jordan Rose <[email protected]>: > Author: jrose > Date: Tue Oct 2 20:08:35 2012 > New Revision: 165079 > > URL: http://llvm.org/viewvc/llvm-project?rev=165079&view=rev > Log: > [analyzer] Adjust the return type of an inlined devirtualized method call. > > In C++, overriding virtual methods are allowed to specify a covariant > return type -- that is, if the return type of the base method is an > object pointer type (or reference type), the overriding method's return > type can be a pointer to a subclass of the original type. The analyzer > was failing to take this into account when devirtualizing a method call, > and anything that relied on the return value having the proper type later > would crash. > > In Objective-C, overriding methods are allowed to specify ANY return type, > meaning we can NEVER be sure that devirtualizing will give us a "safe" > return value. Of course, a program that does this will most likely crash > at runtime, but the analyzer at least shouldn't crash. > > The solution is to check and see if the function/method being inlined is > the function that static binding would have picked. If not, check that > the return value has the same type. If the types don't match, see if we > can fix it with a derived-to-base cast (the C++ case). If we can't, > return UnknownVal to avoid crashing later. > > <rdar://problem/12409977> > > Modified: > cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp > cfe/trunk/test/Analysis/inline.cpp > cfe/trunk/test/Analysis/inlining/InlineObjCInstanceMethod.m > > Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp?rev=165079&r1=165078&r2=165079&view=diff > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp (original) > +++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp Tue Oct 2 > 20:08:35 2012 > @@ -17,6 +17,7 @@ > #include "clang/StaticAnalyzer/Core/CheckerManager.h" > #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h" > #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" > +#include "clang/AST/CXXInheritance.h" > #include "clang/AST/DeclCXX.h" > #include "clang/AST/ParentMap.h" > #include "llvm/ADT/SmallSet.h" > @@ -118,6 +119,44 @@ > return std::pair<const Stmt*, const CFGBlock*>(S, Blk); > } > > +/// Adjusts a return value when the called function's return type does not > +/// match the caller's expression type. This can happen when a dynamic call > +/// is devirtualized, and the overridding method has a covariant (more > specific) > +/// return type than the parent's method. For C++ objects, this means we need > +/// to add base casts. > +static SVal adjustReturnValue(SVal V, QualType ExpectedTy, QualType ActualTy, > + StoreManager &StoreMgr) { > + // For now, the only adjustments we handle apply only to locations. > + if (!isa<Loc>(V)) > + return V; > + > + // If the types already match, don't do any unnecessary work. > + if (ExpectedTy == ActualTy) > + return V; > + > + // No adjustment is needed between Objective-C pointer types. > + if (ExpectedTy->isObjCObjectPointerType() && > + ActualTy->isObjCObjectPointerType()) > + return V; > + > + // C++ object pointers may need "derived-to-base" casts. > + const CXXRecordDecl *ExpectedClass = ExpectedTy->getPointeeCXXRecordDecl(); > + const CXXRecordDecl *ActualClass = ActualTy->getPointeeCXXRecordDecl(); > + if (ExpectedClass && ActualClass) { > + CXXBasePaths Paths(/*FindAmbiguities=*/true, /*RecordPaths=*/true, > + /*DetectVirtual=*/false); > + if (ActualClass->isDerivedFrom(ExpectedClass, Paths) && > + !Paths.isAmbiguous(ActualTy->getCanonicalTypeUnqualified())) { > + return StoreMgr.evalDerivedToBase(V, Paths.front()); > + } > + } > + > + // Unfortunately, Objective-C does not enforce that overridden methods have > + // covariant return types, so we can't assert that that never happens. > + // Be safe and return UnknownVal(). > + return UnknownVal(); > +} > + > /// The call exit is simulated with a sequence of nodes, which occur between > /// CallExitBegin and CallExitEnd. The following operations occur between the > /// two program points: > @@ -144,6 +183,11 @@ > const CFGBlock *Blk = 0; > llvm::tie(LastSt, Blk) = getLastStmt(CEBNode); > > + // Generate a CallEvent /before/ cleaning the state, so that we can get the > + // correct value for 'this' (if necessary). > + CallEventManager &CEMgr = getStateManager().getCallEventManager(); > + CallEventRef<> Call = CEMgr.getCaller(calleeCtx, state); > + > // Step 2: generate node with bound return value: CEBNode -> BindedRetNode. > > // If the callee returns an expression, bind its value to CallExpr. > @@ -151,6 +195,18 @@ > if (const ReturnStmt *RS = dyn_cast_or_null<ReturnStmt>(LastSt)) { > const LocationContext *LCtx = CEBNode->getLocationContext(); > SVal V = state->getSVal(RS, LCtx); > + > + const Decl *Callee = calleeCtx->getDecl(); > + if (Callee != Call->getDecl()) { > + QualType ReturnedTy = CallEvent::getDeclaredResultType(Callee); > + if (!ReturnedTy.isNull()) { > + if (const Expr *Ex = dyn_cast<Expr>(CE)) { > + V = adjustReturnValue(V, Ex->getType(), ReturnedTy, > + getStoreManager()); > + } > + } > + } > + > state = state->BindExpr(CE, callerCtx, V); > } > > @@ -172,11 +228,6 @@ > } > } > > - // Generate a CallEvent /before/ cleaning the state, so that we can get the > - // correct value for 'this' (if necessary). > - CallEventManager &CEMgr = getStateManager().getCallEventManager(); > - CallEventRef<> Call = CEMgr.getCaller(calleeCtx, state); > - > // Step 3: BindedRetNode -> CleanedNodes > // If we can find a statement and a block in the inlined function, run > remove > // dead bindings before returning from the call. This is important to > ensure > > Modified: cfe/trunk/test/Analysis/inline.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/inline.cpp?rev=165079&r1=165078&r2=165079&view=diff > ============================================================================== > --- cfe/trunk/test/Analysis/inline.cpp (original) > +++ cfe/trunk/test/Analysis/inline.cpp Tue Oct 2 20:08:35 2012 > @@ -340,3 +340,30 @@ > clang_analyzer_eval(object->A::getZero() == 0); // > expected-warning{{TRUE}} > } > } > + > + > +namespace rdar12409977 { > + struct Base { > + int x; > + }; > + > + struct Parent : public Base { > + virtual Parent *vGetThis(); > + Parent *getThis() { return vGetThis(); } > + }; > + > + struct Child : public Parent { > + virtual Child *vGetThis() { return this; } > + }; > + > + void test() { > + Child obj; > + obj.x = 42; > + > + // Originally, calling a devirtualized method with a covariant return > type > + // caused a crash because the return value had the wrong type. When we > then > + // go to layer a CXXBaseObjectRegion on it, the base isn't a direct base > of > + // the object region and we get an assertion failure. > + clang_analyzer_eval(obj.getThis()->x == 42); // expected-warning{{TRUE}} > + } > +} > > Modified: cfe/trunk/test/Analysis/inlining/InlineObjCInstanceMethod.m > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/inlining/InlineObjCInstanceMethod.m?rev=165079&r1=165078&r2=165079&view=diff > ============================================================================== > --- cfe/trunk/test/Analysis/inlining/InlineObjCInstanceMethod.m (original) > +++ cfe/trunk/test/Analysis/inlining/InlineObjCInstanceMethod.m Tue Oct 2 > 20:08:35 2012 > @@ -83,4 +83,29 @@ > // Don't crash if we don't know the receiver's region. > void randomlyMessageAnObject(MyClass *arr[], int i) { > (void)[arr[i] getInt]; > -} > \ No newline at end of file > +} > + > + > +@interface EvilChild : MyParent > +- (id)getInt; > +@end > + > +@implementation EvilChild > +- (id)getInt { // expected-warning {{types are incompatible}} > + return self; > +} > +@end > + > +int testNonCovariantReturnType() { > + MyParent *obj = [[EvilChild alloc] init]; > + > + // Devirtualization allows us to directly call -[EvilChild getInt], but > + // that returns an id, not an int. There is an off-by-default warning for > + // this, -Woverriding-method-mismatch, and an on-by-default analyzer > warning, > + // osx.cocoa.IncompatibleMethodTypes. This code would probably crash at > + // runtime, but at least the analyzer shouldn't crash. > + int x = 1 + [obj getInt]; > + > + [obj release]; > + return 5/(x-1); // no-warning > +} _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
