[PATCH] D27918: [analyzer] OStreamChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Hello,

Sorry again for the delays, thank you for your patience. Your checker is in good shape, very well-structured and easy to follow, you definitely know your stuff, and my comments here are relatively minor.

Are you planning on making more varied warning messages, eg. specifying which format modifiers are forgotten, and where in the code they were previously set? The latter could be done by using the "bug reporter visitor" mechanism.

I'm sure we can at the very least eventually land this checker into the `optin` package, which is the new package for warnings that aren't on by default, but are advised to be turned on by the users if the codebase intends to use certain language or API features or contracts (unlike the `alpha` package which is for checkers undergoing development to be ultimately moved out of alpha). For moving out of alpha, i think the work on the warning messages needs to be done, and the checker needs to be tested on a large codebase to make sure that the false positive rate is low (i guess you already did it), and that's pretty much it :) On a side note, i've been recently thinking of making `optin` checkers more visible to the `scan-build` users, eg. mentioning in the log that certain checks may be worth enabling.



================
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:263-282
+  mutable IdentifierInfo *II_BasicOstream, *II_Flags, *II_Setf, *II_Unsetf,
+      *II_Setiosflags, *II_Resetiosflags, *II_Precision, *II_SetPrecision,
+      *II_BaseField, *II_Hex, *II_Dec, *II_Oct, *II_AdjustField, *II_Left,
+      *II_Right, *II_Internal, *II_BoolAlpha, *II_NoBoolAlpha, *II_ShowPos,
+      *II_NoShowPos, *II_ShowBase, *II_NoShowBase, *II_UpperCase,
+      *II_NoUpperCase, *II_ShowPoint, *II_NoShowPoint, *II_FloatField,
+      *II_Fixed, *II_Scientific;
----------------
If you ever want to extend the `CallDescription` class to cover your use case, please let us know :)

While most of these aren't functions, the approach used in this class could be used here as well - making initialization code shorter.


================
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:387
+
+static Optional<int> tryEvaluateAsInt(const Expr *E, ProgramStateRef S,
+                                      CheckerContext C) {
----------------
I think you should use `SimpleSValBuilder::getKnownValue()`. The `EvaluateAsInt` part doesn't add much because the analyzer's engine should already be more powerful than that. On the other hand, the `ConstraintManager::getSymVal()` thing, which is already done in `SimpleSValBuilder::getKnownValue()`, may be useful.


================
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:403
+
+  return Optional<int>();
+}
----------------
By the way, there's the `None` thing that represents any empty optional.


================
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:513
+
+bool OStreamFormatChecker::evalCall(const CallExpr *CE,
+                                    CheckerContext &C) const {
----------------
One of the practical differences between `checkPostCall` and `evalCall` is that in the latter you have full control over the function execution, including invalidations that the call causes. Your code not only sets the return value, but also removes invalidations that normally happen. Like, normally when an unknown function is called, it is either inlined and therefore modeled directly, or destroys all information about any global variables or heap memory that it might have touched. By implementing `evalCall`, you are saying that the only effect of calling `operator<<()` on a `basic_ostream` is returning the first argument lvalue, and nothing else happens; inlining is suppressed, invalidation is suppressed.

I'm not sure if it's a good thing. On one hand, this is not entirely true, because the operator changes the internal state of the stream and maybe of some global stuff inside the standard library. On the other hand, it is unlikely to matter in practice, as far as i can see.

Would it undermine any of your efforts here if you add a manual invalidation of the stream object and of the `GlobalSystemSpaceRegion` memory space (you could construct a temporary `CallEvent` and call one of its methods if it turns out to be easier)?

I'm not exactly in favor of turning this into `checkPostCall()` because binding expressions in that callback may cause hard-to-notice conflicts across multiple checkers. Some checkers may even take the old value before it's replaced. For `evalCall()` we at least have an assert.

If you intend to keep the return value as the only effect, there's option of making a synthesized body in our body farm, which is even better at avoiding inter-checker conflicts. Body farms were created for that specific purpose, even though they also have their drawbacks (sometimes `evalCall` is more flexible than anything we could synthesize, eg. D20811).

If you have considered all the alternative options mentioned above and rejected them, could you add a comment explaining why? :)


================
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:551
+  // We don`t warn if the warning would came from the std namespace.
+  bool ScopeIsInSTD = Scope->isInStdNamespace();
+
----------------
We have a global analyzer flag to suppress those, `-analyzer-config suppress-c++-stdlib=true`, which has been recently turned on by default.  I don't think that duplicating this suppression into every checker separately is worth it.


================
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:677-682
+  // The standard library header wraps the first arg in a
+  // CXXConstructExpr
+  const auto *ConstructExpr = dyn_cast<CXXConstructExpr>(LeftShift->getArg(1));
+
+  // The test header yields a MaterializeTemporaryExpr directly.
+  const auto *MTE = dyn_cast<MaterializeTemporaryExpr>(LeftShift->getArg(1));
----------------
This stuff is often very complex, and the AST here is often quirky.

Could you expand these comments with a few code samples and/or -ast-dump fragments?

I just hope i didn't break this approach of yours with my recent patches on temporaries (D26839, D30534).


================
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:727
+
+  const MemRegion *StreamMemRegion = getMemRegionForFirstManipArg(OCE, C);
+  const StoredState *TrackedState = S->get<OstreamStateMap>(StreamMemRegion);
----------------
This region may be null in case the argument is `UnknownVal` or `UndefinedVal`. I don't think you intend to have a null pointer in the map here and in a few other places.


================
Comment at: test/Analysis/ostream-format.cpp:52
+            << std::setprecision(6); // The stream state is set here.
+  std::cout << "  Maurer Randomness Test returned value " << std::endl;
+} // OK, stream state is recovered here.
----------------
Did you intend to leave this string here? :)


https://reviews.llvm.org/D27918