[PATCH] D81254: [analyzer] Produce symbolic values for C-array elements

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

steakhal added a comment.

In D81254#2122449 <https://reviews.llvm.org/D81254#2122449>, @ASDenysPetrov wrote:

> @NoQ, thanks for the examples.
>
> I didn't get the first one. How do you get to the "//In reality we don't know//", if we don't touch `a[index1]`:

If `index1` and `index2` has the **same** value, we should not be confident that the `x == y` holds.

>   void foo(int *a); // Potentially modifies elements of 'a'.
>   void fooRef(int *&a); // Potentially modifies elements of 'a'.
>   
>   void test2(int *a) {
>     // a - &SymRegion{reg_$6<int * a>}
>     foo(a); // after this, CSA doesn't change a.
>     // a - &SymRegion{reg_$6<int * a>}
>     fooRef(a); // after this, CSA changes a.
>     // a - &SymRegion{conj_$10{int *, LC1, S925, #1}}
>   }

I don't think this is the right way expressing this.
AFAIK this means that we //don't// know where the resulting pointer points to - which is not true.
We know where it points to (aka. the pointer's value is conjured), but we don't know what the value if there.

Unfortunately, I don't know the solution to your problem, but I'm pretty confident that this is something else.
However, I'm really interested in where this discussion goes.

---

I feel like this problem should be handled by some sort of invalidation mechanism.
Take my comments on these with a pinch of salt though - I'm really not experienced in this :|



================
Comment at: clang/test/Analysis/PR9289.cpp:13-21
+int index_int(const int *a, int index) {
+  int var;
+  int ret = 0;
+  if (a[42] < 2)
+    var = 1;
+  if (a[42] < 2)
+    ret = var; // no warning about garbage value
----------------
Why do we test this?
Here we can //reason// about the index.
This patch preserves the current behavior relating to this.


================
Comment at: clang/test/Analysis/PR9289.cpp:36-40
+  if (a[index] < 2)
+    var = 1;
+  index = index2;
+  if (a[index] < 2)
+    ret = var; // expected-warning{{Assigned value is garbage or undefined [core.uninitialized.Assign]}}
----------------
I'm not sure why do you overwrite the `index` if you could simply index with `index2` instead in the following expression. That would make no difference, only make the test more readable IMO.
Same comment for all the`*_change` test cases.


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

https://reviews.llvm.org/D81254