[clang][analyzer] Errno modeling code refactor (NFC).

[balazske]

Summary

Some of the code used in StdLibraryFunctionsChecker is applicable to
other checkers, this is put into common functions. Errno related
parts of the checker are simplified and renamed. Documentations in
errno_modeling functions are updated.

This change makes it available to have more checkers that perform
modeling of some standard functions. These can set the errno state
with common functions and the bug report messages (note tags) can
look similar.

[steakhal]

Approved with nits. Thanks!

clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp

267

The same applies for these.

298

I would prefer returning nullptr explicitly here.

304–307

I believe you should use Twine() for chaining string slices.

clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h

83

It seems like you are already within this namespace. You don't need to explicitly qualify this.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

405

Why don't you = default it if the body is actually empty?

416

I find this name much more appealing than it was before! Thanks.

[balazske]

Small improvements.

[martong]

LGTM!

clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h

 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 
 namespace clang {
 namespace ento {
 namespace errno_modeling {
 
+/// Describe how reads and writes of \c errno are handled by the checker.
 enum ErrnoCheckState : unsigned {
   /// We do not know anything about 'errno'.
+  /// Read and write is always allowed.
   Irrelevant = 0,
 
   /// Value of 'errno' should be checked to find out if a previous function call
   /// has failed.
+  /// When this state is set \c errno must be read by the program before a next
+  /// standard function call or other overwrite of \c errno follows, otherwise
+  /// a bug report is emitted.
   MustBeChecked = 1,
 
   /// Value of 'errno' is not allowed to be read, it can contain an unspecified
   /// value.
+  /// When this state is set \c errno is not allowed to be read by the program
+  /// until it is overwritten or invalidated.
   MustNotBeChecked = 2
 };
 
 /// Returns the value of 'errno', if 'errno' was found in the AST.
 llvm::Optional<SVal> getErrnoValue(ProgramStateRef State);
 
 /// Returns the errno check state, \c Errno_Irrelevant if 'errno' was not found
 /// (this is not the only case for that value).

 
 /// Determine if a `Decl` node related to 'errno'.
 /// This is true if the declaration is the errno variable or a function
 /// that returns a pointer to the 'errno' value (usually the 'errno' macro is
 /// defined with this function). \p D is not required to be a canonical
 /// declaration.
 bool isErrno(const Decl *D);
 
+/// Produce a textual description about how \c errno is allowed to be used
+/// (in a \c ErrnoCheckState).
+/// The returned string is insertable into a longer warning message in the form
+/// "the value 'errno' <...>".
+/// Currently only the \c errno_modeling::MustNotBeChecked state is supported,
+/// others are not used by the clients.
+const char *describeErrnoCheckState(ErrnoCheckState CS);
+
 /// Create a NoteTag that displays the message if the 'errno' memory region is
 /// marked as interesting, and resets the interestingness.
 const NoteTag *getErrnoNoteTag(CheckerContext &C, const std::string &Message);
 
+/// Set errno state for the common case when a standard function is successful.
+/// Set \c ErrnoCheckState to \c MustNotBeChecked (the \c errno value is not
+/// affected). At the state transition a note tag created by
+/// \c getNoteTagForStdSuccess can be used.
+ProgramStateRef setErrnoForStdSuccess(ProgramStateRef State, CheckerContext &C);
+
+/// Set errno state for the common case when a standard function fails.
+/// Set \c errno value to be not equal to zero and \c ErrnoCheckState to
+/// \c Irrelevant . The irrelevant errno state ensures that no related bug
+/// report is emitted later and no note tag is needed.
+/// \arg \c ErrnoSym Value to be used for \c errno and constrained to be
+/// non-zero.
+ProgramStateRef setErrnoForStdFailure(ProgramStateRef State, CheckerContext &C,
+                                      NonLoc ErrnoSym);
+
+/// Generate the note tag that can be applied at the state generated by
+/// \c setErrnoForStdSuccess .
+/// \arg \c Fn Name of the (standard) function that is modeled.
+const NoteTag *getNoteTagForStdSuccess(CheckerContext &C, llvm::StringRef Fn);
+
 } // namespace errno_modeling
 } // namespace ento
 } // namespace clang
 
 #endif // LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_ERRNOMODELING_H

clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp

