[PATCH] D124244: [analyzer] add StoreToImmutable and ModelConstQualifiedReturn checkers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp:43
+
+  if (!isa<GlobalImmutableSpaceRegion>(R->getMemorySpace()))
+    return;
----------------
This check catches a lot more regions than the ones produced by the other checker. In particular, it'll warn on all global constants. Did you intend this to happen? You don't seem to have tests for that.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp:52
+      ImmutableMemoryBind, "modifying immutable memory", ErrorNode);
+  Report->markInteresting(R);
+  C.emitReport(std::move(Report));
----------------
I also recommend `trackExpressionValue` to make sure we have all reassignments highlighted as the value gets bounced between pointer variables. The user will need proof that the pointer actually points to const memory.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp:30-43
+class ModelConstQualifiedReturnChecker : public Checker<eval::Call> {
+private:
+  void evalConstQualifiedReturnCall(const CallEvent &Call,
+                                    CheckerContext &C) const;
+
+  // SEI CERT ENV30-C
+  const CallDescriptionSet ConstQualifiedReturnFunctions = {
----------------
steakhal wrote:
> I can see a small issue by `evaCall`-ing these functions.
> The problem is that we might not model all aspects of these functions, thus the modeling can cause harm to the analysis. E.g. by not doing invalidation where we actually would need invalidation to kick in, or anything else really.
> Consequently, another problem is that no other checker can evaluate these, since we evaluate them here.
> Thus, fixing such improper modeling could end up in further changes down the line.
> Unfortunately, I don't have any better ATM, so let's go with this `evalCall` approach.
Right, this definitely shouldn't be `evalCall`. Post-call seems to be sufficient for this checker.


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

https://reviews.llvm.org/D124244