[PATCH] D127306: [analyzer] Treat system globals as mutable if they are not const

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D127306#3566984 <https://reviews.llvm.org/D127306#3566984>, @steakhal wrote:

> In D127306#3566761 <https://reviews.llvm.org/D127306#3566761>, @martong wrote:
>
>>> In theory, the engine should propagate taint in default eval call. If that would be the case, we would still have this report.
>>
>> How complex would it be to add the taint propagation to the engine/evalCall? Do you see that feasible as a parent patch of this?
>
> I see your point, but I don't think we should delay D124244 <https://reviews.llvm.org/D124244> because of that. By landing this, we could also land that. And the change you propose might take significantly longer to make and verify.
> I could make some prototypes, but I'm not making promises.

Ok.

One concern. Can we decide for a global **const system** variable if that it is a //system// variable? Consider `my_const_system_global`, it will be in `GlobalImmutableSpaceRegion`, however, it is also a //system// variable. Would it make sense to have a GlobalImmutable**System**SpaceRegion?



================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:979-980
+    if (Ty.isConstQualified() && Ty->isArithmeticType()) {
       // TODO: We could walk the complex types here and see if everything is
       // constified.
+      sReg = getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
----------------
Is this comment still meaningful? I don't think so because if the type is const, then it does not matter what sub-types it has.


================
Comment at: clang/test/Analysis/globals-are-not-always-immutable.c:14
+void test_errno() {
+  errno = 2;
+  clang_analyzer_eval(errno == 2); // expected-warning {{TRUE}}
----------------
I am wondering if we should have another test for `assert(errno == 2);`. Because here we add a binding to the store, however, with the `assert` we add a constraint to the range based constraint manager and these are very much different cases.

The same applies to the below tests as well.


================
Comment at: clang/test/Analysis/globals-are-not-always-immutable.c:55
+  invalidate_globals();
+  clang_analyzer_eval(my_mutable_system_global == 2); // expected-warning {{UNKNOWN}} It was previously TRUE.
+}
----------------
Please highlight that this is the essence of the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127306