Charusso updated this revision to Diff 207703.
Charusso marked 10 inline comments as done.
Charusso added a comment.

- Fix.
- Refactor.


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,99 @@
+// 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;
+  }
+
+  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/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
@@ -207,6 +207,7 @@
   /// @param State The state of the generated node.
   /// @param Tag The tag to uniquely identify the creation site. If null,
   ///        the default tag for the checker will be used.
+  /// @param IsPrunable Whether the note is prunable.
   ExplodedNode *
   generateNonFatalErrorNode(ProgramStateRef State = nullptr,
                             const ProgramPointTag *Tag = nullptr) {
@@ -225,18 +226,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,6 @@
 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.
 def APIModeling : Package<"apiModeling">, Hidden;
 def GoogleAPIModeling : Package<"google">, ParentPackage<APIModeling>, Hidden;
 
@@ -274,6 +271,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

Reply via email to