[cfe-commits] r160767 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp test/Analysis/misc-ps-cxx0x.cpp

[Ted Kremenek]

On Jul 25, 2012, at 9:15 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> Nice. So that's what's going on with CallAndMessageChecker!
> 
> On Jul 25, 2012, at 5:22 PM, Ted Kremenek <kremenek at apple.com> wrote:
> 
>> Author: kremenek
>> Date: Wed Jul 25 19:22:32 2012
>> New Revision: 160767
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=160767&view=rev
>> Log:
>> Add static analyzer check for calling a C++ instance method with a null/uninitialized pointer.
>> 
>> Modified:
>>   cfe/trunk/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
>>   cfe/trunk/test/Analysis/misc-ps-cxx0x.cpp
>> 
>> Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp?rev=160767&r1=160766&r2=160767&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp (original)
>> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp Wed Jul 25 19:22:32 2012
>> @@ -31,6 +31,8 @@
>>                    check::PreCall > {
>>  mutable OwningPtr<BugType> BT_call_null;
>>  mutable OwningPtr<BugType> BT_call_undef;
>> +  mutable OwningPtr<BugType> BT_cxx_call_null;
>> +  mutable OwningPtr<BugType> BT_cxx_call_undef;
>>  mutable OwningPtr<BugType> BT_call_arg;
>>  mutable OwningPtr<BugType> BT_msg_undef;
>>  mutable OwningPtr<BugType> BT_objc_prop_undef;
>> @@ -239,14 +241,35 @@
>> 
>> void CallAndMessageChecker::checkPreCall(const CallEvent &Call,
>>                                         CheckerContext &C) const {
>> +  // If this is a call to a C++ method, check if the callee is null or
>> +  // undefined.
>> +  // FIXME: Generalize this to CXXInstanceCall once it supports
>> +  // getCXXThisVal().
> 
> Right now, all CallEvents support getCXXThisVal(); it's type-dispatched. But outside of CXXInstanceCall, "UndefinedVal" is a placeholder for "no 'this' object". Any CXXInstanceCall should have a valid this-val though.

Instead of using UndefinedVal as a placeholder, shouldn't we use something like llvm::Optional for the return type?  I don't like conflating concepts this way.
> 
> 
>> +  if (const CXXMemberCall *CC = dyn_cast<CXXMemberCall>(&Call)) {
>> +    SVal V = CC->getCXXThisVal();
>> +    if (V.isUndef()) {
>> +      if (!BT_cxx_call_undef)
>> +        BT_cxx_call_undef.reset(new BuiltinBug("Called C++ object pointer is "
>> +                                               "uninitialized"));
>> +      EmitBadCall(BT_cxx_call_undef.get(), C, CC->getOriginExpr());
>> +      return;
>> +    }
>> +    if (V.isZeroConstant()) {
>> +      if (!BT_cxx_call_null)
>> +        BT_cxx_call_null.reset(new BuiltinBug("Called C++ object pointer "
>> +                                              "is null"));
>> +      EmitBadCall(BT_cxx_call_null.get(), C, CC->getOriginExpr());
>> +      return;
>> +    }
>> +  }
>> +
> 
> This is probably the place to assume V is non-null if we call a method on it. Do we want to do that?

Yes we should.  One problem that I have observed is that we don't do this consistently in all places (and we do check fro null in multiple places in the corpus of checkers).  It might be worth refactoring this common logic into an API that all of the checkers can share.  This also feels like a lot of boilerplate.

> 
>>  // Don't check for uninitialized field values in arguments if the
>>  // caller has a body that is available and we have the chance to inline it.
>>  // This is a hack, but is a reasonable compromise betweens sometimes warning
>>  // and sometimes not depending on if we decide to inline a function.
>>  const Decl *D = Call.getDecl();
>>  const bool checkUninitFields =
>> -    !(C.getAnalysisManager().shouldInlineCall() &&
>> -      (D && D->getBody()));
>> +    !(C.getAnalysisManager().shouldInlineCall() && (D && D->getBody())); 
> 
> Yeah, this should be refactored into CallEvent anyway, either CallEvent::mayBeInlined or CallEvent::willBeInlined. The test is wrong because a CallEvent's decl is often not the decl with the definition.

Interesting.  All I did was change the indentation.  Perhaps something like "Call.getDefininingDecl()" which returns the correct Decl* if it exists?