[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;
----------------
zukatsinadze wrote:
> 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;
> }
> ```
I was thinking about constants, i.e. `const int a` etc.


================
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 = {
----------------
NoQ wrote:
> 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.
Yeah so the reason why this was made an `evalCall` is because we needed to replace the memory space for the return value. I think the right solution is to re-implement SymRegion memory spaces as traits, so that they could be updated during analysis at any time (under the natural condition that they only ever get narrower). So like dynamic type, just dynamic memory space.


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

https://reviews.llvm.org/D124244