r328282 - [analyzer] Trust _Nonnull annotations for system framework
George Karpenkov via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 26 11:05:57 PDT 2018
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 <mailto: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 <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 <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 <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 <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 <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 <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 <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 <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 <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 <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 <mailto:cfe-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits <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/20180326/a68d3e83/attachment-0001.html>
More information about the cfe-commits
mailing list