r328282 - [analyzer] Trust _Nonnull annotations for system framework
Alexander Kornienko via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 28 17:42:56 PDT 2018
It looks like it was fixed, indeed. After an update I don't see an awful
number of crashes any more. Thanks!
On Mon, Mar 26, 2018 at 8:05 PM George Karpenkov <ekarpenkov at apple.com>
wrote:
> Yeah, I’m pretty sure this was fixed on Friday with
> https://reviews.llvm.org/rC328406
>
> On Mar 26, 2018, at 10:54 AM, Alexander Kornienko <alexfh at google.com>
> wrote:
>
> This checker crashes on almost each file in our codebase. No test case
> yet, but here's a stack trace:
> clang::Type::getTypeClass
> clang::ReferenceType::classof
> llvm::isa_impl::doit
> llvm::isa_impl_cl::doit
> llvm::isa_impl_wrap::doit
> llvm::isa_impl_wrap::doit
> llvm::isa
> clang::Type::getAs
> clang::ASTContext::getLValueReferenceType
> ::TrustNonnullChecker::checkPostCall
> clang::ento::check::PostCall::_checkCall
> clang::ento::CheckerFn::operator()
> ::CheckCallContext::runChecker
> expandGraphWithCheckers
> clang::ento::CheckerManager::runCheckersForCallEvent
> clang::ento::CheckerManager::runCheckersForPostCall
> clang::ento::ExprEngine::VisitCXXDestructor
> clang::ento::ExprEngine::ProcessTemporaryDtor
> clang::ento::ExprEngine::ProcessImplicitDtor
> clang::ento::ExprEngine::processCFGElement
> clang::ento::CoreEngine::dispatchWorkItem
> clang::ento::CoreEngine::ExecuteWorkList
> ::AnalysisConsumer::ActionExprEngine
> ::AnalysisConsumer::HandleCode
> ::AnalysisConsumer::HandleTranslationUnit
> clang::MultiplexConsumer::HandleTranslationUnit
> clang::ParseAST
> clang::FrontendAction::Execute
> clang::CompilerInstance::ExecuteAction
> clang::tooling::FrontendActionFactory::runInvocation
> clang::tooling::ToolInvocation::runInvocation
> clang::tooling::ToolInvocation::run
>
> Could you fix or revert the patch?
>
>
> On Fri, Mar 23, 2018 at 1:18 AM George Karpenkov via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> Author: george.karpenkov
>> Date: Thu Mar 22 17:16:03 2018
>> New Revision: 328282
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=328282&view=rev
>> Log:
>> [analyzer] Trust _Nonnull annotations for system framework
>>
>> Changes the analyzer to believe that methods annotated with _Nonnull
>> from system frameworks indeed return non null objects.
>> Local methods with such annotation are still distrusted.
>> rdar://24291919
>>
>> Differential Revision: https://reviews.llvm.org/D44341
>>
>> Added:
>> cfe/trunk/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp
>> cfe/trunk/test/Analysis/trustnonnullchecker_test.m
>> Modified:
>> cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
>>
>> cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
>> cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt
>> cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
>> cfe/trunk/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
>>
>> cfe/trunk/test/Analysis/Inputs/system-header-simulator-for-nullability.h
>>
>> Modified: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td?rev=328282&r1=328281&r2=328282&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td (original)
>> +++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td Thu Mar
>> 22 17:16:03 2018
>> @@ -218,6 +218,14 @@ def NullableReturnedFromNonnullChecker :
>>
>> } // end "nullability"
>>
>> +let ParentPackage = APIModeling in {
>> +
>> +def TrustNonnullChecker : Checker<"TrustNonnull">,
>> + HelpText<"Trust that returns from framework methods annotated with
>> _Nonnull are not null">,
>> + DescFile<"TrustNonnullChecker.cpp">;
>> +
>> +}
>> +
>>
>> //===----------------------------------------------------------------------===//
>> // Evaluate "builtin" functions.
>>
>> //===----------------------------------------------------------------------===//
>>
>> Modified:
>> cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h?rev=328282&r1=328281&r2=328282&view=diff
>>
>> ==============================================================================
>> ---
>> cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
>> (original)
>> +++
>> cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
>> Thu Mar 22 17:16:03 2018
>> @@ -21,6 +21,8 @@ namespace clang {
>>
>> class Expr;
>> class VarDecl;
>> +class QualType;
>> +class AttributedType;
>>
>> namespace ento {
>>
>> @@ -42,6 +44,25 @@ template <class T> bool containsStmt(con
>> std::pair<const clang::VarDecl *, const clang::Expr *>
>> parseAssignment(const Stmt *S);
>>
>> +// Do not reorder! The getMostNullable method relies on the order.
>> +// Optimization: Most pointers expected to be unspecified. When a symbol
>> has an
>> +// unspecified or nonnull type non of the rules would indicate any
>> problem for
>> +// that symbol. For this reason only nullable and contradicted
>> nullability are
>> +// stored for a symbol. When a symbol is already contradicted, it can
>> not be
>> +// casted back to nullable.
>> +enum class Nullability : char {
>> + Contradicted, // Tracked nullability is contradicted by an explicit
>> cast. Do
>> + // not report any nullability related issue for this
>> symbol.
>> + // This nullability is propagated aggressively to avoid
>> false
>> + // positive results. See the comment on getMostNullable
>> method.
>> + Nullable,
>> + Unspecified,
>> + Nonnull
>> +};
>> +
>> +/// Get nullability annotation for a given type.
>> +Nullability getNullabilityAnnotation(QualType Type);
>> +
>> } // end GR namespace
>>
>> } // end clang namespace
>>
>> Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt?rev=328282&r1=328281&r2=328282&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt (original)
>> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt Thu Mar 22
>> 17:16:03 2018
>> @@ -84,6 +84,7 @@ add_clang_library(clangStaticAnalyzerChe
>> TaintTesterChecker.cpp
>> TestAfterDivZeroChecker.cpp
>> TraversalChecker.cpp
>> + TrustNonnullChecker.cpp
>> UndefBranchChecker.cpp
>> UndefCapturedBlockVarChecker.cpp
>> UndefResultChecker.cpp
>>
>> Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp?rev=328282&r1=328281&r2=328282&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
>> (original)
>> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp Thu Mar
>> 22 17:16:03 2018
>> @@ -30,6 +30,7 @@
>> #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
>> #include "clang/StaticAnalyzer/Core/Checker.h"
>> #include "clang/StaticAnalyzer/Core/CheckerManager.h"
>> +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
>> #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
>> #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
>>
>> @@ -40,21 +41,6 @@ using namespace clang;
>> using namespace ento;
>>
>> namespace {
>> -// Do not reorder! The getMostNullable method relies on the order.
>> -// Optimization: Most pointers expected to be unspecified. When a symbol
>> has an
>> -// unspecified or nonnull type non of the rules would indicate any
>> problem for
>> -// that symbol. For this reason only nullable and contradicted
>> nullability are
>> -// stored for a symbol. When a symbol is already contradicted, it can
>> not be
>> -// casted back to nullable.
>> -enum class Nullability : char {
>> - Contradicted, // Tracked nullability is contradicted by an explicit
>> cast. Do
>> - // not report any nullability related issue for this
>> symbol.
>> - // This nullability is propagated aggressively to avoid
>> false
>> - // positive results. See the comment on getMostNullable
>> method.
>> - Nullable,
>> - Unspecified,
>> - Nonnull
>> -};
>>
>> /// Returns the most nullable nullability. This is used for message
>> expressions
>> /// like [receiver method], where the nullability of this expression is
>> either
>> @@ -345,17 +331,6 @@ NullabilityChecker::NullabilityBugVisito
>> nullptr);
>> }
>>
>> -static Nullability getNullabilityAnnotation(QualType Type) {
>> - const auto *AttrType = Type->getAs<AttributedType>();
>> - if (!AttrType)
>> - return Nullability::Unspecified;
>> - if (AttrType->getAttrKind() == AttributedType::attr_nullable)
>> - return Nullability::Nullable;
>> - else if (AttrType->getAttrKind() == AttributedType::attr_nonnull)
>> - return Nullability::Nonnull;
>> - return Nullability::Unspecified;
>> -}
>> -
>> /// Returns true when the value stored at the given location is null
>> /// and the passed in type is nonnnull.
>> static bool checkValueAtLValForInvariantViolation(ProgramStateRef State,
>> @@ -872,7 +847,7 @@ void NullabilityChecker::checkPostObjCMe
>> // are either item retrieval related or not interesting nullability
>> wise.
>> // Using this fact, to keep the code easier to read just ignore the
>> return
>> // value of every instance method of dictionaries.
>> - if (M.isInstanceMessage() && Name.find("Dictionary") !=
>> StringRef::npos) {
>> + if (M.isInstanceMessage() && Name.contains("Dictionary")) {
>> State =
>> State->set<NullabilityMap>(ReturnRegion,
>> Nullability::Contradicted);
>> C.addTransition(State);
>> @@ -880,7 +855,7 @@ void NullabilityChecker::checkPostObjCMe
>> }
>> // For similar reasons ignore some methods of Cocoa arrays.
>> StringRef FirstSelectorSlot = M.getSelector().getNameForSlot(0);
>> - if (Name.find("Array") != StringRef::npos &&
>> + if (Name.contains("Array") &&
>> (FirstSelectorSlot == "firstObject" ||
>> FirstSelectorSlot == "lastObject")) {
>> State =
>> @@ -893,7 +868,7 @@ void NullabilityChecker::checkPostObjCMe
>> // encodings are used. Using lossless encodings is so frequent that
>> ignoring
>> // this class of methods reduced the emitted diagnostics by about
>> 30% on
>> // some projects (and all of that was false positives).
>> - if (Name.find("String") != StringRef::npos) {
>> + if (Name.contains("String")) {
>> for (auto Param : M.parameters()) {
>> if (Param->getName() == "encoding") {
>> State = State->set<NullabilityMap>(ReturnRegion,
>>
>> Added: cfe/trunk/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp?rev=328282&view=auto
>>
>> ==============================================================================
>> --- cfe/trunk/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp (added)
>> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp Thu Mar
>> 22 17:16:03 2018
>> @@ -0,0 +1,52 @@
>> +//== TrustNonnullChecker.cpp - Checker for trusting annotations -*- C++
>> -*--==//
>> +//
>> +// The LLVM Compiler Infrastructure
>> +//
>> +// This file is distributed under the University of Illinois Open Source
>> +// License. See LICENSE.TXT for details.
>> +//
>>
>> +//===----------------------------------------------------------------------===//
>> +//
>> +// This checker adds an assumption that methods annotated with _Nonnull
>> +// which come from system headers actually return a non-null pointer.
>> +//
>>
>> +//===----------------------------------------------------------------------===//
>> +
>> +#include "ClangSACheckers.h"
>> +#include "clang/StaticAnalyzer/Core/Checker.h"
>> +#include "clang/StaticAnalyzer/Core/CheckerManager.h"
>> +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
>> +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
>> +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
>> +
>> +using namespace clang;
>> +using namespace ento;
>> +
>> +namespace {
>> +
>> +class TrustNonnullChecker : public Checker<check::PostCall> {
>> +public:
>> + void checkPostCall(const CallEvent &Call, CheckerContext &C) const {
>> + // Only trust annotations for system headers for non-protocols.
>> + if (!Call.isInSystemHeader())
>> + return;
>> +
>> + QualType RetType = Call.getResultType();
>> + if (!RetType->isAnyPointerType())
>> + return;
>> +
>> + ProgramStateRef State = C.getState();
>> + if (getNullabilityAnnotation(RetType) == Nullability::Nonnull)
>> + if (auto L = Call.getReturnValue().getAs<Loc>())
>> + State = State->assume(*L, /*Assumption=*/true);
>> +
>> + C.addTransition(State);
>> + }
>> +};
>> +
>> +} // end empty namespace
>> +
>> +
>> +void ento::registerTrustNonnullChecker(CheckerManager &Mgr) {
>> + Mgr.registerChecker<TrustNonnullChecker>();
>> +}
>>
>> Modified: cfe/trunk/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/CheckerHelpers.cpp?rev=328282&r1=328281&r2=328282&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/StaticAnalyzer/Core/CheckerHelpers.cpp (original)
>> +++ cfe/trunk/lib/StaticAnalyzer/Core/CheckerHelpers.cpp Thu Mar 22
>> 17:16:03 2018
>> @@ -15,8 +15,12 @@
>> #include "clang/AST/Decl.h"
>> #include "clang/AST/Expr.h"
>>
>> +namespace clang {
>> +
>> +namespace ento {
>> +
>> // Recursively find any substatements containing macros
>> -bool clang::ento::containsMacro(const Stmt *S) {
>> +bool containsMacro(const Stmt *S) {
>> if (S->getLocStart().isMacroID())
>> return true;
>>
>> @@ -31,7 +35,7 @@ bool clang::ento::containsMacro(const St
>> }
>>
>> // Recursively find any substatements containing enum constants
>> -bool clang::ento::containsEnum(const Stmt *S) {
>> +bool containsEnum(const Stmt *S) {
>> const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(S);
>>
>> if (DR && isa<EnumConstantDecl>(DR->getDecl()))
>> @@ -45,7 +49,7 @@ bool clang::ento::containsEnum(const Stm
>> }
>>
>> // Recursively find any substatements containing static vars
>> -bool clang::ento::containsStaticLocal(const Stmt *S) {
>> +bool containsStaticLocal(const Stmt *S) {
>> const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(S);
>>
>> if (DR)
>> @@ -61,7 +65,7 @@ bool clang::ento::containsStaticLocal(co
>> }
>>
>> // Recursively find any substatements containing __builtin_offsetof
>> -bool clang::ento::containsBuiltinOffsetOf(const Stmt *S) {
>> +bool containsBuiltinOffsetOf(const Stmt *S) {
>> if (isa<OffsetOfExpr>(S))
>> return true;
>>
>> @@ -74,7 +78,7 @@ bool clang::ento::containsBuiltinOffsetO
>>
>> // Extract lhs and rhs from assignment statement
>> std::pair<const clang::VarDecl *, const clang::Expr *>
>> -clang::ento::parseAssignment(const Stmt *S) {
>> +parseAssignment(const Stmt *S) {
>> const VarDecl *VD = nullptr;
>> const Expr *RHS = nullptr;
>>
>> @@ -94,3 +98,18 @@ clang::ento::parseAssignment(const Stmt
>>
>> return std::make_pair(VD, RHS);
>> }
>> +
>> +Nullability getNullabilityAnnotation(QualType Type) {
>> + const auto *AttrType = Type->getAs<AttributedType>();
>> + if (!AttrType)
>> + return Nullability::Unspecified;
>> + if (AttrType->getAttrKind() == AttributedType::attr_nullable)
>> + return Nullability::Nullable;
>> + else if (AttrType->getAttrKind() == AttributedType::attr_nonnull)
>> + return Nullability::Nonnull;
>> + return Nullability::Unspecified;
>> +}
>> +
>> +
>> +} // end namespace ento
>> +} // end namespace clang
>>
>> Modified:
>> cfe/trunk/test/Analysis/Inputs/system-header-simulator-for-nullability.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/Inputs/system-header-simulator-for-nullability.h?rev=328282&r1=328281&r2=328282&view=diff
>>
>> ==============================================================================
>> ---
>> cfe/trunk/test/Analysis/Inputs/system-header-simulator-for-nullability.h
>> (original)
>> +++
>> cfe/trunk/test/Analysis/Inputs/system-header-simulator-for-nullability.h
>> Thu Mar 22 17:16:03 2018
>> @@ -32,6 +32,8 @@ NSObject<NSObject>
>> @interface NSString : NSObject<NSCopying>
>> - (BOOL)isEqualToString : (NSString *)aString;
>> - (NSString *)stringByAppendingString:(NSString *)aString;
>> ++ (_Nonnull NSString *) generateString;
>> ++ (_Nullable NSString *) generatePossiblyNullString;
>> @end
>>
>> void NSSystemFunctionTakingNonnull(NSString *s);
>> @@ -40,4 +42,11 @@ void NSSystemFunctionTakingNonnull(NSStr
>> - (void) takesNonnull:(NSString *)s;
>> @end
>>
>> +NSString* _Nullable getPossiblyNullString();
>> +NSString* _Nonnull getString();
>> +
>> + at protocol MyProtocol
>> +- (_Nonnull NSString *) getString;
>> + at end
>> +
>> NS_ASSUME_NONNULL_END
>>
>> Added: cfe/trunk/test/Analysis/trustnonnullchecker_test.m
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/trustnonnullchecker_test.m?rev=328282&view=auto
>>
>> ==============================================================================
>> --- cfe/trunk/test/Analysis/trustnonnullchecker_test.m (added)
>> +++ cfe/trunk/test/Analysis/trustnonnullchecker_test.m Thu Mar 22
>> 17:16:03 2018
>> @@ -0,0 +1,43 @@
>> +// RUN: %clang_analyze_cc1 -fblocks -analyze
>> -analyzer-checker=core,nullability,apiModeling -verify %s
>> +
>> +#include "Inputs/system-header-simulator-for-nullability.h"
>> +
>> +NSString* getUnknownString();
>> +
>> +NSString* _Nonnull trust_nonnull_framework_annotation() {
>> + NSString* out = [NSString generateString];
>> + if (out) {}
>> + return out; // no-warning
>> +}
>> +
>> +NSString* _Nonnull distrust_without_annotation() {
>> + NSString* out = [NSString generatePossiblyNullString];
>> + if (out) {}
>> + return out; // expected-warning{{}}
>> +}
>> +
>> +NSString* _Nonnull nonnull_please_trust_me();
>> +
>> +NSString* _Nonnull distrust_local_nonnull_annotation() {
>> + NSString* out = nonnull_please_trust_me();
>> + if (out) {}
>> + return out; // expected-warning{{}}
>> +}
>> +
>> +NSString* _Nonnull trust_c_function() {
>> + NSString* out = getString();
>> + if (out) {};
>> + return out; // no-warning
>> +}
>> +
>> +NSString* _Nonnull distrust_unannoted_function() {
>> + NSString* out = getPossiblyNullString();
>> + if (out) {};
>> + return out; // expected-warning{{}}
>> +}
>> +
>> +NSString * _Nonnull distrustProtocol(id<MyProtocol> o) {
>> + NSString* out = [o getString];
>> + if (out) {};
>> + return out; // expected-warning{{}}
>> +}
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180329/152af018/attachment-0001.html>
More information about the cfe-commits
mailing list