I ran the TOT analyzer over WebKit and looks like the null reference returned + null reference as argument changes from yesterday resulted in 45% of warnings being removed(226 ->148). I did not look at the diff, but most warnings were false positives to begin with, so this probably means that our suppression mechanisms/bug reporter did not track references to null pointers correctly. (This also explains why spurious "null pointer dereferences" were more of a problem for C++ codebases.) Cheers, Anna. On Mar 6, 2013, at 7:09 PM, Jordan Rose <[email protected]> wrote:
> For the record, it's unclear whether the standard actually allows this. Null > lvalues seem to be allowed, while null references seem to be disallowed, but > neither is definitively in or out. > > http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#232 > > In the analyzer, as Anna says, we're going to take a hard line and guess > people would rather be warned. > > > On Mar 6, 2013, at 19:02 , Anna Zaks <[email protected]> wrote: > >> Author: zaks >> Date: Wed Mar 6 21:02:36 2013 >> New Revision: 176612 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=176612&view=rev >> Log: >> [analyzer] Warn on passing a reference to null pointer as an argument in a >> call >> >> Warn about null pointer dereference earlier when a reference to a null >> pointer is >> passed in a call. The idea is that even though the standard might allow >> this, reporting >> the issue earlier is better for diagnostics (the error is reported closer to >> the place where >> the pointer was set to NULL). This also simplifies analyzer’s diagnostic >> logic, which has >> to track “where the null came from”. As a consequence, some of our null >> pointer >> warning suppression mechanisms started triggering more often. >> >> TODO: Change the name of the file and class to reflect the new check. >> >> Modified: >> cfe/trunk/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp >> cfe/trunk/test/Analysis/initializer.cpp >> cfe/trunk/test/Analysis/reference.cpp >> >> Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp?rev=176612&r1=176611&r2=176612&view=diff >> ============================================================================== >> --- cfe/trunk/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp (original) >> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp Wed Mar 6 >> 21:02:36 2013 >> @@ -26,10 +26,16 @@ using namespace ento; >> namespace { >> class AttrNonNullChecker >> : public Checker< check::PreCall > { >> - mutable OwningPtr<BugType> BT; >> + mutable OwningPtr<BugType> BTAttrNonNull; >> + mutable OwningPtr<BugType> BTNullRefArg; >> public: >> >> void checkPreCall(const CallEvent &Call, CheckerContext &C) const; >> + >> + BugReport *genReportNullAttrNonNull(const ExplodedNode *ErrorN, >> + const Expr *ArgE) const; >> + BugReport *genReportReferenceToNullPointer(const ExplodedNode *ErrorN, >> + const Expr *ArgE) const; >> }; >> } // end anonymous namespace >> >> @@ -40,26 +46,38 @@ void AttrNonNullChecker::checkPreCall(co >> return; >> >> const NonNullAttr *Att = FD->getAttr<NonNullAttr>(); >> - if (!Att) >> - return; >> >> ProgramStateRef state = C.getState(); >> >> - // Iterate through the arguments of CE and check them for null. >> - for (unsigned idx = 0, count = Call.getNumArgs(); idx != count; ++idx) { >> - if (!Att->isNonNull(idx)) >> + CallEvent::param_type_iterator TyI = Call.param_type_begin(), >> + TyE = Call.param_type_end(); >> + >> + for (unsigned idx = 0, count = Call.getNumArgs(); idx != count; ++idx){ >> + >> + // Check if the parameter is a reference. We want to report when >> reference >> + // to a null pointer is passed as a paramter. >> + bool haveRefTypeParam = false; >> + if (TyI != TyE) { >> + haveRefTypeParam = (*TyI)->isReferenceType(); >> + TyI++; >> + } >> + >> + bool haveAttrNonNull = Att && Att->isNonNull(idx); >> + >> + if (!haveRefTypeParam && !haveAttrNonNull) >> continue; >> >> + // If the value is unknown or undefined, we can't perform this check. >> + const Expr *ArgE = Call.getArgExpr(idx); >> SVal V = Call.getArgSVal(idx); >> Optional<DefinedSVal> DV = V.getAs<DefinedSVal>(); >> - >> - // If the value is unknown or undefined, we can't perform this check. >> if (!DV) >> continue; >> >> - const Expr *ArgE = Call.getArgExpr(idx); >> - >> - if (!DV->getAs<Loc>()) { >> + // Process the case when the argument is not a location. >> + assert(!haveRefTypeParam || DV->getAs<Loc>()); >> + >> + if (haveAttrNonNull && !DV->getAs<Loc>()) { >> // If the argument is a union type, we want to handle a potential >> // transparent_union GCC extension. >> if (!ArgE) >> @@ -100,21 +118,15 @@ void AttrNonNullChecker::checkPreCall(co >> // we cache out. >> if (ExplodedNode *errorNode = C.generateSink(stateNull)) { >> >> - // Lazily allocate the BugType object if it hasn't already been >> - // created. Ownership is transferred to the BugReporter object once >> - // the BugReport is passed to 'EmitWarning'. >> - if (!BT) >> - BT.reset(new BugType("Argument with 'nonnull' attribute passed >> null", >> - "API")); >> - >> - BugReport *R = >> - new BugReport(*BT, "Null pointer passed as an argument to a " >> - "'nonnull' parameter", errorNode); >> + BugReport *R = 0; >> + if (haveAttrNonNull) >> + R = genReportNullAttrNonNull(errorNode, ArgE); >> + else if (haveRefTypeParam) >> + R = genReportReferenceToNullPointer(errorNode, ArgE); >> >> // Highlight the range of the argument that was null. >> R->addRange(Call.getArgSourceRange(idx)); >> - if (ArgE) >> - bugreporter::trackNullOrUndefValue(errorNode, ArgE, *R); >> + >> // Emit the bug report. >> C.emitReport(R); >> } >> @@ -134,6 +146,45 @@ void AttrNonNullChecker::checkPreCall(co >> C.addTransition(state); >> } >> >> +BugReport *AttrNonNullChecker::genReportNullAttrNonNull( >> + const ExplodedNode *ErrorNode, const Expr *ArgE) const { >> + // Lazily allocate the BugType object if it hasn't already been >> + // created. Ownership is transferred to the BugReporter object once >> + // the BugReport is passed to 'EmitWarning'. >> + if (!BTAttrNonNull) >> + BTAttrNonNull.reset(new BugType( >> + "Argument with 'nonnull' attribute passed null", >> + "API")); >> + >> + BugReport *R = new BugReport(*BTAttrNonNull, >> + "Null pointer passed as an argument to a 'nonnull' >> parameter", >> + ErrorNode); >> + if (ArgE) >> + bugreporter::trackNullOrUndefValue(ErrorNode, ArgE, *R); >> + >> + return R; >> +} >> + >> +BugReport *AttrNonNullChecker::genReportReferenceToNullPointer( >> + const ExplodedNode *ErrorNode, const Expr *ArgE) const { >> + if (!BTNullRefArg) >> + BTNullRefArg.reset(new BuiltinBug("Dereference of null pointer")); >> + >> + BugReport *R = new BugReport(*BTNullRefArg, >> + "Forming reference to null pointer", >> + ErrorNode); >> + if (ArgE) { >> + const Expr *ArgEDeref = bugreporter::getDerefExpr(ArgE); >> + if (ArgEDeref == 0) >> + ArgEDeref = ArgE; >> + bugreporter::trackNullOrUndefValue(ErrorNode, >> + ArgEDeref, >> + *R); >> + } >> + return R; >> + >> +} >> + >> void ento::registerAttrNonNullChecker(CheckerManager &mgr) { >> mgr.registerChecker<AttrNonNullChecker>(); >> } >> >> Modified: cfe/trunk/test/Analysis/initializer.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/initializer.cpp?rev=176612&r1=176611&r2=176612&view=diff >> ============================================================================== >> --- cfe/trunk/test/Analysis/initializer.cpp (original) >> +++ cfe/trunk/test/Analysis/initializer.cpp Wed Mar 6 21:02:36 2013 >> @@ -68,8 +68,7 @@ void testReferenceMember() { >> >> void testReferenceMember2() { >> int *p = 0; >> - // FIXME: We should warn here, since we're creating the reference here. >> - RefWrapper X(*p); // expected-warning@-12 {{Dereference of null pointer}} >> + RefWrapper X(*p); // expected-warning {{Forming reference to null >> pointer}} >> } >> >> >> >> Modified: cfe/trunk/test/Analysis/reference.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/reference.cpp?rev=176612&r1=176611&r2=176612&view=diff >> ============================================================================== >> --- cfe/trunk/test/Analysis/reference.cpp (original) >> +++ cfe/trunk/test/Analysis/reference.cpp Wed Mar 6 21:02:36 2013 >> @@ -149,16 +149,78 @@ void testReturnReference() { >> clang_analyzer_eval(&refFromPointer() == 0); // expected-warning{{FALSE}} >> } >> >> +void intRefParam(int &r) { >> + ; >> +} >> + >> +void test(int *ptr) { >> + clang_analyzer_eval(ptr == 0); // expected-warning{{UNKNOWN}} >> + >> + extern void use(int &ref); >> + use(*ptr); >> + >> + clang_analyzer_eval(ptr == 0); // expected-warning{{FALSE}} >> +} >> + >> +void testIntRefParam() { >> + int i = 0; >> + intRefParam(i); // no-warning >> +} >> >> -// ------------------------------------ >> -// False negatives >> -// ------------------------------------ >> +int refParam(int &byteIndex) { >> + return byteIndex; >> +} >> + >> +void testRefParam(int *p) { >> + if (p) >> + ; >> + refParam(*p); // expected-warning {{Forming reference to null pointer}} >> +} >> + >> +int ptrRefParam(int *&byteIndex) { >> + return *byteIndex; // expected-warning {{Dereference of null pointer}} >> +} >> +void testRefParam2() { >> + int *p = 0; >> + int *&rp = p; >> + ptrRefParam(rp); >> +} >> + >> +int *maybeNull() { >> + extern bool coin(); >> + static int x; >> + return coin() ? &x : 0; >> +} >> + >> +void use(int &x) { >> + x = 1; // no-warning >> +} >> + >> +void testSuppression() { >> + use(*maybeNull()); >> +} >> >> namespace rdar11212286 { >> class B{}; >> >> B test() { >> B *x = 0; >> - return *x; // should warn here! >> + return *x; // expected-warning {{Forming reference to null pointer}} >> + } >> + >> + B testif(B *x) { >> + if (x) >> + ; >> + return *x; // expected-warning {{Forming reference to null pointer}} >> + } >> + >> + void idc(B *x) { >> + if (x) >> + ; >> + } >> + >> + B testidc(B *x) { >> + idc(x); >> + return *x; // no-warning >> } >> } >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
