[CFG] Fix crash in thread sanitizer.

[• nandor]

Summary

clang was crashing with -Wthread-safety due to the incorrect construction of the CFG when dealing with C++ lifetime-extending constructs.

struct Holder {
  virtual ~Holder() throw() {}
  int i = 0;
};

int main() {
  const auto &value = Holder().i;
}

This patch fixes the crash by correctly identifying the destructor.

[• nandor]

Fixed typo

[dcoughlin]

This is https://llvm.org/bugs/show_bug.cgi?id=28666

[• nandor]

ping?

[NoQ]

Thanks for working on this!

While i probably won't be of much help (no expert in temporaries yet), i notice that this code piece (with getReferenceInitTemporaryType()) seems to be duplicated at least three times in this file, with slight variations and FIXMEs. So i guess this needs some care, not necessarily in this patch.

lib/Analysis/CFG.cpp

3910 ↗

(On Diff #64368)

Simple ty->isReferenceType() should work as well (the overloaded operator ->() in QualType).

3912–3914 ↗

(On Diff #64368)

Is this sequence of checks certainly needed? All tests seem to pass without it. Or maybe it's also needed in addAutomaticObjectDtors()? Perhaps just nest these checks inside getReferenceInitTemporaryType()?

[• nandor]

removed redundant checks

[• nandor]

I'm not an expert in temporaries either, I think for a proper fix a lot more work is required.

It seems that the problem is with locating destructors in destructor calls. I get the impression that currently a destructor node pointing to the declaration is added and the type that is actually destroyed is inferred from the expression every time it is needed. I think destructors should be able to point to specific temporary subexpressions that they destroy in order to deal with multiple temporaries from the same expression.

This fix only ensures that all scenarios which lead to a placement of such a destructor node are handled so no SIGSEV occurs due to a destructor being added, but not handled at all later on.

lib/Analysis/CFG.cpp

3912–3914 ↗

(On Diff #64368)

It seems like getReferenceInitTemporaryType includes these two statements, we can remove them from here.

[• nandor]

ping

[dcoughlin]

LGTM. I will commit. Thanks Nandor!

cfe/trunk/lib/Analysis/CFG.cpp

switch (getKind()) {
     case CFGElement::Statement:
     case CFGElement::Initializer:
     case CFGElement::NewAllocator:
       llvm_unreachable("getDestructorDecl should only be used with "
                        "ImplicitDtors");
     case CFGElement::AutomaticObjectDtor: {
       const VarDecl *var = castAs<CFGAutomaticObjDtor>().getVarDecl();
       QualType ty = var->getType();
-      ty = ty.getNonReferenceType();
+
+      // FIXME: See CFGBuilder::addLocalScopeForVarDecl.
+      //
+      // Lifetime-extending constructs are handled here. This works for a single
+      // temporary in an initializer expression.
+      if (ty->isReferenceType()) {
+        if (const Expr *Init = var->getInit()) {
+          ty = getReferenceInitTemporaryType(astContext, Init);
+        }
+      }
+
       while (const ArrayType *arrayType = astContext.getAsArrayType(ty)) {
         ty = arrayType->getElementType();
       }
       const RecordType *recordType = ty->getAs<RecordType>();
       const CXXRecordDecl *classDecl =
       cast<CXXRecordDecl>(recordType->getDecl());
       return classDecl->getDestructor();      
     }

cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp

void test3() {
   mu1.Lock();  // expected-warning {{mutex 'mu1' must be acquired before 'mu2'}}
   mu1.Unlock();
   mu2.Unlock();
 }
 
 }  // end namespace  GlobalAcquiredBeforeAfterTest
 
 
+namespace LifetimeExtensionText {
+
+struct Holder {
+  virtual ~Holder() throw() {}
+  int i = 0;
+};
+
+void test() {
+  // Should not crash.
+  const auto &value = Holder().i;
+}
+
+} // end namespace LifetimeExtensionTest
+
+
 namespace LockableUnions {
 
 union LOCKABLE MutexUnion {
   int a;
   char* b;
 
   void Lock()   EXCLUSIVE_LOCK_FUNCTION();
   void Unlock() UNLOCK_FUNCTION();