[PATCH] D48436: [analyzer][UninitializedObjectChecker] Fixed a false negative by no longer filtering out certain constructor calls

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

Szelethus added a comment.

> I'll think about that a bit more; it might be worth it to track such deferred subregions in a state trait and drain it whenever we pop back to an explicit constructor.

There are so many things to consider, like the following case:

  struct DynTBase {};
  struct DynTDerived : DynTBase {
    // TODO: we'd expect the note: {{uninitialized field 'this->x'}}
    int x; // no-note
  };
  
  struct DynamicTypeTest {
    DynTBase *bptr;
    int i = 0;
  
    // TODO: we'd expect the warning: {{1 uninitialized field}}
    DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // no-warning
  };
  
  void f() {
    DynTDerived d;
    DynamicTypeTest t(&d);
  };

As of now, the checker misses this one.
We talked about two approaches so far, the one already in the checker and the one I proposed now. I think the current implementation solves this issue a lot more naturally -- `isNonUnionUninit` has to be changed so that the fields dynamic type is checked rather then its static type.
The proposed method would have a tough time dealing with this -- we don't want an error for `DynTDerived::DynTDerived()`, since it's a compiler generated ctor, but ideally we wouldn't like to have a warning for an object (`t`) and a separate warning for it's field (`t.bptr.x`), but I'm afraid this new approach would make this a necessity.

So, bottom line, there are a bunch of `TODO`s in the code to make sure this issue is not forgotten, I strongly believe that we need a separate discussion for this, if you'd agree :)



================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:674
+  const LocationContext *LC = Context.getLocationContext();
+  while ((LC = LC->getParent())) {
+
----------------
NoQ wrote:
> george.karpenkov wrote:
> > nit: could we have `while (LC)` followed by `LC = LC->getParent()` ? Do you intentionally skip the first location context?
> I guess the predicate we're checking is trivially true for the current location context.
Completely missed this inline, sorry!

As @NoQ said, since this checker only fires after a constructor call, the first location context will surely be that, and I'm only checking whether the current ctor was called by another.


https://reviews.llvm.org/D48436