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,137 @@ +//===- 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; + +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 + +void ReturnValueChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + // Get the concrete return value. + Optional RawValue = CDM.lookup(Call); + if (!RawValue) + return; + + bool Value = **RawValue; + + // Get the symbolic return value. + ProgramStateRef State = C.getState(); + SVal RetV = Call.getReturnValue(); + Optional RetDV = RetV.getAs(); + if (!RetDV.hasValue()) + return; + + // The predefinitions could break due to the ever growing code base. + // Check for our invariants and see whether they still apply. + bool IsInvariantBreak = false; + if (Value) { + IsInvariantBreak = State->isNull(*RetDV).isConstrainedTrue(); + } else { + IsInvariantBreak = State->isNull(*RetDV).isConstrainedFalse(); + } + + // Create a note based on the invariant. + std::string Name = ""; + Name += dyn_cast(Call.getDecl())->getParent()->getName(); + Name += "::"; + Name += Call.getCalleeIdentifier()->getName(); + + if (!IsInvariantBreak) { + const NoteTag *CallTag = C.getNoteTag( + [Name, Value](BugReport &BR) -> std::string { + SmallString<128> Msg; + llvm::raw_svector_ostream Out(Msg); + + Out << '\'' << Name << "' always returns " + << (Value ? "true" : "false"); + + return Out.str(); + }, + /*IsPrunable=*/true); + + // Set the concrete return value with the note. + State = State->assume(*RetDV, Value); + C.addTransition(State, CallTag); + return; + } + + // We are in the case when the code has been changed. + const StackFrameContext *SFC = C.getStackFrame(); + const NoteTag *CallTag = C.getNoteTag( + [Name, Value, SFC](BugReport &BR) -> std::string { + SmallString<128> Msg; + llvm::raw_svector_ostream Out(Msg); + + BR.markInteresting(SFC); + + Out << '\'' << Name << "' should always return " + << (Value ? "true" : "false") + << " according to the LLVM coding standard, but it returns " + << (Value ? "false" : "true"); + + return Out.str(); + }, + /*IsPrunable=*/false); + + // Set just the note. + C.addTransition(State, CallTag); +} + +void ento::registerReturnValueChecker(CheckerManager &Mgr) { + Mgr.registerChecker(); +} + +bool ento::shouldRegisterReturnValueChecker(const LangOptions &LO) { + return true; +} Index: clang/test/Analysis/return-value-guaranteed.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/return-value-guaranteed.cpp @@ -0,0 +1,96 @@ +// 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' always 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 the LLVM coding standard. Test the note. +namespace test_break { +struct MCAsmParser { + static bool Error() { return 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 {{'MCAsmParser::Error' should always return true according to the LLVM coding standard, 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.