if (const auto *VD = dyn_cast_or_null<VarDecl>(D))
     if (const IdentifierInfo *II = VD->getIdentifier())
       return II->getName() == ErrnoVarName;
   if (const auto *FD = dyn_cast_or_null<FunctionDecl>(D))
     if (const IdentifierInfo *II = FD->getIdentifier())
       return llvm::is_contained(ErrnoLocationFuncNames, II->getName());
   return false;
 }
 
+const char *describeErrnoCheckState(ErrnoCheckState CS) {
+  assert(CS == errno_modeling::MustNotBeChecked &&
+         "Errno description not applicable.");
+  return "may be undefined after the call and should not be used";
+}
+
 const NoteTag *getErrnoNoteTag(CheckerContext &C, const std::string &Message) {
   return C.getNoteTag([Message](PathSensitiveBugReport &BR) -> std::string {
     const MemRegion *ErrnoR = BR.getErrorNode()->getState()->get<ErrnoRegion>();
     if (ErrnoR && BR.isInteresting(ErrnoR)) {
       BR.markNotInteresting(ErrnoR);
       return Message;
     }
     return "";
   });
 }
 
+ProgramStateRef setErrnoForStdSuccess(ProgramStateRef State,
+                                      CheckerContext &C) {
+  return setErrnoState(State, MustNotBeChecked);
+}
+
+ProgramStateRef setErrnoForStdFailure(ProgramStateRef State, CheckerContext &C,
+                                      NonLoc ErrnoSym) {
+  SValBuilder &SVB = C.getSValBuilder();
+  NonLoc ZeroVal = SVB.makeZeroVal(C.getASTContext().IntTy).castAs<NonLoc>();
+  DefinedOrUnknownSVal Cond =
+      SVB.evalBinOp(State, BO_NE, ErrnoSym, ZeroVal, SVB.getConditionType())
+          .castAs<DefinedOrUnknownSVal>();
+  State = State->assume(Cond, true);
+  if (!State)
+    return nullptr;
+  return setErrnoValue(State, C.getLocationContext(), ErrnoSym, Irrelevant);
+}
+
+const NoteTag *getNoteTagForStdSuccess(CheckerContext &C, llvm::StringRef Fn) {
+  return getErrnoNoteTag(
+      C, (Twine("Assuming that function '") + Twine(Fn) +
+          Twine("' is successful, in this case the value 'errno' ") +
+          Twine(describeErrnoCheckState(MustNotBeChecked)))
+             .str());
+}
+
 } // namespace errno_modeling
 } // namespace ento
 } // namespace clang
 
 void ento::registerErrnoModeling(CheckerManager &mgr) {
   mgr.registerChecker<ErrnoModeling>();
 }
 
 bool ento::shouldRegisterErrnoModeling(const CheckerManager &mgr) {
   return true;
 }

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 
 #include <string>
 
 using namespace clang;
 using namespace clang::ento;
 
