[cfe-commits] r165079 - in /cfe/trunk: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp test/Analysis/inline.cpp test/Analysis/inlining/InlineObjCInstanceMethod.m
Jordan Rose
jordan_rose at apple.com
Wed Oct 3 09:02:37 PDT 2012
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 <geek4civic at gmail.com> 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 <jordan_rose at apple.com>:
>> 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
>> +}
>> +
>> +
>> + at interface EvilChild : MyParent
>> +- (id)getInt;
>> + at end
>> +
>> + at implementation EvilChild
>> +- (id)getInt { // expected-warning {{types are incompatible}}
>> + return self;
>> +}
>> + at 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
>> +}
More information about the cfe-commits
mailing list