[PATCH] D125400: [clang][Analyzer] Add errno state to standard functions modeling.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:412
+  class ZeroRelatedErrnoConstraint : public ErrnoConstraintKind {
+    clang::BinaryOperatorKind Relation;
+
----------------
The explicit qualification is not necessary. There are other cases like this.

In the constraint manager, we tend to use `Op` for //operator kinds//, it would be nice to use the same taxonomy here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:417
+                               errno_modeling::ErrnoCheckState CS)
+        : ErrnoConstraintKind(CS), Relation(Rel) {}
+
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:406
+    /// It may return empty string if no note tag is to be added.
+    virtual std::string describe(StringRef FunctionName) const = 0;
+
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:443-446
+      Optional<DefinedOrUnknownSVal> Cond =
+          SVB.evalBinOpNN(State, Relation, ErrnoSVal, ZeroVal,
+                          SVB.getConditionType())
+              .getAs<DefinedOrUnknownSVal>();
----------------
Just use `.castAs<NonLoc>()`. You will assert it anyway, and I find this much more readable.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:454-460
+      std::string Result = "Assuming that function '";
+      Result.append(FunctionName.str());
+      Result.append("' fails, in this case the value 'errno' becomes ");
+      Result.append(BinaryOperator::getOpcodeStr(Relation).str());
+      Result.append(" 0 and ");
+      Result.append(describeErrnoCheckState(CheckState));
+      return Result;
----------------
I believe you should use `Twine` for this.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1028
+        std::string Note = Case.getErrnoConstraint().describe(
+            D ? D->getNameAsString() : "<unknown>");
+        if (Note.empty())
----------------
What is `<unknown>` in this context? Please add a test for it or remove this branch.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1037-1042
+            [Node, Note]() -> std::string {
+              // Don't emit "Assuming..." note when we ended up
+              // knowing in advance which branch is taken.
+              if (Node->succ_size() == 1)
+                return "";
+              return Note.str();
----------------
Why did you change these?


================
Comment at: clang/test/Analysis/errno-stdlibraryfunctions.c:20-24
+void test2() {
+  if (access("path", 0) == -1) {
+    if (errno != 0) { }
+  }
+}
----------------



================
Comment at: clang/test/Analysis/errno-stdlibraryfunctions.c:28
+  if (access("path", 0) != -1) {
+    // expected-note at -1{{Assuming that function 'access' is successful, in this case the value 'errno' may be undefined after the call and should not be used}}
+    if (errno != 0) { }
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125400/new/

https://reviews.llvm.org/D125400