[analyzer] Utility to match function calls.

[xazax.hun]

Summary

This patch adds a small utility to match function calls and Obj-C messages. This utility abstracts away the mutable keywords and the lazy initialization and caching logic of identifiers from the checkers. The SimpleStreamChecker is ported over this utility within this patch to show the reduction of code and to test this change.

[zaks.anna]

include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h

307

"is call" -> "is a call"

lib/StaticAnalyzer/Core/CheckerContext.cpp

41

Please, add && "ObjC Methods are not supported"

47

You can micro optimize by checking if the identifier matches first (and only checking arguments later); in most cases it won't.

57

This will only match against the first part of the ObjC method name. Also, this is not tested. I am Ok with adding the ObjC support later and adding an assert above to error out if this is used for ObjC.

[xazax.hun]

• Removed support for matching ObjC messages. It might be added in a later.
• Addressed review comments.

[xazax.hun]

Updated the comments.

[zaks.anna]

include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h

312

The API is a bit awkward. Maybe it would be better if we make this a member of CallEvent as you've suggested initially?

[xazax.hun]

• Moved the API to CallEvent.

[xazax.hun]

include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h

312

I agree. Moved it to CallEvent.

[zaks.anna]

LGTM

[krasin]

FYI: this revision has likely broken the build: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/9561

FAIL: Clang :: Analysis/simple-stream-checks.c (367 of 8927)
******************** TEST 'Clang :: Analysis/simple-stream-checks.c' FAILED ********************
Script:
--
/mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm_build_asan/./bin/clang -cc1
-internal-isystem /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm_build_asan/bin/../lib/clang/3.9.0/include -nostdsysteminc -analyze -analyzer-checker=core,alpha.unix.SimpleStream
-verify /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/simple-stream-checks.c
--

[zaks.anna]

Looks like the 'II' pointer wasn't initialized. Should be fixed by r258591.

[xazax.hun]

Thank you for the fix and sorry for not noticing this. I was unlucky and the tests did pass on my machine.

[ariccio]

Side note: Has anybody ever considered just treating fopen and fclose like malloc and free?

On Windows I use a trick that I discovered what I call "the _Post_ptr_invalid_ hack" because* the _Post_ptr_invalid_ SAL annotation lets me treat the argument to CloseHandle like the argument to free, and reports double closes as double frees.

Here we'd just treat the FILE* as a different region of memory, to check that they're not mixed.

• this also works in part because a Windows HANDLE is just a typedefd void*

cfe/trunk/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp

106 ↗

