[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

309 ↗

(On Diff #44110)

"is call" -> "is a call"

lib/StaticAnalyzer/Core/CheckerContext.cpp

41 ↗

(On Diff #44110)

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

47 ↗

(On Diff #44110)

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

57 ↗

(On Diff #44110)

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 ↗

(On Diff #44325)

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 ↗

(On Diff #44325)

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

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

Not sure. Maybe it's an omission.

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

enum CallEventKind {
   CE_END_FUNCTION_CALLS = CE_CXXAllocator,
   CE_Block,
   CE_ObjCMessage
 };
 
 class CallEvent;
 class CallEventManager;
 
+/// This class represents a description of a function call using the number of
+/// arguments and the name of the function.
+class CallDescription {
+  friend CallEvent;
+  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) {}
+};
+
 template<typename T = CallEvent>
 class CallEventRef : public IntrusiveRefCntPtr<const T> {
 public:
   CallEventRef(const T *Call) : IntrusiveRefCntPtr<const T>(Call) {}
   CallEventRef(const CallEventRef &Orig) : IntrusiveRefCntPtr<const T>(Orig) {}
 
   CallEventRef<T> cloneWithState(ProgramStateRef State) const {
     return this->get()->template cloneWithState<T>(State);
bool isInSystemHeader() const {
     // Special case for implicitly-declared global operator new/delete.
     // These should be considered system functions.
     if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D))
       return FD->isOverloadedOperator() && FD->isImplicit() && FD->isGlobal();
 
     return false;
   }
 
+  /// \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 CallDescription &CD) const;
+
   /// \brief Returns a source range for the entire call, suitable for
   /// outputting in diagnostics.
   virtual SourceRange getSourceRange() const {
     return getOriginExpr()->getSourceRange();
   }
 
   /// \brief Returns the value of a given argument at the time of the call.
   virtual SVal getArgSVal(unsigned Index) const;

cfe/trunk/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 (!Call.isCalled(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 (!Call.isCalled(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>();
 }

cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp

ProgramPoint CallEvent::getProgramPoint(bool IsPreVisit,
   assert(D && "Cannot get a program point without a statement or decl");
 
   SourceLocation Loc = getSourceRange().getBegin();
   if (IsPreVisit)
     return PreImplicitCall(D, Loc, getLocationContext(), Tag);
   return PostImplicitCall(D, Loc, getLocationContext(), Tag);
 }
 
+bool CallEvent::isCalled(const CallDescription &CD) const {
+  assert(getKind() != CE_ObjCMessage && "Obj-C methods are not supported");
+  if (!CD.II)
+    CD.II = &getState()->getStateManager().getContext().Idents.get(CD.FuncName);
+  if (getCalleeIdentifier() != CD.II)
+    return false;
+  return (CD.RequiredArgs == CallDescription::NoArgRequirement ||
+          CD.RequiredArgs == getNumArgs());
+}
+
 SVal CallEvent::getArgSVal(unsigned Index) const {
   const Expr *ArgE = getArgExpr(Index);
   if (!ArgE)
     return UnknownVal();
   return getSVal(ArgE);
 }
 
 SourceRange CallEvent::getArgSourceRange(unsigned Index) const {