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

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

Szelethus marked 9 inline comments as done.
Szelethus added a comment.

Thanks again for taking a look :)



================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:359-372
+  // Checking bases.
+  const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD);
+  if (!CXXRD)
+    return ContainsUninitField;
+
+  for (const CXXBaseSpecifier BaseSpec : CXXRD->bases()) {
+    const auto *Base = BaseSpec.getType()->getAsCXXRecordDecl();
----------------
NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Are there many warnings that will be caused by this but won't be caused by conducting the check for the constructor-within-constructor that's currently disabled? Not sure - is it even syntactically possible to not initialize the base class?
> > I'm not a 100% sure what you mean. Can you clarify?
> > >Not sure - is it even syntactically possible to not initialize the base class?
> > If I understand the question correctly, no, as far as I know.
> You have a check for `isCalledByConstructor()` in order to skip base class constructors but then you check them manually. That's a lot of code, but it seems that the same result could have been achieved by simply skipping the descent into the base class.
> 
> Same question for class-type fields, actually.
>Same question for class-type fields, actually.

My problem with that approach is that the same uninitialized field could've been emitted multiple times in different warnings. From an earlier conversation (assume that its run in pedantic mode):

>For this code snippet:
>
> ```
>struct A {
>   struct B {
>      int x, y;
>      
>      B() {}
>   } b;
>   int w;
>
>   A() {
>      b = B();
>   }
>};
>```
>the warning message after a call to `A::A()` would be "3 uninitialized fields[...]", and for `B::B()` inside `A`s constructor would be "2 uninitialized fields[...]", so it wouldn't be filtered out.

In my opinion, if `A` contains a field of type `B`, it should be the responsibility of `A`'s constructors to initialize those fields properly.
>You have a check for isCalledByConstructor() in order to skip base class constructors but then you check them manually. That's a lot of code, but it seems that the same result could have been achieved by simply skipping the descent into the base class.
I also believe that a constructor "should be held responsible" for the initialization of the inherited data members. However, in the case of base classes, uniqueing isn't an issue, as in this case:
```
struct B {
  int a;
  B() {}
};

struct D : public B {
  int b;
  D() {}
};
```
a call to `D::D()` would not check the inherited member (`a`), if I didn't implement it explicitly. That also means that if `isCalledByConstructor` was refactored to not filter out base classes, we would get two warning each with a single note, reporting `b` and `a` respectively. (I haven't actually checked it, but its a reasonable assumption)

Actually, I'm not 100% sure which approach is better: a warning for each base, or one warning for the object, bases included. I personally think one warning per object results in the best user experience.

I like both ideas, but I think if we'd come to the conclusion that a warning per base should be the way to go, it should definitely be implemented in a followup patch, as `isCalledByConstructor` is already in line for some major refactoring.

What do you think? :)


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:405-406
+  SVal DerefdV = StoreMgr.getBinding(S, V.castAs<Loc>());
+  while (Optional<Loc> L = DerefdV.getAs<Loc>())
+    DerefdV = StoreMgr.getBinding(S, *L);
+
----------------
NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Is this loop guaranteed to terminate? Is something like `void *v; v = &v;` possible?
> > > 
> > > I think this loop should unwrap the AST type on every iteration and dereference the pointer accordingly.
> > > 
> > > In general, i believe that every symbolic pointer dereference should be made with awareness of the type of the object we're trying to retrieve. Which is an optional argument for `State->getSVal(R, ...)`, but it seems that it should be made mandatory because in a lot of cases omitting it causes problems.
> > > Is this loop guaranteed to terminate? Is something like void *v; v = &v; possible?
> > I looked this up, and I am confident that it is not possible for a pointer to point to itself. Only a `void**` object may point to a `void*`. The loop terminates even if you do something evil like `v = reinterpret_cast<void*>(&v);` (I have tried this with `int`s).
> > 
> > >I think this loop should unwrap the AST type on every iteration and dereference the pointer accordingly.
> > >
> > >In general, i believe that every symbolic pointer dereference should be made with awareness of the type of the object we're trying to retrieve. Which is an optional argument for State->getSVal(R, ...), but it seems that it should be made mandatory because in a lot of cases omitting it causes problems.
> > 
> > I invested some serious effort into `getDynamicType`, but I think it'll require more time and some serious refactoring of this (`isPointerOrReferenceUninit`) function, so I'd post it separately.
> > Only a `void**` object may point to a `void*`.
> 
> That's definitely not true. The whole point of `void *` is that it can point to anything, including another `void *`. In fact, even `void *v = &v;` compiles in both C and C++.
> 
> I think it's better to have a stronger guarantee that this loop terminates.
Oh, right. Yea. Silly me. Well in any case, non-void pointers can't point to themselves, as I understand it. And since void pointers are skipped for now, I think (and have found after some serious testing) that loop will terminate just fine.

>I think it's better to have a stronger guarantee that this loop terminates.

Agreed. I see now however why did you find parts of this function so fishy.


https://reviews.llvm.org/D45532