-/// Produce a textual description of the state of \c errno (this describes the
-/// way how it is allowed to be used).
-/// The returned string is insertable into a longer warning message (in the form
-/// "the value 'errno' <...>").
-/// Currently only the \c errno_modeling::MustNotBeChecked state is supported.
-/// But later other kind of errno state may be needed if functions with special
-/// handling of \c errno are added.
-static const char *describeErrnoCheckState(errno_modeling::ErrnoCheckState CS) {
-  assert(CS == errno_modeling::MustNotBeChecked &&
-         "Errno description not applicable.");
-  return "may be undefined after the call and should not be used";
-}
-
 namespace {
 class StdLibraryFunctionsChecker
     : public Checker<check::PreCall, check::PostCall, eval::Call> {
 
   class Summary;
 
   /// Specify how much the analyzer engine should entrust modeling this function
   /// to us. If he doesn't, he performs additional invalidations.
bool checkSpecificValidity(const FunctionDecl *FD) const override {
       return ValidArg;
     }
   };
 
   /// The complete list of constraints that defines a single branch.
   using ConstraintSet = std::vector<ValueConstraintPtr>;
 
   /// Define how a function affects the system variable 'errno'.
-  /// This works together with the ErrnoModeling and ErrnoChecker classes.
+  /// This works together with the \c ErrnoModeling and \c ErrnoChecker classes.
+  /// Currently 3 use cases exist: success, failure, irrelevant.
+  /// In the future the failure case can be customized to set \c errno to a
+  /// more specific constraint (for example > 0), or new case can be added
+  /// for functions which require check of \c errno in both success and failure
+  /// case.
   class ErrnoConstraintBase {
   public:
     /// Apply specific state changes related to the errno variable.
     virtual ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
                                   const Summary &Summary,
                                   CheckerContext &C) const = 0;
-    /// Get a description about what is applied to 'errno' and how is it allowed
-    /// to be used. If ErrnoChecker generates a bug then this message is
-    /// displayed as a note at the function call.
-    /// It may return empty string if no note tag is to be added.
-    virtual std::string describe(StringRef FunctionName) const { return ""; }
+    /// Get a NoteTag about the changes made to 'errno' and the possible bug.
+    /// It may return \c nullptr (if no bug report from \c ErrnoChecker is
+    /// expected).
+    virtual const NoteTag *describe(CheckerContext &C,
+                                    StringRef FunctionName) const {
+      return nullptr;
+    }
 
     virtual ~ErrnoConstraintBase() {}
 
   protected:
-    /// Many of the descendant classes use this value.
-    const errno_modeling::ErrnoCheckState CheckState;
-
-    ErrnoConstraintBase(errno_modeling::ErrnoCheckState CS) : CheckState(CS) {}
+    ErrnoConstraintBase() = default;
 
     /// This is used for conjure symbol for errno to differentiate from the
     /// original call expression (same expression is used for the errno symbol).
     static int Tag;
   };
 
-  /// Set value of 'errno' to be related to 0 in a specified way, with a
-  /// specified "errno check state". For example with \c BO_GT 'errno' is
-  /// constrained to be greater than 0. Use this for failure cases of functions.
-  class ZeroRelatedErrnoConstraint : public ErrnoConstraintBase {
-    BinaryOperatorKind Op;
-
+  /// Set errno constraint at failure cases of standard functions.
+  /// Failure case: 'errno' becomes not equal to 0 and may or may not be checked
+  /// by the program. \c ErrnoChecker does not emit a bug report after such a
+  /// function call.
+  class FailureErrnoConstraint : public ErrnoConstraintBase {
   public:
-    ZeroRelatedErrnoConstraint(clang::BinaryOperatorKind OpK,
-                               errno_modeling::ErrnoCheckState CS)
-        : ErrnoConstraintBase(CS), Op(OpK) {
-      assert(BinaryOperator::isComparisonOp(OpK));
-    }
-
     ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
                           const Summary &Summary,
                           CheckerContext &C) const override {
       SValBuilder &SVB = C.getSValBuilder();
       NonLoc ErrnoSVal =
           SVB.conjureSymbolVal(&Tag, Call.getOriginExpr(),
                                C.getLocationContext(), C.getASTContext().IntTy,
                                C.blockCount())
               .castAs<NonLoc>();
-      NonLoc ZeroVal =
-          SVB.makeZeroVal(C.getASTContext().IntTy).castAs<NonLoc>();
-      DefinedOrUnknownSVal Cond =
-          SVB.evalBinOp(State, Op, ErrnoSVal, ZeroVal, SVB.getConditionType())
-              .castAs<DefinedOrUnknownSVal>();
-      State = State->assume(Cond, true);
-      if (!State)
-        return State;
-      return errno_modeling::setErrnoValue(State, C.getLocationContext(),
-                                           ErrnoSVal, CheckState);
-    }
-
-    std::string describe(StringRef FunctionName) const override {
-      if (CheckState == errno_modeling::Irrelevant)
-        return "";
-      return (Twine("Assuming that function '") + FunctionName.str() +
-              "' fails, in this case the value 'errno' becomes " +
-              BinaryOperator::getOpcodeStr(Op).str() + " 0 and " +
-              describeErrnoCheckState(CheckState))
-          .str();
+      return errno_modeling::setErrnoForStdFailure(State, C, ErrnoSVal);
     }
   };
 
