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 <ekarpen...@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 <ale...@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@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(); >> + >> +@protocol MyProtocol >> +- (_Nonnull NSString *) getString; >> +@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@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