(On Diff #45748)

Why specify the RequiredArgs for fclose, and not fopen?

[zaks.anna]

Side note: Has anybody ever considered just treating fopen and fclose like malloc and free?

Yes, there are a few checkers that follow the same open/close rule and could in theory be merged together.

cfe/trunk/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp

106 ↗

(On Diff #45748)

Not sure. Maybe it's an omission.

include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h

namespace ento {
   /// NameTy List = State->get<Name>();
   /// \endcode
   ///
   /// The macro should not be used inside namespaces, or for traits that must
   /// be accessible from more than one translation unit.
   #define REGISTER_LIST_WITH_PROGRAMSTATE(Name, Elem) \
     REGISTER_TRAIT_WITH_PROGRAMSTATE(Name, llvm::ImmutableList<Elem>)
 
+/// This class represents a description of a function call using the number of
+/// arguments and the name of the function.
+class CallDescription {
+  friend class CheckerContext;
+  mutable IdentifierInfo *II;
+  StringRef FuncName;
+  unsigned RequiredArgs;
+
+public:
+  const static unsigned NoArgRequirement = ~0;
+  /// \brief Constructs a CallDescription object.
+  ///
+  /// @param FuncName The name of the function that will be matched.
+  ///
+  /// @param RequiredArgs The number of arguments that is expected to match a
+  /// call. Omit this parameter to match every occurance of call with a given
+  /// name regardless the number of arguments.
+  CallDescription(StringRef FuncName, unsigned RequiredArgs = NoArgRequirement)
+      : FuncName(FuncName), RequiredArgs(RequiredArgs) {}
+};
 
 class CheckerContext {
   ExprEngine &Eng;
   /// The current exploded(symbolic execution) graph node.
   ExplodedNode *Pred;
   /// The flag is true if the (state of the execution) has been modified
   /// by the checker using this context. For example, a new transition has been
   /// added or a bug report issued.
public:
   }
 
   /// \brief Get the name of the called function (path-sensitive).
   StringRef getCalleeName(const CallExpr *CE) const {
     const FunctionDecl *FunDecl = getCalleeDecl(CE);
     return getCalleeName(FunDecl);
   }
 
+  /// \brief Returns true if the CallEvent is a call to a function that matches
+  /// the CallDescription.
+  ///
+  /// Note that this function is not intended to be used to match Obj-C method
+  /// calls.
+  bool isCalled(const CallEvent &Call, const CallDescription &CD);
+
   /// \brief Returns true if the callee is an externally-visible function in the
   /// top-level namespace, such as \c malloc.
   ///
   /// If a name is provided, the function must additionally match the given
   /// name.
   ///
   /// Note that this deliberately excludes C++ library functions in the \c std
   /// namespace, but will include C library functions accessed through the

lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp

void Profile(llvm::FoldingSetNodeID &ID) const {
     ID.AddInteger(K);
   }
 };
 
 class SimpleStreamChecker : public Checker<check::PostCall,
                                            check::PreCall,
                                            check::DeadSymbols,
                                            check::PointerEscape> {
-
-  mutable IdentifierInfo *IIfopen, *IIfclose;
+  CallDescription OpenFn, CloseFn;
 
   std::unique_ptr<BugType> DoubleCloseBugType;
   std::unique_ptr<BugType> LeakBugType;
 
-  void initIdentifierInfo(ASTContext &Ctx) const;
-
   void reportDoubleClose(SymbolRef FileDescSym,
                          const CallEvent &Call,
                          CheckerContext &C) const;
 
   void reportLeaks(ArrayRef<SymbolRef> LeakedStreams, CheckerContext &C,
                    ExplodedNode *ErrNode) const;
 
   bool guaranteedNotToCloseFile(const CallEvent &Call) const;
public:
   bool VisitSymbol(SymbolRef sym) override {
     state = state->remove<StreamMap>(sym);
     return true;
   }
 };
 } // end anonymous namespace
 
 SimpleStreamChecker::SimpleStreamChecker()
-    : IIfopen(nullptr), IIfclose(nullptr) {
+    : OpenFn("fopen"), CloseFn("fclose", 1) {
   // Initialize the bug types.
   DoubleCloseBugType.reset(
       new BugType(this, "Double fclose", "Unix Stream API Error"));
 
   LeakBugType.reset(
       new BugType(this, "Resource Leak", "Unix Stream API Error"));
   // Sinks are higher importance bugs as well as calls to assert() or exit(0).
   LeakBugType->setSuppressOnSink(true);
 }
 
 void SimpleStreamChecker::checkPostCall(const CallEvent &Call,
                                         CheckerContext &C) const {
-  initIdentifierInfo(C.getASTContext());
-
   if (!Call.isGlobalCFunction())
     return;
 
-  if (Call.getCalleeIdentifier() != IIfopen)
+  if (!C.isCalled(Call, OpenFn))
     return;
 
   // Get the symbolic value corresponding to the file handle.
   SymbolRef FileDesc = Call.getReturnValue().getAsSymbol();
   if (!FileDesc)
     return;
 
   // Generate the next transition (an edge in the exploded graph).
   ProgramStateRef State = C.getState();
   State = State->set<StreamMap>(FileDesc, StreamState::getOpened());
   C.addTransition(State);
 }
 
 void SimpleStreamChecker::checkPreCall(const CallEvent &Call,
                                        CheckerContext &C) const {
-  initIdentifierInfo(C.getASTContext());
-
   if (!Call.isGlobalCFunction())
     return;
 
-  if (Call.getCalleeIdentifier() != IIfclose)
-    return;
-
-  if (Call.getNumArgs() != 1)
+  if (!C.isCalled(Call, CloseFn))
     return;
 
   // Get the symbolic value corresponding to the file handle.
   SymbolRef FileDesc = Call.getArgSVal(0).getAsSymbol();
   if (!FileDesc)
     return;
 
   // Check if the stream has already been closed.
for (InvalidatedSymbols::const_iterator I = Escaped.begin(),
 
     // The symbol escaped. Optimistically, assume that the corresponding file
     // handle will be closed somewhere else.
     State = State->remove<StreamMap>(Sym);
   }
   return State;
 }
 
-void SimpleStreamChecker::initIdentifierInfo(ASTContext &Ctx) const {
-  if (IIfopen)
-    return;
-  IIfopen = &Ctx.Idents.get("fopen");
-  IIfclose = &Ctx.Idents.get("fclose");
-}
-
 void ento::registerSimpleStreamChecker(CheckerManager &mgr) {
   mgr.registerChecker<SimpleStreamChecker>();
 }

lib/StaticAnalyzer/Core/CheckerContext.cpp

 //== CheckerContext.cpp - Context info for path-sensitive checkers-----------=//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines CheckerContext that provides contextual info for
 //  path-sensitive checkers.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Lex/Lexer.h"
 
 using namespace clang;
 using namespace ento;
 
 const FunctionDecl *CheckerContext::getCalleeDecl(const CallExpr *CE) const {
   ProgramStateRef State = getState();
   const Expr *Callee = CE->getCallee();
   SVal L = State->getSVal(Callee, Pred->getLocationContext());
   return L.getAsFunctionDecl();
 }
 
 StringRef CheckerContext::getCalleeName(const FunctionDecl *FunDecl) const {
   if (!FunDecl)
     return StringRef();
   IdentifierInfo *funI = FunDecl->getIdentifier();
   if (!funI)
     return StringRef();
   return funI->getName();
 }
 
+bool CheckerContext::isCalled(const CallEvent &Call,
+                              const CallDescription &CD) {
+  assert(Call.getKind() != CE_ObjCMessage && "Obj-C methods are not supported");
+  if (!CD.II)
+    CD.II = &getASTContext().Idents.get(CD.FuncName);
+  if (Call.getCalleeIdentifier() != CD.II)
+    return false;
+  return (CD.RequiredArgs == CallDescription::NoArgRequirement ||
+          CD.RequiredArgs == Call.getNumArgs());
+}
 
 bool CheckerContext::isCLibraryFunction(const FunctionDecl *FD,
                                         StringRef Name) {
   // To avoid false positives (Ex: finding user defined functions with
   // similar names), only perform fuzzy name matching when it's a builtin.
   // Using a string compare is slow, we might want to switch on BuiltinID here.
   unsigned BId = FD->getBuiltinID();
   if (BId != 0) {
     if (Name.empty())
       return true;
     StringRef BName = FD->getASTContext().BuiltinInfo.getName(BId);
     if (BName.find(Name) != StringRef::npos)
       return true;
   }
 
   const IdentifierInfo *II = FD->getIdentifier();
   // If this is a special C++ name without IdentifierInfo, it can't be a