[PATCH] D67706: [clang][analyzer] Using CallDescription in StreamChecker.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:60-61
 private:
-  void Fopen(CheckerContext &C, const CallExpr *CE) const;
-  void Tmpfile(CheckerContext &C, const CallExpr *CE) const;
-  void Fclose(CheckerContext &C, const CallExpr *CE) const;
-  void Fread(CheckerContext &C, const CallExpr *CE) const;
-  void Fwrite(CheckerContext &C, const CallExpr *CE) const;
-  void Fseek(CheckerContext &C, const CallExpr *CE) const;
-  void Ftell(CheckerContext &C, const CallExpr *CE) const;
-  void Rewind(CheckerContext &C, const CallExpr *CE) const;
-  void Fgetpos(CheckerContext &C, const CallExpr *CE) const;
-  void Fsetpos(CheckerContext &C, const CallExpr *CE) const;
-  void Clearerr(CheckerContext &C, const CallExpr *CE) const;
-  void Feof(CheckerContext &C, const CallExpr *CE) const;
-  void Ferror(CheckerContext &C, const CallExpr *CE) const;
-  void Fileno(CheckerContext &C, const CallExpr *CE) const;
-
-  void OpenFileAux(CheckerContext &C, const CallExpr *CE) const;
-
-  ProgramStateRef CheckNullStream(SVal SV, ProgramStateRef state,
-                                 CheckerContext &C) const;
-  ProgramStateRef CheckDoubleClose(const CallExpr *CE, ProgramStateRef state,
-                                 CheckerContext &C) const;
+  typedef void (StreamChecker::*FnCheck)(const CallEvent &,
+                                         CheckerContext &) const;
+
----------------
Szelethus wrote:
> Prefer using. When I wrote D68165, I spent about 10 minutes figuring out how to do it... Ah, the joys of the C++ syntax.
> ```lang=c++
> using FnCheck = void (StreamChecker::*)(const CallEvent &,
>                                         CheckerContext &) const;
> ```
It's actually very easy to remember, it's just an alien kiss smiley ::*)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:127-131
+  for (auto P : Call.parameters()) {
+    QualType T = P->getType();
+    if (!T->isIntegralOrEnumerationType() && !T->isPointerType())
+      return nullptr;
+  }
----------------
balazske wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > balazske wrote:
> > > > Szelethus wrote:
> > > > > I'm not sure why we need this, is it true that *all* stream related functions return a pointer or a numerical value? Are we actually checking whether this really is a library function? If so, this looks pretty arbitrary.
> > > > This comes from code of CStringChecker:
> > > > ```
> > > >   // Pro-actively check that argument types are safe to do arithmetic upon.
> > > >   // We do not want to crash if someone accidentally passes a structure
> > > >   // into, say, a C++ overload of any of these functions. We could not check
> > > >   // that for std::copy because they may have arguments of other types.
> > > > ```
> > > > Still I am not sure that the checker works correct with code that contains similar named but "arbitrary" functions.
> > > Oops, meant to write that ", is it true that *all* stream related functions have only pointer or a numerical parameters?".
> > ...and removing this one, or changing it to an assert?
> This can not be an assert because it is possible to have global functions with same name but different signature. The check can be kept to filter out such cases. The wanted stream functions have only integral or enum or pointer arguments.
Clang shouldn't crash even when the library definition is incorrect.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:156
+void StreamChecker::evalFread(const CallEvent &Call, CheckerContext &C) const {
+  (void)CheckNullStream(Call.getArgSVal(3), C);
 }
----------------
Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > Why the need for `(void)`? `CheckNullSteam` doesn't seem to have an `LLVM_NODISCARD` attribute.
> > I wanted to be sure to get no buildbot compile errors (where -Werror is used).
> They actually break on this?? Let me guess, is it the windows one? :D
I'd rather not discard the return value, but allow callbacks indicate an error when something goes wrong, so that to allow them to abort `evalCall()`. 

But more importantly, note how `CheckNullStream` actually returns a `ProgramStateRef` that was meant to be transitioned into. Which means that the primary execution path actually gets cut off.

So even if buildbots didn't warn on this, i wish they did.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67706