[PATCH] D106681: [analyzer][NFCI] Move a block from `getBindingForElement` to separate functions

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

steakhal added a comment.

In D106681#3074779 <https://reviews.llvm.org/D106681#3074779>, @ASDenysPetrov wrote:

> In D106681#3074678 <https://reviews.llvm.org/D106681#3074678>, @steakhal wrote:
>
>> I think it's fine, maybe `NFCi` is would be slightly more accurate, while stating the minor behavior change and the reason for doing so in the patch summary could further improve the visibility of this issue.
>>
>> That being said, since it actually changes some behavior, I'd like to request some tests covering the following (uncovered lines):
>> L1646, L1689, L1699
>
> For **L1646** currently I'm not sure about the exact test, since it is a heritage of the old code, so it just jumped here from the past. If you could give an example I would appreciate this.
> For **L1689** I'll add it.
> For **L1699** I've added //TODO// cases in D104285 <https://reviews.llvm.org/D104285>.

I just wanted you to think about these cases.
BTW this should work for **L1646**:

  extern const int arr[]; // Incomplete array type
  void top() {
    (void)arr[42];
  }

And I'm okay with the rest of the uncovered lines.



================
Comment at: clang/test/Analysis/initialization.c:103
+void glob_arr_index4() {
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}
----------------
I'm pretty sure we should not get `Unknown` for simply loading from a global variable.
That would imply that loading two times subsequently we could not prove that the value remained the same.
But that should remain the same, thus compare equal unless some other code modifier that memory region.

That could happen for two reasons:
1) There is a racecondition, and another thread modified that location. But that would be UB, so that could not happen.
2) The global variable is //volatile//, so the hardware might changed its content- but this global is not volatile so this does not apply.

That being said, this load should have resulted in a //fresh// conjured symbolic value instead of //unknown//.
Could you please check if it did result in //unknown// before your patch, or you did introduce this behavior?


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

https://reviews.llvm.org/D106681