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


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:674
+  const LocationContext *LC = Context.getLocationContext();
+  while ((LC = LC->getParent())) {
+
----------------
Szelethus wrote:
> 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.
I'm actually not sure how you imagined that. The loop condition is `LC`, but if I assign it to its parent right after that, I won't be sure that it's non-null. The reason why I placed it there is that early exits could restart the loop at "random" places.


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:681-689
+    Optional<nonloc::LazyCompoundVal> OtherObject =
+        getObjectVal(OtherCtor, Context);
+    if (!OtherObject)
+      continue;
+
+    // If the CurrentObject is a field of OtherObject, it will be analyzed
+    // during the analysis of OtherObject.
----------------
NoQ wrote:
> It seems safer to look at `CXXConstructExpr::getConstructionKind()`.
> 
> Taking a `LazyCompoundVal` and converting it back to a region is definitely a bad idea because the region within `LazyCompoundVal` is completely immaterial and carries no meaning for the value represented by this `SVal`; it's not necessarily the region you're looking for.
Looking at the singleton test case, I think I need to get that region somehow, and I'm not too sure what you meant under using `CXXConstructExpr::getConstructionKind()`. One thing I could do, is similar to how `getObjectVal` works:
```
  Loc Tmp = Context.getSValBuilder().getCXXThis(OtherCtor->getParent(),
                                                Context.getStackFrame());

  auto OtherThis = Context.getState()->getSVal(Tmp).castAs<loc::MemRegionVal>();

  OtherThis.getRegion(); // Hooray!
```

I have tested it, and it works wonders. Is this a "safe-to-use" region?


https://reviews.llvm.org/D48436