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