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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
