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