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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added inline comments.
This revision is now accepted and ready to land.


================
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();
----------------
Szelethus wrote:
> 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?
I guess a link on the website would be good. We should also convert a lot of our comments to doxygen comments, because some people do read doxygen (including myself).

I don't advice copy-paste-ing texts from the workbook to the code because i picked a weird licensing scheme (no idea why i picked a -BY license) and wasn't straight enough about authorship (so i don't feel i can change it anymore). But actually documenting things that are currently not documented is something i'd greatly appreciate :)


https://reviews.llvm.org/D50892