ASDenysPetrov added a comment.
In D110927#3117728 <https://reviews.llvm.org/D110927#3117728>, @steakhal wrote:
> For testing this I would recommend using a separate test file.
> That being said, you should avoid globals even in tests when you can. The
> distance between its declaration and use just makes it harder to comprehend
> and reason about.
I'll add a new tests file.
> You could have a parameter, and take its address to accomplish your
> reinterpret casts and type puns.
Do you mean:
void foo(signed char * ptr) {
ptr = (signed char *)glob_arr2;
}
instead of
void foo() {
auto *ptr = (signed char *)glob_arr2;
}
?
If so, IMO it doesn't matter.
> BTW your patch crashes on the test suite:
> `initialization.cpp:242`:
I caught it. Thanks! I wonder how it slipped through unnoticed.
================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1661
+/// E.g. for `const int[1][2][3];` returns `int`.
+QualType getConstantArrayRootElement(const ConstantArrayType *CAT) {
+ assert(CAT && "ConstantArrayType should not be null");
----------------
steakhal wrote:
> Maybe //unwrapConstantArrayTypes()//?
I think about //getConstantArrayRoot**Type**//. IMO such name distinctly tells
its intention and purpose.
================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1790
+ // paper P1839R0 be considered by the committee.
+ if ((Index != 0))
+ return false;
----------------
steakhal wrote:
> Even though you are technically correct, how can you know that the pointer
> you try to dereference actually points to the beginning of the object?
> Consider something like this:
>
> ```lang=C++
> void callback(void *payload) {
> // lets skip the first char object...
> int *num = (int*)((char*)payload + 1);
> clang_analyzer_dump(num); // Element{Element{SymRegion{payload}, 1, char},
> 0, int}
> }
> ```
> how can you know that the pointer you try to dereference actually points to
> the beginning of the object?
We look at the offset of the region. In your example it is //1//. And it is UB.
Unfortinatelly the Standard forbids to do such accesses due to the different
strict access rules. I recommend this talk https://youtu.be/_qzMpk-22cc . I
took inspiration from there.
================
Comment at: clang/test/Analysis/initialization.cpp:295-299
+void glob_cast_opposite_sign1() {
+ auto *ptr = (unsigned int *)glob_arr2;
+ auto x1 = ptr[0]; // no-warning
+ auto x2 = ptr[1]; // expected-warning{{garbage or undefined}}
+}
----------------
steakhal wrote:
> I think it's not correct.
>
> `glob_arr2` refers to an object of dynamic type `int[2]`.
> And the pointer decayed from it is `int *`, which has //similar type// to
> `unsigned *` what you are using to access the memory.
> Since they are //similar//, this operation should work for all the valid
> indices, thus `ptr[0]`, `ptr[1]`, `ptr[2]`, `ptr[3]` should all be valid.
>
>
> glob_arr2 refers to an object of dynamic type int[2].
`glob_arr2` has an extent of 4.
> And the pointer decayed from it is int *, which has similar type to unsigned
> * what you are using to access the memory.
Yes, they are the same and it perfectly suits to
http://eel.is/c++draft/basic.lval#11 . But still you can't access other element
then the first one according to http://eel.is/c++draft/basic.compound#3 : //For
purposes of pointer arithmetic ([expr.add]) and comparison ([expr.rel],
[expr.eq]), [...] an object of type T that is not an array element is
considered to belong to an array with one element of type T. //
`unsigned int` and `int` are different types according to
http://eel.is/c++draft/basic.fundamental#2 . The object of type `unsigned int`
is NOT an array, beacuse there is no array of type `unsigned int`. Hence you
can only only access the first and a single element of type `unsigned int`.
================
Comment at: clang/test/Analysis/initialization.cpp:311-314
+void glob_cast_invalid3() {
+ auto *ptr = (char32_t *)glob_arr2;
+ auto x = ptr[0]; // expected-warning{{garbage or undefined}}
+}
----------------
steakhal wrote:
> Please also try `char8_t`.
Correct. We should check it. It should be a different type as well
(http://eel.is/c++draft/basic.fundamental#9).
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
