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

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:767-769
     assert(isa<UnknownSpaceRegion>(sreg) || isa<HeapSpaceRegion>(sreg) ||
-           isa<GlobalSystemSpaceRegion>(sreg));
+           isa<GlobalSystemSpaceRegion>(sreg) ||
+           isa<GlobalImmutableSpaceRegion>(sreg));
----------------
steakhal wrote:
> Please merge these into a single `isa<T1, T2,...>()`.
error: macro "assert" passed 4 arguments, but takes just 1
`assert(isa<UnknownSpaceRegion, HeapSpaceRegion, GlobalSystemSpaceRegion, GlobalImmutableSpaceRegion>(sreg));`

So I had to add extra ()-s around `isa`, I guess macro expands weirdly? 


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt:68-70
   MIGChecker.cpp
+  cert/ModelConstQualifiedReturnChecker.cpp
   MoveChecker.cpp
----------------
steakhal wrote:
> Put this in the right alphabetical place.
I think it is in the right alphabetical place not considering cert/ (other certs are in similar order), but maybe I should remove the cert directory, what do you think? 
It was created by me a few years ago, but I don't see a point now anymore.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp:27-29
+  BugType ImmutableMemoryBind{this,
+                              "Immutable memory should not be overwritten",
+                              categories::MemoryError};
----------------
steakhal wrote:
> You could expose a pointer to this by creating a `const BugType *getBugType() const;` getter method, which could be used by other checkers.
I did something similar based on SmartPtrChecker.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp:43
+
+  if (!isa<GlobalImmutableSpaceRegion>(R->getMemorySpace()))
+    return;
----------------
NoQ wrote:
> 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.
Could you give an example? I tried with the following snippet and it didn't give a warning

```
int a = 1, b = 1;
void foo(int *p) {
  a = 2;
  p[b++] = 3;
  int *c = &a;
  *c = 5;
}
```


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp:52
+      ImmutableMemoryBind, "modifying immutable memory", ErrorNode);
+  Report->markInteresting(R);
+  C.emitReport(std::move(Report));
----------------
NoQ wrote:
> 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.
What expression should I use for tracking? I tried to simply cast `S` to `Expr`, but it didn't really work. 
Then I came up with something ugly: cast `S` to `BinaryOperator` -> `getLHS` -> `getSubExpr` -> `ignoreImpCasts`.  


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

https://reviews.llvm.org/D124244