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

Wed May 18 03:22:32 PDT 2022

steakhal added a comment.

I found this report at `vim/src/term.c` (https://github.com/vim/vim.git at `v8.2.1920`):
F23102904: image.png <https://reviews.llvm.org/F23102904>

I'm not sure what the exact type of `BC` global variable is but I think it's declared as either `extern char *BC;` or just `char *BC;`.
`empty_option` is declared as either `extern unsigned char *empty_option;` or `unsigned char *empty_option = (unsigned char *)"";`

Could you please have a look at this FP @zukatsinadze?

================
Comment at: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt:68-70
   MIGChecker.cpp
+  cert/ModelConstQualifiedReturnChecker.cpp
   MoveChecker.cpp
----------------
zukatsinadze wrote:
> 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.
Leave it here.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp:52
+      ImmutableMemoryBind, "modifying immutable memory", ErrorNode);
+  Report->markInteresting(R);
+  C.emitReport(std::move(Report));
----------------
zukatsinadze wrote:
> 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`.  
Transitive interestingness might be related to this: D125362

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

https://reviews.llvm.org/D124244