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