steakhal accepted this revision.
steakhal added a comment.
Sorry for blocking the review of this one for so long.
================
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:
> ASDenysPetrov wrote:
> > steakhal wrote:
> > > ASDenysPetrov wrote:
> > > > ASDenysPetrov wrote:
> > > > > steakhal wrote:
> > > > > > martong wrote:
> > > > > > > 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
> > > > > > Oh true. I was actually tricked by the `initialization.cpp:38`,
> > > > > > where I actually caught this. And in that case, you use `dump()`
> > > > > > yet you get `Unknown` as a result. But the issue remains the same.
> > > > > C++ also states about zero-initialization for static storage lifetime
> > > > > duration: http://eel.is/c++draft/basic.start.static#2
> > > > > I think it will be among my next patches.
> > > > > And in that case, you use dump() yet you get Unknown as a result. But
> > > > > the issue remains the same.
> > > > I just realized that I was confused as well :) The `dump` returns a
> > > > symbolic value like `reg_$0<int Element{glob_arr_no_init,2 S64b,int}>`,
> > > > **not** `Unknown`. So my intention of using `eval` was deliberate.
> > > > Anyway we should improve this to produce `FALSE` instead of `UNKNOWN`,
> > > > since it has a //static storage//.
> > > It's a really complex topic. I would highly recommend taking baby steps
> > > to improve this area.
> > >
> > > In C, you might have a chance to accomplish something, but in C++ static
> > > globals might be initialized by running a constructor, which means
> > > arbitrary user-defined code. This is actually why we disabled similar
> > > logic in the `RegionStore::getInitialStore()`. I highly recommend taking
> > > a look.
> > >
> > > Consider this code:
> > > ```lang=C++
> > > // TU 1:
> > > #include <cstdio>
> > > static int a; // zero-initialized initially
> > > int *p2a = &a; // escapes the address of 'a'
> > >
> > > int main() {
> > > printf("%d\n", a); // reports 42
> > > }
> > >
> > > // TU 2:
> > > extern int *p2a;
> > > static bool sentinel = (*p2a = 42, false);
> > > ```
> > Yes, I see what you mean. I'm going to tough the part of C++ in the near
> > future.
> > Yes, I see what you mean. I'm going to tough the part of C++ in the near
> > future.
> *Yes, I see what you mean. I'm **not** going to tough the part of C++ in the
> near future.
Okay, I see now.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106681/new/
https://reviews.llvm.org/D106681
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
