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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to