Charusso updated this revision to Diff 207851.
Charusso marked 8 inline comments as done.
Charusso added a comment.
- Fix.
- Document `NoteTag`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63915/new/
https://reviews.llvm.org/D63915
Files:
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
clang/test/Analysis/return-value-guaranteed.cpp
clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
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<const bool *> 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.
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/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<FunctionExitPoint> FE = P.getAs<FunctionExitPoint>()) {
+ return PathDiagnosticLocation(FE->getStmt(), SMng,
+ FE->getLocationContext());
} else {
llvm_unreachable("Unexpected ProgramPoint");
}
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<check::PostCall, check::EndFunction> {
+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<bool> 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<CXXMethodDecl>(Call.getDecl()))
+ if (const auto *RD = dyn_cast<CXXRecordDecl>(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<bool> isInvariantBreak(bool ExpectedValue, SVal ReturnV,
+ CheckerContext &C) {
+ auto ReturnDV = ReturnV.getAs<DefinedOrUnknownSVal>();
+ 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<const bool *> RawExpectedValue = CDM.lookup(Call);
+ if (!RawExpectedValue)
+ return;
+
+ SVal ReturnV = Call.getReturnValue();
+ bool ExpectedValue = **RawExpectedValue;
+ Optional<bool> 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<DefinedOrUnknownSVal>(), 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<const bool *> RawExpectedValue = CDM.lookup(*Call);
+ if (!RawExpectedValue)
+ return;
+
+ SVal ReturnV = State->getSVal(RS->getRetValue(), C.getLocationContext());
+ bool ExpectedValue = **RawExpectedValue;
+ Optional<bool> 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<ReturnValueChecker>();
+}
+
+bool ento::shouldRegisterReturnValueChecker(const LangOptions &LO) {
+ return true;
+}
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<const FnCheck *> Callback = Callbacks.lookup(Call))
+ return **Callback;
return nullptr;
}
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/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<std::string(BugReport &)> &&Cb) {
+ ///
+ /// @param Cb Callback only with 'BugReport &' parameter.
+ /// @param IsPrunable Whether the note is prunable.
+ const NoteTag *getNoteTag(std::function<std::string(BugReport &)> &&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/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<const T *> lookup(const CallEvent &Call) const {
// Slow path: linear lookup.
// TODO: Implement some sort of fast path.
for (const std::pair<CallDescription, T> &I : LinearMap)
if (Call.isCalled(I.first))
return &I.second;
- return nullptr;
+ return None;
}
};
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<Alpha>;
-// 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<APIModeling>, Hidden;
@@ -274,6 +275,10 @@
let ParentPackage = APIModeling in {
+def ReturnValueChecker : Checker<"ReturnValue">,
+ HelpText<"Model the guaranteed boolean return value of function calls">,
+ Documentation<NotDocumented>;
+
def StdCLibraryFunctionsChecker : Checker<"StdCLibraryFunctions">,
HelpText<"Improve modeling of the C standard library functions">,
Documentation<NotDocumented>;
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