-  /// Applies the constraints to 'errno' for a common case when a standard
-  /// function is successful. The value of 'errno' after the call is not
-  /// specified by the standard (it may change or not). The \c ErrnoChecker can
-  /// generate a bug if 'errno' is read afterwards.
+  /// Set errno constraint at success cases of standard functions.
+  /// Success case: 'errno' is not allowed to be used.
+  /// \c ErrnoChecker can emit bug report after such a function call if errno
+  /// is used.
   class SuccessErrnoConstraint : public ErrnoConstraintBase {
   public:
-    SuccessErrnoConstraint()
-        : ErrnoConstraintBase(errno_modeling::MustNotBeChecked) {}
-
     ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
                           const Summary &Summary,
                           CheckerContext &C) const override {
-      return errno_modeling::setErrnoState(State, CheckState);
+      return errno_modeling::setErrnoForStdSuccess(State, C);
     }
 
-    std::string describe(StringRef FunctionName) const override {
-      return (Twine("Assuming that function '") + FunctionName.str() +
-              "' is successful, in this case the value 'errno' " +
-              describeErrnoCheckState(CheckState))
-          .str();
+    const NoteTag *describe(CheckerContext &C,
+                            StringRef FunctionName) const override {
+      return errno_modeling::getNoteTagForStdSuccess(C, FunctionName);
     }
   };
 
-  /// Set errno constraints if use of 'errno' is completely irrelevant to the
+  /// Set errno constraints if use of 'errno' is irrelevant to the
   /// modeled function or modeling is not possible.
   class NoErrnoConstraint : public ErrnoConstraintBase {
   public:
-    NoErrnoConstraint() : ErrnoConstraintBase(errno_modeling::Irrelevant) {}
-
     ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
                           const Summary &Summary,
                           CheckerContext &C) const override {
-      return errno_modeling::setErrnoState(State, CheckState);
+      return errno_modeling::setErrnoState(State, errno_modeling::Irrelevant);
     }
   };
 
   /// A single branch of a function summary.
   ///
   /// A branch is defined by a series of constraints - "assumptions" -
   /// that together form a single possible outcome of invoking the function.
   /// When static analyzer considers a branch, it tries to introduce
void reportBug(const CallEvent &Call, ExplodedNode *N,
     C.emitReport(std::move(R));
   }
 
   /// These are the errno constraints that can be passed to summary cases.
   /// One of these should fit for a single summary case.
   /// Usually if a failure return value exists for function, that function
   /// needs different cases for success and failure with different errno
   /// constraints (and different return value constraints).
-  const NoErrnoConstraint ErrnoIrrelevant;
-  const SuccessErrnoConstraint ErrnoMustNotBeChecked;
-  const ZeroRelatedErrnoConstraint ErrnoNEZeroIrrelevant{
-      clang::BinaryOperatorKind::BO_NE, errno_modeling::Irrelevant};
+  const NoErrnoConstraint ErrnoIrrelevant{};
+  const SuccessErrnoConstraint ErrnoMustNotBeChecked{};
+  const FailureErrnoConstraint ErrnoNEZeroIrrelevant{};
 };
 
 int StdLibraryFunctionsChecker::ErrnoConstraintBase::Tag = 0;
 
 const StdLibraryFunctionsChecker::ArgNo StdLibraryFunctionsChecker::Ret =
     std::numeric_limits<ArgNo>::max();
 
 } // end of anonymous namespace
for (const ValueConstraintPtr &Constraint : Case.getConstraints()) {
         break;
     }
 
     if (NewState)
       NewState = Case.getErrnoConstraint().apply(NewState, Call, Summary, C);
 
     if (NewState && NewState != State) {
       if (Case.getNote().empty()) {
-        std::string Note;
+        const NoteTag *NT = nullptr;
         if (const auto *D = dyn_cast_or_null<FunctionDecl>(Call.getDecl()))
-          Note = Case.getErrnoConstraint().describe(D->getNameAsString());
-        if (Note.empty())
-          C.addTransition(NewState);
-        else
-          C.addTransition(NewState, errno_modeling::getErrnoNoteTag(C, Note));
+          NT = Case.getErrnoConstraint().describe(C, D->getNameAsString());
+        C.addTransition(NewState, NT);
       } else {
         StringRef Note = Case.getNote();
         const NoteTag *Tag = C.getNoteTag(
             // Sorry couldn't help myself.
             [Node, Note]() -> std::string {
               // Don't emit "Assuming..." note when we ended up
               // knowing in advance which branch is taken.
               return (Node->succ_size() > 1) ? Note.str() : "";