[analyzer][UninitializedObjectChecker] No longer using nonloc::LazyCompoundVal

[Szelethus]

Summary

As rightly pointed out by @NoQ, nonloc::LazyCompoundVals were only used to acquire a constructed object's region, which isn't what LazyCompoundVal was made for.

[NoQ]

Yup, looks correct to me!

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

448–449 ↗

(On Diff #162669)

This totally needs assert(CtorDecl == Context.getStackFrame()->getDecl()). Otherwise we're in big trouble because we'll be looking into a this-region that doesn't exist on this stack frame.

On second thought, though, i guess we should put this assertion into the constructor of CXXThisRegion. I'll do this.

Also there's an overload of getCXXThis that accepts the method itself, no need to get parent.

[Szelethus]

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

448–449 ↗

(On Diff #162669)

Ummmm that wouldn't be very nice, because...

456–483 ↗

(On Diff #162669)

...willBeAnalyzerLater() relies on this, and it uses all sorts of constructor decls to check whether Context.getLocationContext()->getDecl() would be a subregion of another object. Are you sure that this is incorrect?

[NoQ]

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

448–449 ↗

(On Diff #162669)

Yeah, i guess i'll have to think a bit deeper about this. I really want to prevent invalid CXXThisRegions from appearing, but it might be not that simple.

456–483 ↗

(On Diff #162669)

I mean not the this-region of the object, but the CXXThisRegion itself, in which this-region is stored. It is definitely not aliased across stack frames.

[Szelethus]

Would you be comfortable me commiting this without that assert, or should I come up with a more risk-free solution?

[Szelethus]

Fixed a crash where the returned region's type wasn't CXXRecordDecl. I'm not even sure how this is possible -- and unfortunately I've been unable to create a minimal (not) working example for this, and I wasn't even able to recreate the error locally. However, on the server where I could repeatedly cause a crash with the analysis of lib/AST/Expr.cpp, this fix resolved the issue.

Note that this assert failure didn't happen inside willBeAnalyzedLater, where getConstructedRegion is used with a different CXXConstructorDecl then the one on top of the stack frame.

[NoQ]

In D51300#1220537, @Szelethus wrote:

Would you be comfortable me commiting this without that assert

Yup sure.

In D51300#1223042, @Szelethus wrote:

I'm not even sure how this is possible -- and unfortunately I've been unable to create a minimal (not) working example for this, and I wasn't even able to recreate the error locally.

Sounds like a test case would be great to have. Consider extracting a preprocessed file and running it under creduce, that's a great generic method for obtaining small reproducers for crashes and regressions (but not for false positives). As far as i understand, it's a crash on llvm codebase, which should be easy to re-analyze locally, even just one file, because llvm is built with cmake and dumps compilation databases, so just use clang-check on a single file, or simply append --analyze to the compilation database run-line.

This specific problem sounds elusive because it's a problem with pointer casts, and pointer casts are currently a mess. I cannot state for sure that typed this-region type must always be a record, but it's definitely a bad smell when it is't. So i recommend a quick investigation of whether the region in question is (1) well-formed and (2) correctly reflects the semantics of the program.

[Szelethus]

Sounds like a test case would be great to have.

Aye. I'd be good to land this with that testcase. There are a variety of other projects I'm working on (related to the checker), and none of them depend on this patch, so I'll commit this once I figure out how to use creduce ><

[NoQ]

Very easy:

1. Put preprocessed file that crashes into repro.cpp (.c, .m, whatever).
2. Write repro.sh as follows:

#!/bin/bash
blah/blah/path/to/clang -cc1 -analyze -blah-blah-arguments-you-need-to-cause-a-crash repro.cpp 2>&1 | grep "Any assertion message or whatever"

3. Do:

creduce repro.sh repro.cpp

4. Wait until it finishes.
5. repro.cpp now contains the reduced test case.

Tip for speed:

• The repro.sh script should be as fast as possible. Use a release+asserts build of clang.
• Also -analyze-function top_function_that_we_analyze_when_we_crash is a great flag to add to the run-line if possible, because it'll make the test much faster; see -analyzer-display-progress to figure out how to specify the function; if it's a lambda or a block you're out of luck because you'll need to specify line number which would change during reduce; you may also fail because top-level analyses are interacting a bit, so it may fail to crash if you restrict to a function.

[Szelethus]

There are some nasty environment issues I'm dealing with, so the main problem is the lack of a well functioning creduce, but I'll get it done eventually.

[Szelethus]

Finally managed to run creduce. I added the test that caused a crash on our server, but as I said, I couldn't replicate it elsewhere.

[Szelethus]

Since a good amount of time passed since this patch was made, I'll re-analyze the LLVM project with this patch rebased on trunk just to be sure that nothing breaks.

cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

public:
 
   virtual bool isBase() const override { return true; }
 };
 
 } // end of anonymous namespace
 
 // Utility function declarations.
 
-/// Returns the object that was constructed by CtorDecl, or None if that isn't
-/// possible.
-// TODO: Refactor this function so that it returns the constructed object's
-// region.
-static Optional<nonloc::LazyCompoundVal>
-getObjectVal(const CXXConstructorDecl *CtorDecl, CheckerContext &Context);
+/// Returns the region that was constructed by CtorDecl, or nullptr if that
+/// isn't possible.
+static const TypedValueRegion *
+getConstructedRegion(const CXXConstructorDecl *CtorDecl,
+                     CheckerContext &Context);
 
 /// Checks whether the object constructed by \p Ctor will be analyzed later
 /// (e.g. if the object is a field of another object, in which case we'd check
 /// it multiple times).
 static bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor,
                                       CheckerContext &Context);
 
 /// Checks whether RD contains a field with a name or type name that matches
void UninitializedObjectChecker::checkEndFunction(
 
   if (CtorDecl->getParent()->isUnion())
     return;
 
   // This avoids essentially the same error being reported multiple times.
   if (willObjectBeAnalyzedLater(CtorDecl, Context))
     return;
 
-  Optional<nonloc::LazyCompoundVal> Object = getObjectVal(CtorDecl, Context);
-  if (!Object)
+  const TypedValueRegion *R = getConstructedRegion(CtorDecl, Context);
+  if (!R)
     return;
 
-  FindUninitializedFields F(Context.getState(), Object->getRegion(), Opts);
+  FindUninitializedFields F(Context.getState(), R, Opts);
 
   const UninitFieldMap &UninitFields = F.getUninitFields();
 
   if (UninitFields.empty())
     return;
 
   // There are uninitialized fields in the record.
 
static void printTail(llvm::raw_ostream &Out,
   L.getHead().printNode(Out);
   L.getHead().printSeparator(Out);
 }
 
 //===----------------------------------------------------------------------===//
 //                           Utility functions.
 //===----------------------------------------------------------------------===//
 
-static Optional<nonloc::LazyCompoundVal>
-getObjectVal(const CXXConstructorDecl *CtorDecl, CheckerContext &Context) {
+static const TypedValueRegion *
+getConstructedRegion(const CXXConstructorDecl *CtorDecl,
+                     CheckerContext &Context) {
 
-  Loc ThisLoc = Context.getSValBuilder().getCXXThis(CtorDecl->getParent(),
+  Loc ThisLoc = Context.getSValBuilder().getCXXThis(CtorDecl,
                                                     Context.getStackFrame());
-  // Getting the value for 'this'.
-  SVal This = Context.getState()->getSVal(ThisLoc);
 
-  // Getting the value for '*this'.
-  SVal Object = Context.getState()->getSVal(This.castAs<Loc>());
+  SVal ObjectV = Context.getState()->getSVal(ThisLoc);
+
+  auto *R = ObjectV.getAsRegion()->getAs<TypedValueRegion>();
+  if (R && !R->getValueType()->getAsCXXRecordDecl())
+    return nullptr;
 
-  return Object.getAs<nonloc::LazyCompoundVal>();
+  return R;
 }
 
 static bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor,
                                       CheckerContext &Context) {
 
-  Optional<nonloc::LazyCompoundVal> CurrentObject = getObjectVal(Ctor, Context);
-  if (!CurrentObject)
+  const TypedValueRegion *CurrRegion = getConstructedRegion(Ctor, Context);
+  if (!CurrRegion)
     return false;
 
   const LocationContext *LC = Context.getLocationContext();
   while ((LC = LC->getParent())) {
 
     // If \p Ctor was called by another constructor.
     const auto *OtherCtor = dyn_cast<CXXConstructorDecl>(LC->getDecl());
     if (!OtherCtor)
       continue;
 
-    Optional<nonloc::LazyCompoundVal> OtherObject =
-        getObjectVal(OtherCtor, Context);
-    if (!OtherObject)
+    const TypedValueRegion *OtherRegion =
+        getConstructedRegion(OtherCtor, Context);
+    if (!OtherRegion)
       continue;
 
-    // If the CurrentObject is a subregion of OtherObject, it will be analyzed
-    // during the analysis of OtherObject.
-    if (CurrentObject->getRegion()->isSubRegionOf(OtherObject->getRegion()))
+    // If the CurrRegion is a subregion of OtherRegion, it will be analyzed
+    // during the analysis of OtherRegion.
+    if (CurrRegion->isSubRegionOf(OtherRegion))
       return true;
   }
 
   return false;
 }
 
 static bool shouldIgnoreRecord(const RecordDecl *RD, StringRef Pattern) {
   llvm::Regex R(Pattern);