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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.


================
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
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1657-1671
+      (((OrigT == Ctx.CharTy && ThroughT == Ctx.UnsignedCharTy) ||
+        (OrigT == Ctx.SignedCharTy && ThroughT == Ctx.UnsignedCharTy) ||
+        (OrigT == Ctx.ShortTy && ThroughT == Ctx.UnsignedShortTy) ||
+        (OrigT == Ctx.IntTy && ThroughT == Ctx.UnsignedIntTy) ||
+        (OrigT == Ctx.LongTy && ThroughT == Ctx.UnsignedLongTy) ||
+        (OrigT == Ctx.LongLongTy && ThroughT == Ctx.UnsignedLongLongTy) ||
+        (ThroughT == Ctx.CharTy && OrigT == Ctx.UnsignedCharTy) ||
----------------
I would rather `Ctx.getCorrespondingUnsignedType()` on both of the types and if they compare equal, that would mean that they differed only in signedness.
This is probably more costly, and that function will assert that it's a scalar type or something, so it would make sense to check `OrigT == ThroughT` separately earlier than this.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1672-1677
+       // NOTE: C++20 6.8.2(3.4) [basic.compound]:
+       //  An object of type T that is not an array element is considered to
+       //  belong to an array with one element of type T.
+       // Hence, the first element can be retrieved only. At least untill a
+       // paper P1839R0 be considered by the committee.
+       (Index == 0));
----------------
You should probably early return on this.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1758
+              // type.
+              QualType ElemT = Ctx.getCanonicalType(R->getElementType());
+              if (!canAccessStoredValue(ArrT, ElemT, I))
----------------
If you already compute the //canonical type// why do you recompute in the `canAccessStoredValue()`?
You could simply assert that instead.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1759-1760
+              QualType ElemT = Ctx.getCanonicalType(R->getElementType());
+              if (!canAccessStoredValue(ArrT, ElemT, I))
+                return UndefinedVal();
+
----------------
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?


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

https://reviews.llvm.org/D110927