Thanks, Takumi. My fault for forgetting that the osx checkers are only enabled by default on Darwin (duh). Should be fixed in r165124.
Jordan On Oct 2, 2012, at 19:43 , NAKAMURA Takumi <[email protected]> wrote: > 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
