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

Thu Jun 9 08:48:14 PDT 2022

martong added a comment.

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

> In D127306#3570083 <https://reviews.llvm.org/D127306#3570083>, @martong wrote:
>
>> 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?
>
> Well, there is more to this story. Constness and storage addressspace should be two different propery. Memspaces supposed to represent the latter. The mutability should have been designed as a GDM trait instead. It just happens to be that on some platforms there is a segment which is mapped as  read only. As far as memspaces goes, it shouldn't be important.

Okay, makes sense. Maybe we could go into that direction in a later patch stack. 
So, I think this patch is indeed a step forward, nice work.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127306