Charusso updated this revision to Diff 207458.
Charusso added a comment.
- I do not like `Optional<const T *>` anymore.
- More simple notes.
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/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,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/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<check::PostCall> {
+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<bool> 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<const bool *> RawValue = CDM.lookup(Call);
+ if (!RawValue)
+ return;
+
+ bool Value = **RawValue;
+
+ // Get the symbolic return value.
+ ProgramStateRef State = C.getState();
+ SVal RetV = Call.getReturnValue();
+ Optional<DefinedOrUnknownSVal> RetDV = RetV.getAs<DefinedOrUnknownSVal>();
+ 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<CXXMethodDecl>(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<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
@@ -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<std::string(BugReport &)> &&Cb) {
+ 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,9 @@
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.
+// 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<APIModeling>, Hidden;
@@ -274,6 +274,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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
