[cfe-commits] r161357 - in /cfe/trunk: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h lib/StaticAnalyzer/Checkers/CMakeLists.txt lib/StaticAnalyzer/Checkers/Checkers.td lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp lib/StaticAnalyzer/Core/CallEvent.cpp lib/StaticAnalyzer/Core/ExprEngineC.cpp lib/StaticAnalyzer/Core/ProgramState.cpp test/Analysis/inlining/InlineObjCInstanceMethod.m test/Analysis/inlining/ObjCDynTypePopagation.m

[Jordan Rose]

Nice refactoring. I was a little iffy at first but actually it's probably a good idea to pull as much functionality out of ExprEngine as possible. Comments inline.


> +void DynamicTypePropagation::checkPostCall(const CallEvent &Call,
> +                                           CheckerContext &C) const {
> +  // We can obtain perfect type info for return values from some calls.
> +  if (const ObjCMethodCall *Msg = dyn_cast<ObjCMethodCall>(&Call)) {
> +
> +    // Get the returned value if it's a region.
> +    SVal Result = C.getSVal(Call.getOriginExpr());
> +    const MemRegion *RetReg = Result.getAsRegion();
> +    if (!RetReg)
> +      return;
> +
> +    ProgramStateRef State = C.getState();
> +
> +    switch (Msg->getMethodFamily()) {
> +    default:
> +      break;
> +
> +    // We assume that the type of the object returned by alloc and new are the
> +    // pointer to the object of the class specified in the receiver of the
> +    // message.
> +    case OMF_alloc:
> +    case OMF_new: {
> +      // Get the type of object that will get created.
> +      const ObjCMessageExpr *MsgE = Msg->getOriginExpr();
> +      const ObjCObjectType *ObjTy = getObjectTypeForAllocAndNew(MsgE, C);
> +      if (!ObjTy)
> +        return;
> +      QualType DynResTy =
> +                 C.getASTContext().getObjCObjectPointerType(QualType(ObjTy, 0));
> +      C.addTransition(State->addDynamicTypeInfo(RetReg, DynResTy));
> +      break;
> +    }
> +    case OMF_init: {
> +      // Assume, the result of the init method has the same dynamic type as
> +      // the receiver and propagate the dynamic type info.
> +      const MemRegion *RecReg = Msg->getReceiverSVal().getAsRegion();
> +      assert(RecReg);

This isn't 100% safe; we should assume UnknownVal can come up pretty much anywhere. Or 'nil', for that matter, if we ever change our nil-handling logic. Can we put an actual test here?

(But this is a clever way to get the type from +alloc. I like it.)


> +const ObjCObjectType *
> +DynamicTypePropagation::getObjectTypeForAllocAndNew(const ObjCMessageExpr *MsgE,
> +                                                    CheckerContext &C) const {
> +  if (MsgE->getReceiverKind() == ObjCMessageExpr::Class) {
> +    if (const ObjCObjectType *ObjTy
> +          = MsgE->getClassReceiver()->getAs<ObjCObjectType>())
> +    return ObjTy;
> +  }
> +
> +  if (MsgE->getReceiverKind() == ObjCMessageExpr::SuperClass) {
> +    if (const ObjCObjectType *ObjTy
> +          = MsgE->getSuperType()->getAs<ObjCObjectType>())
> +      return ObjTy;
> +  }
> +
> +  const Expr *RecE = MsgE->getInstanceReceiver();
> +  if (!RecE)
> +    return 0;
> +
> +  RecE= RecE->IgnoreParenImpCasts();
> +  if (const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(RecE)) {
> +    const StackFrameContext *SFCtx = C.getCurrentStackFrame();
> +    // Are we calling [self alloc]? If this is self, get the type of the
> +    // enclosing ObjC class.
> +    if (DRE->getDecl() == SFCtx->getSelfDecl()) {
> +      if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(SFCtx->getDecl()))
> +        if (const ObjCObjectType *ObjTy =
> +            dyn_cast<ObjCObjectType>(MD->getClassInterface()->getTypeForDecl()))
> +          return ObjTy;
> +    }
> +  }
> +  return 0;
> +}

Eventually this should handle arbitrary things like this:

void test(bool coin) {
  Class AllocClass = (coin ? [NSObject class] : [NSArray class]).
  id x = [[AllocClass alloc] init];
}

But in order to do that we need a MemRegion for class types. So it's okay to get back to this later.


Also, the counter-example here is this:

@implementation NSSet
+ (void)setWithArray:(NSArray *)array {
  return [[[self alloc] initWithArray:array] autorelease];
}
@end

NSMutableSet *makeSet(NSArray *array) {
  return [NSMutableSet setWithArray:array]; // calls +[NSSet setWithArray:], but returns an NSMutableSet.
}

I'm not sure if that means we just shouldn't record dynamic type info from class calls to 'self', or if we should only do it if we start tracking class regions (i.e. make this equivalent to the example above), or if we just shouldn't care. I'm actually inclined to go with the first one, simply because the user probably wrote 'self' because they want to allow the behavior to be different when a subclass is around. (They could have just written the class name.)


> -      DynamicTypeInfo TI = getState()->getDynamicTypeInfo(Receiver);
> -      ReceiverT = dyn_cast<ObjCObjectPointerType>(TI.getType().getTypePtr());
> +      QualType DynType = getState()->getDynamicTypeInfo(Receiver).getType();
> +      ReceiverT = dyn_cast<ObjCObjectPointerType>(DynType.getTypePtr());

Only noticing this now, but you can dyn_cast through a QualType without saying 'getTypePtr'.


> + id a = [self alloc];
> + clang_analyzer_eval([a getZeroOverridden] == 0); // expected-warning{{TRUE}}
> +}

Yuck…completely off-topic, but we should have a warning for calling methods on an object before you call an -init method.