[PATCH] D110927: [analyzer] Access stored value of a constant array through a pointer to another type

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@steakhal I'll address all of your remarks. Thanks a lot!



================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1630-1641
+/// Returns true if the stored value can be accessed through the pointer to
+/// another type:
+///  const int arr[42] = {};
+///  auto* pchar     = (char*)arr;
+///  auto* punsigned = (unsigned int*)arr;
+///  auto* pshort    = (short*)arr;
+///  auto x1 = pchar[0];     // valid
----------------
steakhal wrote:
> This is basically the //strict-aliasing// rule, we could probably mention this.
> Although, I don't mind the current name.
> You probably know about it, but Shafik made a fancy [[ https://gist.github.com/shafik/848ae25ee209f698763cffee272a58f8 | GitHub Gist ]] about it.
> The only thing I missed was checking //type similarity// according to [[https://eel.is/c++draft/basic.lval#11 | basic.lval ]], but I'm not exactly sure if we should check that here.
> You probably know about it, but Shafik made a fancy GitHub Gist about it.
Yes, this is the one of those things which inspired me to take care of aliasing as a part of RegionStoreManager improvements.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1759-1760
+              QualType ElemT = Ctx.getCanonicalType(R->getElementType());
+              if (!canAccessStoredValue(ArrT, ElemT, I))
+                return UndefinedVal();
+
----------------
steakhal wrote:
> Even though I agree with you, I think it would be useful to hide this behavior behind an analyzer option.
> There is quite a lot code out in the wild that violate the //strict-aliasing// rule and they probably pass the `-fno-strict-aliasing` compiler flag to accommodate this in codegen. AFAIK Linux is one of these projects for example.
> So, I think there is a need to opt-out of this and/or bind the behavior to the presence of the mentioned compiler flag.
> 
> By not doing this, the user would get //garbage// value reports all over the place.
> @NoQ @martong WDYT?
> There is quite a lot code out in the wild that violate the strict-aliasing rule 
Agree.
> By not doing this, the user would get garbage value reports all over the place.
Definitely.
Using the flag is a good option. But the question whether to use existing `-fno-strict-aliasing` or introduce a new one?


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

https://reviews.llvm.org/D110927