[cfe-commits] r165079 - in /cfe/trunk: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp test/Analysis/inline.cpp test/Analysis/inlining/InlineObjCInstanceMethod.m
NAKAMURA Takumi
geek4civic at gmail.com
Tue Oct 2 19:43:54 PDT 2012
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