[analyzer] pr37688: Fix a crash on trying to evaluate a deleted destructor of a union.

[NoQ]

Summary

This is a slow quick fix for https://bugs.llvm.org/show_bug.cgi?id=37688 - add a defensive check against an invalid destructor in the CFG.

Unions with fields with destructors have their own destructor implicitly deleted. Due to a bug in the CFG we're still trying to evaluate them at the end of the object's lifetime and crash because we are unable to find the declaration.

Add a FIXME test for the CFG that demonstrates the bug and a normal test that demonstrates that we've fixed the crash and are even trying to continue the analysis beyond that point.

I've no idea why did this require a noexcept specification. I also don't know why does it require that many levels of structure nestedness; it might be that Clang doesn't emit an error in this case but in fact the code is invalid - an error about attempting to rely upon a deleted destructor is emitted in many simpler cases.

[george.karpenkov]

The code is fine, but I obviously would prefer a proper fix in a CFG

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

 
 void ExprEngine::VisitCXXDestructor(QualType ObjectType,
                                     const MemRegion *Dest,
                                     const Stmt *S,
                                     bool IsBaseDtor,
                                     ExplodedNode *Pred,
                                     ExplodedNodeSet &Dst,
                                     const EvalCallOptions &CallOpts) {
+  assert(S && "A destructor without a trigger!");
   const LocationContext *LCtx = Pred->getLocationContext();
   ProgramStateRef State = Pred->getState();
 
   const CXXRecordDecl *RecordDecl = ObjectType->getAsCXXRecordDecl();
   assert(RecordDecl && "Only CXXRecordDecls should have destructors");
   const CXXDestructorDecl *DtorDecl = RecordDecl->getDestructor();
 
+  // FIXME: There should always be a Decl, otherwise the destructor call
+  // shouldn't have been added to the CFG in the first place.
+  if (!DtorDecl) {
+    // Skip the invalid destructor. We cannot simply return because
+    // it would interrupt the analysis instead.
+    static SimpleProgramPointTag T("ExprEngine", "SkipInvalidDestructor");
+    // FIXME: PostImplicitCall with a null decl may crash elsewhere anyway.
+    PostImplicitCall PP(/*Decl=*/nullptr, S->getEndLoc(), LCtx, &T);
+    NodeBuilder Bldr(Pred, Dst, *currBldrCtx);
+    Bldr.generateNode(PP, Pred->getState(), Pred);
+    return;
+  }
+
   CallEventManager &CEMgr = getStateManager().getCallEventManager();
   CallEventRef<CXXDestructorCall> Call =
     CEMgr.getCXXDestructorCall(DtorDecl, S, Dest, IsBaseDtor, State, LCtx);
 
   PrettyStackTraceLoc CrashInfo(getContext().getSourceManager(),
                                 Call->getSourceRange().getBegin(),
                                 "Error evaluating destructor");
 
   ExplodedNodeSet DstPreCall;
   getCheckerManager().runCheckersForPreCall(DstPreCall, Pred,
                                             *Call, *this);
 
   ExplodedNodeSet DstInvalidated;
   StmtNodeBuilder Bldr(DstPreCall, DstInvalidated, *currBldrCtx);
   for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end();
        I != E; ++I)
     defaultEvalCall(Bldr, *I, *Call, CallOpts);
 
-  ExplodedNodeSet DstPostCall;
   getCheckerManager().runCheckersForPostCall(Dst, DstInvalidated,
                                              *Call, *this);
 }
 
 void ExprEngine::VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
                                           ExplodedNode *Pred,
                                           ExplodedNodeSet &Dst) {
   ProgramStateRef State = Pred->getState();

test/Analysis/cfg.cpp

// CHECK-NOT: ~LifetimeExtend()
     AggregateRef a{LifetimeExtend(5), LifetimeExtend(5)};
     5;
   }
   // FIXME: Add tests for lifetime extension via subobject
   // references (LifetimeExtend().some_member).
 }
 
 
+// FIXME: The destructor for 'a' shouldn't be there because it's deleted
+// in the union.
+// CHECK-LABEL: void foo()
+// CHECK:  [B2 (ENTRY)]
+// CHECK-NEXT:    Succs (1): B1
+// CHECK:  [B1]
+// WARNINGS-NEXT:    1:  (CXXConstructExpr, struct pr37688_deleted_union_destructor::A)
+// ANALYZER-NEXT:    1:  (CXXConstructExpr, [B1.2], struct pr37688_deleted_union_destructor::A)
+// CHECK-NEXT:    2: pr37688_deleted_union_destructor::A a;
+// CHECK-NEXT:    3: [B1.2].~A() (Implicit destructor)
+// CHECK-NEXT:    Preds (1): B2
+// CHECK-NEXT:    Succs (1): B0
+// CHECK:  [B0 (EXIT)]
+// CHECK-NEXT:    Preds (1): B1
+
+namespace pr37688_deleted_union_destructor {
+struct S { ~S(); };
+struct A {
+  ~A() noexcept {}
+  union {
+    struct {
+      S s;
+    } ss;
+  };
+};
+void foo() {
+  A a;
+}
+} // end namespace pr37688_deleted_union_destructor
+
+
 // CHECK-LABEL: template<> int *PR18472<int>()
 // CHECK: [B2 (ENTRY)]
 // CHECK-NEXT:   Succs (1): B1
 // CHECK: [B1]
 // CHECK-NEXT:   1: 0
 // CHECK-NEXT:   2: [B1.1] (ImplicitCastExpr, NullToPointer, PR18472_t)
 // CHECK-NEXT:   3: (PR18472_t)[B1.2] (CStyleCastExpr, NoOp, PR18472_t)
 // CHECK-NEXT:   4: CFGNewAllocator(int *)

test/Analysis/unions.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,debug.ExprInspection %s -analyzer-config eagerly-assume=false -verify
 
 extern void clang_analyzer_eval(bool);
+extern void clang_analyzer_warnIfReached();
 extern "C" char *strdup(const char *s);
 
 namespace PR14054_reduced {
   struct Definition;
   struct ParseNode {
     union {
       Definition *lexdef;
       ParseNode *data;
void test() {
   U u = get();
   int y = 0;
   if (u.x)
     y = 1;
   if (u.x)
     y = 1 / y; // no-warning
 }
 } // end namespace assume_union_contents
+
+namespace pr37688_deleted_union_destructor {
+struct S { ~S(); };
+struct A {
+  ~A() noexcept {}
+  union {
+    struct {
+      S s;
+    } ss;
+  };
+};
+void foo() {
+  A a;
+} // no-crash
+void bar() {
+  foo();
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+}
+} // end namespace pr37688_deleted_union_destructor