[PATCH] D50892: [analyzer][UninitializedObjectChecker] Correct dynamic type is acquired for record pointees

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:187-191
   // If FR is a pointer pointing to a non-primitive type.
   if (Optional<nonloc::LazyCompoundVal> RecordV =
           DerefdV.getAs<nonloc::LazyCompoundVal>()) {
 
     const TypedValueRegion *R = RecordV->getRegion();
----------------
NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > This looks like one more situation where we dereference a location to get a value and then struggle to get back to the location that we've dereferenced by looking at the value. Can we just use `V`?
> > I've struggled with derefencing for months now -- I'm afraid I just don't really get what you'd like to see here.
> > 
> > Here's what I attempted to implement:
> > I'd like to obtain the pointee's region of a `Loc` region, even if it has to be casted to another type, like through void pointers and `nonloc::LocAsInteger`, and continue analysis on said region as usual.
> > 
> > The trickiest part I can't seem to get right is the acquisition of the pointee region. For the problem this patch attempts to solve, even though `DynT` correctly says that the dynamic type is `DynTDerived2 *`, `DerefdV` contains a region for `DynTBase`.
> > 
> > I uploaded a new patch, D51057, which hopefully settles derefence related issues. Please note that it **does not **replace this diff, as the acquired region is still of type `DynTBase`.
> > 
> > I find understanding these intricate details of the analyzer very difficult, as I found very little documentation about how this works, which often left me guessing what the proper way to do this is. Can you recommend some literature for me on this field?
> > Can you recommend some literature for me on this field?
> 
> This is pretty specific to our analyzer. `SVal`/`SymExpr`/`MemRegion` hierarchy is tightly coupled to implementation details of the `RegionStore`, which is our memory model. There's a paper on it [1]. We have some in-tree documentation of the `RegionStore` [2] (other docs there are also interesting to read). And there's my old workbook [3]. And i guess that's it.
> 
> [1] Xu, Zhongxing & Kremenek, Ted & Zhang, Jian. (2010). A Memory Model for Static Analysis of C Programs. 535-548. 10.1007/978-3-642-16558-0_44.
> [2] https://github.com/llvm-mirror/clang/blob/master/docs/analyzer/RegionStore.txt
> [3] https://github.com/haoNoQ/clang-analyzer-guide/releases/download/v0.1/clang-analyzer-guide-v0.1.pdf
Thank you! I'm aware of these works, and have read them already.

The actual implementation of the analyzer is most described in your book, and I've often got the best ideas from that. However, some very well described things in there have absolutely no documentation in the actual code -- for example, it isn't obvious at all to me what `CompundValue` is, and I was surprised to learn what it does from your guide. Other examples are the difference between `TypedValueRegion`s `getLocationType` and `getValueType`, `SymExpr` in general, and so on.

I think it would also be very valuable to have a link in `docs/analyzer` to your book. On another note, would you mind if I were to put some of the information from there into the actual code?


================
Comment at: test/Analysis/cxx-uninitialized-object-inheritance.cpp:787
   // TODO: we'd expect the note: {{uninitialized field 'this->x'}}
   int x; // no-note
 };
----------------
NoQ wrote:
> Szelethus wrote:
> > The checker should be able to catch this one -- for some reason it is regarded as an unknown region. Odd, as the test case right after this one works perfectly.
> There's a variety of problems we have with empty base classes, might be one of those, and they are usually easy to fix because, well, yes it's a special case, but it's also an extremely simple case.
> 
> I encourage you to open up the Exploded Graph and study it carefully to see what and where goes wrong (not for this revision).
I'd love to learn about other parts of the analyzer (besides chasing pointers), but for now, this seems to have fixed itself ;)


https://reviews.llvm.org/D50892