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

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@steakhal

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

Thanks! Now I see. Shame on me =)

> We know where it points to (aka. the pointer's value is conjured), but we don't know what the value if there.

Absolutely right. I looked at this patch after a while and don't currently see a proper solution.

> I feel like this problem should be handled by some sort of invalidation mechanism.

Definitely it should be some criteria/mark/flag about the region invalidation. Maybe someone more confident could prompt us how to.



================
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
----------------
steakhal wrote:
> Why do we test this?
> Here we can //reason// about the index.
> This patch preserves the current behavior relating to this.
This is a root case of the bug which this patch came from. I decided to vary it by using an explicit index.


================
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]}}
----------------
steakhal wrote:
> 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.
My intention is to retain `index` inside brackets. Just to check that `index` changed indeed.


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

https://reviews.llvm.org/D81254