[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

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

Szelethus updated this revision to Diff 147485.
Szelethus added a comment.

In this diff, I preferred placing `TODO`s in places where the logic of the checker would have changed too much. While I didn't have a strict rule for this in mind, I tried changing as little as possible in order to implement the fixes in smaller followup patches.

Now with that being said, I still did make numerous changes to address the inline comments, so here is a complete list of them:

- I placed the checker back in `alpha.cplusplus`
- While the main logic of the checker is unchanged, I did make some small additions: it turned out (thanks to @NoQ) that objects I wrongly referred to as fundamental were in fact of `BuiltinType` or `EnumeralType`. In the checker I defined these types as "primitive", as neither clang or the C++ standard defines a type called "primitive"
- A new node is added to the directed tree: objects of `MemberPointerType`. I updated the documentation, and added a function to `FindUninitializedFields` but left the implementation to be done in a followup patch
- String concatenations are all done with streams
- `FieldChainInfo` now uses `llvm::ImmutableList`
- `FieldChainInfo` is now completely immutable
- `FindUninitializedFields` no longer checks lazily, and has only a single `getUninitFields` public non-special member function.
- `FindUninitializedFields` now has a `ProgramStateRef` field instead of a bunch of managers (this added a dependency https://reviews.llvm.org/D46891)
- Added asserts and `llvm_unreachable` at numerous locations
- More comments for easier reading

For the test files:

- Changed all non-member function names to eliminate numbering
- Split `cxx-uninitialized-object.cpp` up, and placed pointer/reference tests in a different file
- Added tests for
  - `loc::ConcreteInt`
  - Member pointers
  - `BuiltinType` and `EnumeralType`
  - Constructorcall within a constructor, without the two constructors being in relation, to highlight a false negative
  - C++11 member initializers.

I'll be the first to admit that I'm not 100% sure I implemented the change to `llvm::ImmutableList` perfectly, so I guess maybe it's worth taking a deeper look at (but it certainly works!). Shall I tie its lifetime to the checker object's lifetime?
Of course, I rechecked the entire LLVM/Clang project and I'm happy to report that no crashes occured, and it emitted almost the same amount of warning (almost, because I used a newer version of clang).


https://reviews.llvm.org/D45532

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
  test/Analysis/Inputs/system-header-simulator-for-cxx-uninitialized-object.h
  test/Analysis/cxx-uninitialized-object-inheritance.cpp
  test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
  test/Analysis/cxx-uninitialized-object.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D45532.147485.patch
Type: text/x-patch
Size: 83442 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180518/ba6d7844/attachment-0001.bin>