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,9 @@ 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. +// The APIModeling package is for checkers that model APIs. These checkers are +// always turned on; this package is intended for API modeling that is not +// controlled by the target triple. def APIModeling : Package<"apiModeling">, Hidden; def GoogleAPIModeling : Package<"google">, ParentPackage, Hidden; @@ -274,6 +274,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 @@ -225,18 +225,19 @@ /// 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)); + 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) { + 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,188 @@ +//===- 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: + void checkPostCall(const CallEvent &Call, CheckerContext &C) const; + + // It reports whether one of the predefined invariants ('CDM') is broken. + void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const; + +private: + // These are known in the LLVM project. + // The pairs are in the following form: {{{class, call}}, return value} + CallDescriptionMap CDM = { + // '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 + +// The predefinitions could break due to the ever growing code base. +// Check for our invariants and see whether they still apply. +static bool isInvariantBreak(ProgramStateRef State, bool ConcreteValue, + DefinedOrUnknownSVal ReturnDV) { + if (ConcreteValue) + return State->isNull(ReturnDV).isConstrainedTrue(); + + return State->isNull(ReturnDV).isConstrainedFalse(); +} + +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; +} + +void ReturnValueChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + // Get the concrete return value. + Optional RawConcreteValue = CDM.lookup(Call); + if (!RawConcreteValue) + return; + + // Get the symbolic return value. + ProgramStateRef State = C.getState(); + SVal ReturnV = Call.getReturnValue(); + auto ReturnDV = ReturnV.getAs(); + if (!ReturnDV) + return; + + bool ConcreteValue = **RawConcreteValue; + bool IsInvariantBreak = isInvariantBreak(State, ConcreteValue, *ReturnDV); + std::string Name = getName(Call); + + const NoteTag *CallTag = C.getNoteTag( + [Name, ConcreteValue, IsInvariantBreak](BugReport &BR) -> std::string { + SmallString<128> Msg; + llvm::raw_svector_ostream Out(Msg); + + Out << '\'' << Name << '\''; + + // If the invariant is broken we do not return the concrete value. + if (IsInvariantBreak) + Out << " should return "; + else + Out << " returns "; + + Out << (ConcreteValue ? "true" : "false"); + + if (IsInvariantBreak) + Out << " but it returns " << (ConcreteValue ? "false" : "true"); + + return Out.str(); + }, + /*IsPrunable=*/true); + + // Set the concrete return value with the note. + State = State->assume(*ReturnDV, ConcreteValue); + C.addTransition(State, CallTag); + return; +} + +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; + + // Get the caller. + ProgramStateRef State = C.getState(); + CallEventRef<> Call = + C.getStateManager().getCallEventManager().getCaller(SFC, State); + if (!Call) + return; + + // Get the concrete return value. + Optional RawConcreteValue = CDM.lookup(*Call); + if (!RawConcreteValue) + return; + + // Get the symbolic return value. + SVal ReturnV = State->getSVal(RS->getRetValue(), C.getLocationContext()); + auto ReturnDV = ReturnV.getAs(); + if (!ReturnDV) + return; + + // See whether the invariant is broken. + bool ConcreteValue = **RawConcreteValue; + if (!isInvariantBreak(State, ConcreteValue, *ReturnDV)) + return; + + std::string Name = getName(*Call); + + const NoteTag *CallTag = C.getNoteTag( + [Name, ConcreteValue, 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 " + << (ConcreteValue ? "false" : "true"); + + return Out.str(); + }, + /*IsPrunable=*/false); + + // Set the note. + 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,100 @@ +// 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 an unspoken LLVM coding standard. 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'}} + // class-note@-3 {{'MCAsmParser::Error' should return true but it returns false}} +} + +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 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.