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

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

Thanks, this looks good to me! But, please wait for an approve from @steakhal as well.



================
Comment at: clang/test/Analysis/initialization.c:103
+void glob_arr_index4() {
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}
----------------
ASDenysPetrov wrote:
> steakhal wrote:
> > 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?
> I'm not sure I caught your thoughts.
> But I think the things is much simplier:
> `clang_analyzer_eval` can only produce `UNKNOWN` or `TRUE` or `FALSE`. If we know the constraint of `glob_arr_no_init[2]` we return `TRUE` or `FALSE`, and `UNKNOWN` otherwise.
> But in fact I should use `clang_analyzer_dump` here instead of `clang_analyzer_eval`. This is actually my fault. I'll update this.
> Could you please check if it did result in unknown before your patch, or you did introduce this behavior?

I've just checked it, it was `Unknown` before this patch as well. 
And actually, that is wrong because the array has static storage duration and as such, we know that it is initialized with zeros according to the C standard. But, that should be addressed in a follow-up patch (if someone has the capacity).
https://stackoverflow.com/questions/32708161/value-of-uninitialized-elements-in-array-of-c-language/32708288


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

https://reviews.llvm.org/D106681