[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.

In https://reviews.llvm.org/D48436#1191458, @NoQ wrote:

> Ok, let's commit this and see how to fix it later.


Thanks! ^-^

> I still think it's more important to come up with clear rules of who is responsible for initializing fields than making sure our warnings are properly grouped together.

I'll admit that the main reason why I only added a TODO about this issue and delayed it for later is that I'm a little unsure about some details myself. I spent serious effort on some ideas, which I later realized didn't work out too well, so I'm looking for a solid solution on some issues (like base class handling) that is both efficient to a degree and makes sense from a user standpoint before writing a doc file of some sort.

Currently, I'm working on refactoring the checker (which is why I didn't update some of my patches, as those changes would become obsolete), which will greatly increase readability and reliability (let's face it, the code about pointer chasing is a spaghetti).

>> [...]ideally we wouldn't like to have a warning for an object (`t`) and a separate warning for it's field (`t.bptr.x`)[...]
> 
> I don't quite understand this. How would the first warning look like? What would it warn about?

I'm afraid I explained my thoughts very poorly. I have a much better grasp on this issue now, and I have a very good solution in testing. I'll upload a new diff during the week that will clarify this.
Here's what I meant with the correct code:

  struct DynTBase {
    // This is the line I meant but forgot to add.
    int x; // expected-note{{uninitialized field 'this->bptr->x'}}
  };
  struct DynTDerived : DynTBase {
    // TODO: we'd expect the note: {{uninitialized field 'this->bptr->y'}}
    int y; // no-note
  };
  
  struct DynamicTypeTest {
    DynTBase *bptr;
    
    int i = 0;
  
    DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // expected-warning{{1 uninitialized field}}
  };
  
  void f() {
    DynTDerived d;
    DynamicTypeTest t(&d);
  };

In this one, note that the constructed object `t` has two uninitialized fields, `t.bptr->x` and `t.bptr->y`. Currently, my checker misses the second one. The first one gets picked up rather easily, but then the problem arises about how do we deal with `t.bptr->y`. I proposed two ideas:

1. In `isNonUnionUninit`, obtain the dynamic type of the object we're analyzing. This is the easier solution, as the current implementation explicitly checks each base. This would result in one warning with a note for each field.
2. Change `willObjectBeAnalyzedLater` so that bases are analyzed on their own, and eliminate checking bases explicitly (https://reviews.llvm.org/D45532#inline-415396). The checker won't run when `d` is constructed, because it has a default ctor, but will run after `t`. How do we catch `t.bptr->y?` If we don't analyze bases explicitly, we can't just obtain the dynamic type in `isNonUnionUninit`, because then we'd miss `t.bptr->x`. If we decide that for fields we will explicitly check bases, that would be inconsistent, because in `t`s case we would analyze base classes on their own (if it had any), but we wouldn't for its fields. As you said,

> With that i guess you'll still have to deep-scan fields and bases, which prevents us from simplifying our code.

I really just wanted to emphasize the point that I need more time to figure this out.



================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:669-671
+  Optional<nonloc::LazyCompoundVal> CurrentObject = getObjectVal(Ctor, Context);
+  if (!CurrentObject)
+    return false;
----------------
NoQ wrote:
> All uses of `getObjectVal` so far are followed by retrieving the parent region from the `LazyCompoundVal`. Why do you need to obtain the `LazyCompoundVal` in the first place, i.e. do the second `getSVal` "for '*this'" in `getObjectVal`? Why not just operate over the this-region on the current Store? I think there isn't even a guarantee that these two regions are the same. Like, in this case they probably will be the same, but we shouldn't rely on that.
Fair point, that part was written when I knew very little about how these things worked. I'll add a TODO to get that fixed before commiting.


https://reviews.llvm.org/D48436