Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -95,9 +95,10 @@ def LLVM : Package<"llvm">; def LLVMAlpha : Package<"llvm">, ParentPackage; -// The APIModeling package is for checkers that model APIs and don't perform -// any diagnostics. These checkers are always turned on; this package is -// intended for API modeling that is not controlled by the target triple. +// Checkers within APIModeling package are model APIs and enabled by the core +// or through checker dependencies, so one should not enable/disable them by +// hand (unless they cause a crash, but that will cause dependent checkers to +// be implicitly disabled). def APIModeling : Package<"apiModeling">, Hidden; def GoogleAPIModeling : Package<"google">, ParentPackage, Hidden; @@ -274,6 +275,10 @@ let ParentPackage = APIModeling in { +def ReturnValueChecker : Checker<"ReturnValue">, + HelpText<"Model the guaranteed boolean return value of function calls">, + Documentation; + def StdCLibraryFunctionsChecker : Checker<"StdCLibraryFunctions">, HelpText<"Improve modeling of the C standard library functions">, Documentation; Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h +++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h @@ -1112,14 +1112,14 @@ CallDescriptionMap(const CallDescriptionMap &) = delete; CallDescriptionMap &operator=(const CallDescription &) = delete; - const T *lookup(const CallEvent &Call) const { + Optional lookup(const CallEvent &Call) const { // Slow path: linear lookup. // TODO: Implement some sort of fast path. for (const std::pair &I : LinearMap) if (Call.isCalled(I.first)) return &I.second; - return nullptr; + return None; } }; Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h +++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h @@ -219,24 +219,30 @@ Eng.getBugReporter().emitReport(std::move(R)); } - /// Produce a program point tag that displays an additional path note /// to the user. This is a lightweight alternative to the /// BugReporterVisitor mechanism: instead of visiting the bug report /// node-by-node to restore the sequence of events that led to discovering /// a bug, you can add notes as you add your transitions. - const NoteTag *getNoteTag(NoteTag::Callback &&Cb) { - return Eng.getNoteTags().makeNoteTag(std::move(Cb)); + /// + /// @param Cb Callback with 'BugReporterContext &, BugReport &' parameters. + /// @param IsPrunable Whether the note is prunable. + const NoteTag *getNoteTag(NoteTag::Callback &&Cb, bool IsPrunable = false) { + return Eng.getNoteTags().makeNoteTag(std::move(Cb), IsPrunable); } /// A shorthand version of getNoteTag that doesn't require you to accept /// the BugReporterContext arguments when you don't need it. - const NoteTag *getNoteTag(std::function &&Cb) { + /// + /// @param Cb Callback only with 'BugReport &' parameter. + /// @param IsPrunable Whether the note is prunable. + const NoteTag *getNoteTag(std::function &&Cb, + bool IsPrunable = false) { return getNoteTag( - [Cb](BugReporterContext &, BugReport &BR) { return Cb(BR); }); + [Cb](BugReporterContext &, BugReport &BR) { return Cb(BR); }, + IsPrunable); } - /// Returns the word that should be used to refer to the declaration /// in the report. StringRef getDeclDescription(const Decl *D); Index: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -83,6 +83,7 @@ RetainCountChecker/RetainCountDiagnostics.cpp ReturnPointerRangeChecker.cpp ReturnUndefChecker.cpp + ReturnValueChecker.cpp RunLoopAutoreleaseLeakChecker.cpp SimpleStreamChecker.cpp SmartPtrModeling.cpp Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -2239,9 +2239,8 @@ return nullptr; } - const FnCheck *Callback = Callbacks.lookup(Call); - if (Callback) - return *Callback; + if (Optional Callback = Callbacks.lookup(Call)) + return **Callback; return nullptr; } Index: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp =================================================================== --- /dev/null +++ clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp @@ -0,0 +1,174 @@ +//===- ReturnValueChecker - Applies guaranteed return values ----*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// This defines ReturnValueChecker, which checks for calls with guaranteed +// boolean return value. It ensures the return value of each function call. +// +//===----------------------------------------------------------------------===// + +#include "clang/Driver/DriverDiagnostic.h" +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "llvm/ADT/Optional.h" +#include "llvm/ADT/SmallVector.h" + +using namespace clang; +using namespace ento; + +namespace { +class ReturnValueChecker : public Checker { +public: + // It sets the predefined invariant ('CDM') if the current call not break it. + void checkPostCall(const CallEvent &Call, CheckerContext &C) const; + + // It reports whether a predefined invariant ('CDM') is broken. + void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const; + +private: + // The pairs are in the following form: {{{class, call}}, return value} + CallDescriptionMap CDM = { + // These are known in the LLVM project: 'Error()' + {{{"ARMAsmParser", "Error"}}, true}, + {{{"HexagonAsmParser", "Error"}}, true}, + {{{"LLLexer", "Error"}}, true}, + {{{"LLParser", "Error"}}, true}, + {{{"MCAsmParser", "Error"}}, true}, + {{{"MCAsmParserExtension", "Error"}}, true}, + {{{"TGParser", "Error"}}, true}, + {{{"X86AsmParser", "Error"}}, true}, + // 'TokError()' + {{{"LLParser", "TokError"}}, true}, + {{{"MCAsmParser", "TokError"}}, true}, + {{{"MCAsmParserExtension", "TokError"}}, true}, + {{{"TGParser", "TokError"}}, true}, + // 'error()' + {{{"MIParser", "error"}}, true}, + {{{"WasmAsmParser", "error"}}, true}, + {{{"WebAssemblyAsmParser", "error"}}, true}, + // Other + {{{"AsmParser", "printError"}}, true}}; +}; +} // namespace + +static std::string getName(const CallEvent &Call) { + std::string Name = ""; + if (const auto *MD = dyn_cast(Call.getDecl())) + if (const auto *RD = dyn_cast(MD->getParent())) + Name += RD->getNameAsString() + "::"; + + Name += Call.getCalleeIdentifier()->getName(); + return Name; +} + +// The predefinitions ('CDM') could break due to the ever growing code base. +// Check for the expected invariants and see whether they apply. +static Optional isInvariantBreak(bool ExpectedValue, SVal ReturnV, + CheckerContext &C) { + auto ReturnDV = ReturnV.getAs(); + if (!ReturnDV) + return None; + + if (ExpectedValue) + return C.getState()->isNull(*ReturnDV).isConstrainedTrue(); + + return C.getState()->isNull(*ReturnDV).isConstrainedFalse(); +} + +void ReturnValueChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + Optional RawExpectedValue = CDM.lookup(Call); + if (!RawExpectedValue) + return; + + SVal ReturnV = Call.getReturnValue(); + bool ExpectedValue = **RawExpectedValue; + Optional IsInvariantBreak = isInvariantBreak(ExpectedValue, ReturnV, C); + if (!IsInvariantBreak) + return; + + // If the invariant is broken it is reported by 'checkEndFunction()'. + if (*IsInvariantBreak) + return; + + std::string Name = getName(Call); + const NoteTag *CallTag = C.getNoteTag( + [Name, ExpectedValue](BugReport &) -> std::string { + SmallString<128> Msg; + llvm::raw_svector_ostream Out(Msg); + + Out << '\'' << Name << "' returns " + << (ExpectedValue ? "true" : "false"); + return Out.str(); + }, + /*IsPrunable=*/true); + + ProgramStateRef State = C.getState(); + State = State->assume(ReturnV.castAs(), ExpectedValue); + C.addTransition(State, CallTag); +} + +void ReturnValueChecker::checkEndFunction(const ReturnStmt *RS, + CheckerContext &C) const { + if (!RS || !RS->getRetValue()) + return; + + // We cannot get the caller in the top-frame. + const StackFrameContext *SFC = C.getStackFrame(); + if (SFC->inTopFrame()) + return; + + ProgramStateRef State = C.getState(); + CallEventManager &CMgr = C.getStateManager().getCallEventManager(); + CallEventRef<> Call = CMgr.getCaller(SFC, State); + if (!Call) + return; + + Optional RawExpectedValue = CDM.lookup(*Call); + if (!RawExpectedValue) + return; + + SVal ReturnV = State->getSVal(RS->getRetValue(), C.getLocationContext()); + bool ExpectedValue = **RawExpectedValue; + Optional IsInvariantBreak = isInvariantBreak(ExpectedValue, ReturnV, C); + if (!IsInvariantBreak) + return; + + // If the invariant is appropriate it is reported by 'checkPostCall()'. + if (!*IsInvariantBreak) + return; + + std::string Name = getName(*Call); + const NoteTag *CallTag = C.getNoteTag( + [Name, ExpectedValue, SFC](BugReport &BR) -> std::string { + SmallString<128> Msg; + llvm::raw_svector_ostream Out(Msg); + + BR.markInteresting(SFC); + + // The following is swapped because the invariant is broken. + Out << '\'' << Name << "' returns " + << (ExpectedValue ? "false" : "true"); + + return Out.str(); + }, + /*IsPrunable=*/false); + + C.addTransition(State, CallTag); +} + +void ento::registerReturnValueChecker(CheckerManager &Mgr) { + Mgr.registerChecker(); +} + +bool ento::shouldRegisterReturnValueChecker(const LangOptions &LO) { + return true; +} Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp +++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp @@ -780,6 +780,9 @@ NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng); } llvm_unreachable("Unexpected CFG element at front of block"); + } else if (Optional FE = P.getAs()) { + return PathDiagnosticLocation(FE->getStmt(), SMng, + FE->getLocationContext()); } else { llvm_unreachable("Unexpected ProgramPoint"); } Index: clang/test/Analysis/return-value-guaranteed.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/return-value-guaranteed.cpp @@ -0,0 +1,91 @@ +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=core,apiModeling.ReturnValue \ +// RUN: -analyzer-output=text -verify=class %s + +struct Foo { int Field; }; +bool problem(); +void doSomething(); + +// We predefined the return value of 'MCAsmParser::Error' as true and we cannot +// take the false-branches which leads to a "garbage value" false positive. +namespace test_classes { +struct MCAsmParser { + static bool Error(); +}; + +bool parseFoo(Foo &F) { + if (problem()) { + // class-note@-1 {{Assuming the condition is false}} + // class-note@-2 {{Taking false branch}} + return MCAsmParser::Error(); + } + + F.Field = 0; + // class-note@-1 {{The value 0 is assigned to 'F.Field'}} + return !MCAsmParser::Error(); + // class-note@-1 {{'MCAsmParser::Error' returns true}} +} + +bool parseFile() { + Foo F; + if (parseFoo(F)) { + // class-note@-1 {{Calling 'parseFoo'}} + // class-note@-2 {{Returning from 'parseFoo'}} + // class-note@-3 {{Taking false branch}} + return true; + } + + if (F.Field == 0) { + // class-note@-1 {{Field 'Field' is equal to 0}} + // class-note@-2 {{Taking true branch}} + + // no-warning: "The left operand of '==' is a garbage value" was here. + doSomething(); + } + + (void)(1 / F.Field); + // class-warning@-1 {{Division by zero}} + // class-note@-2 {{Division by zero}} + return false; +} +} // namespace test_classes + + +// We predefined 'MCAsmParser::Error' as returning true, but now it returns +// false, which breaks our invariant. Test the notes. +namespace test_break { +struct MCAsmParser { + static bool Error() { + return false; // class-note {{'MCAsmParser::Error' returns false}} + } +}; + +bool parseFoo(Foo &F) { + if (problem()) { + // class-note@-1 {{Assuming the condition is false}} + // class-note@-2 {{Taking false branch}} + return !MCAsmParser::Error(); + } + + F.Field = 0; + // class-note@-1 {{The value 0 is assigned to 'F.Field'}} + return MCAsmParser::Error(); + // class-note@-1 {{Calling 'MCAsmParser::Error'}} + // class-note@-2 {{Returning from 'MCAsmParser::Error'}} +} + +bool parseFile() { + Foo F; + if (parseFoo(F)) { + // class-note@-1 {{Calling 'parseFoo'}} + // class-note@-2 {{Returning from 'parseFoo'}} + // class-note@-3 {{Taking false branch}} + return true; + } + + (void)(1 / F.Field); + // class-warning@-1 {{Division by zero}} + // class-note@-2 {{Division by zero}} + return false; +} +} // namespace test_classes Index: clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp =================================================================== --- clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp +++ clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp @@ -33,11 +33,11 @@ Impl(std::move(Data)) {} const bool *lookup(const CallEvent &Call) { - const bool *Result = Impl.lookup(Call); + Optional Result = Impl.lookup(Call); // If it's a function we expected to find, remember that we've found it. - if (Result && *Result) + if (Result && *Result && **Result) ++Found; - return Result; + return *Result; } // Fail the test if we haven't found all the true-calls we were looking for.