[PATCH] D69948: [Checkers] Added support for freopen to StreamChecker.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:176
+  const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
+  ConstraintManager &CM = C.getConstraintManager();
+  auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
----------------
balazske wrote:
> baloghadamsoftware wrote:
> > The four lines above are typical cases for using auto, since the type is duplicated in the line. See: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
> This is not totally clear if we can trust the pattern that at `auto X = Y.getXxx()` type of `X` will be `Xxx`.
@balazske +1. I'm personally in favor of using `auto` here, but the community's stance is to use `auto` only for `dyn_cast`/`getAs` etc. calls where type is explicitly spelled out, and also for iterators where the type is going to be too long and annoying to spell out (but in the latter case i'm suddenly in favor of spelling it out, as i always get confused about how many `*`s do i need to add in order to properly dereference whatever's in there).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:189-190
+  // Check if error was generated.
+  if (C.isDifferent())
+    return;
+
----------------
That's actually one more good reason to move all `checkNullStream` to `PreCall`: less spaghetti. The same checker should feel free to subscribe for the same function in multiple different callbacks and do different things in them.

Rule of a thumb (i'm not sure if it's //always// applicable but it's applicable to most "typical" checkers even if they aren't currently written this way):
1. Pre-condition checking goes in PreCall;
2. Modeling that affects everybody goes in evalCall;
3. Checker's internal bookkeeping goes in PostCall.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:192-204
+  DefinedSVal RetVal =
+      SValBuilder.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount())
+          .castAs<DefinedSVal>();
+  SymbolRef RetSym = RetVal.getAsSymbol();
+  assert(RetSym && "RetVal must be a symbol here.");
+
+  SymbolRef StreamSym = StreamVal->getAsSymbol();
----------------
It looks like you're creating a symbol only to assume that it's null. You should bind a concrete null (i.e., `loc::ConcreteInt`) instead.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:208
+      StateRetNull->set<StreamMap>(RetSym, StreamState::getOpenFailed());
+  if (StreamSym)
+    StateRetNull =
----------------
The null `StreamVal` is going to be caught by `checkNullStream()`. What are the other possibilities here? Concrete `FILE *` that isn't null?

I guess it's possible but it probably deserves a comment describing that this is actually a very rare scenario. Maybe a test that involves a value like `((FILE *)0x12345)`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:219-220
+  if (StreamSym)
+    StateRetNotNull =
+        StateRetNotNull->set<StreamMap>(StreamSym, StreamState::getOpened());
+  C.addTransition(StateRetNotNull);
----------------
What does really happen when you `freopen` a stream that isn't already open? It sounds like it's doing a `fclose` blindly, so we might as well report a double close in this case, and then we don't need to update the program state because it's already opened(?) Though later we still might want to update the program state if we start keeping track of the filename or the mode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69948