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

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