[analyzer] Do not destruct fields of unions

[vtjnash]

The C++ standard prohibits this implicit destructor call, leading to incorrect reports from clang-analyzer. This causes projects that use std::option (including llvm) to fail the cplusplus.NewDelete test incorrectly when run through the analyzer.

Fixes #119415

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Jameson Nash (vtjnash)

Changes

The C++ standard prohibits this implicit destructor call, leading to incorrect reports from clang-analyzer. This causes projects that use std::option (including llvm) to fail the cplusplus.NewDelete test incorrectly when run through the analyzer.

Fixes #119415

---

Full diff: https://github.com/llvm/llvm-project/pull/122330.diff

3 Files Affected:

• (modified) clang/lib/Analysis/CFG.cpp (+2)
• (modified) clang/test/Analysis/NewDelete-checker-test.cpp (+28)
• (modified) clang/test/Analysis/dtor-array.cpp (+24)

diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 304bbb2b422c61..3e144395cffc6f 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -2041,6 +2041,8 @@ void CFGBuilder::addImplicitDtorsForDestructor(const CXXDestructorDecl *DD) {
   }
 
   // First destroy member objects.
+  if (RD->isUnion())
+    return;
   for (auto *FI : RD->fields()) {
     // Check for constant size array. Set type to array element type.
     QualType QT = FI->getType();
diff --git a/clang/test/Analysis/NewDelete-checker-test.cpp b/clang/test/Analysis/NewDelete-checker-test.cpp
index 21b4cf817b5df6..806edd47840fc1 100644
--- a/clang/test/Analysis/NewDelete-checker-test.cpp
+++ b/clang/test/Analysis/NewDelete-checker-test.cpp
@@ -441,3 +441,31 @@ void testLeakBecauseNTTPIsNotDeallocation() {
   void* p = ::operator new(10);
   deallocate_via_nttp<not_free>(p);
 }  // leak-warning{{Potential leak of memory pointed to by 'p'}}
+
+namespace optional_union {
+  template <typename T>
+  class unique_ptr {
+    T *q;
+  public:
+    unique_ptr() : q(new T) {}
+    ~unique_ptr() {
+      delete q;
+    }
+  };
+
+  union custom_union_t {
+    unique_ptr<int> present;
+    char notpresent;
+    custom_union_t() : present(unique_ptr<int>()) {}
+    ~custom_union_t() {};
+  };
+
+  void testUnionCorrect() {
+    custom_union_t a;
+    a.present.~unique_ptr<int>();
+  }
+
+  void testUnionLeak() {
+    custom_union_t a;
+  } // leak-warning{{Potential leak of memory pointed to by 'a.present.q'}}
+}
diff --git a/clang/test/Analysis/dtor-array.cpp b/clang/test/Analysis/dtor-array.cpp
index 84a34af9225169..1bbe55c09ee7e2 100644
--- a/clang/test/Analysis/dtor-array.cpp
+++ b/clang/test/Analysis/dtor-array.cpp
@@ -377,3 +377,27 @@ void directUnknownSymbol() {
 }
 
 }
+
+void testUnionDtor() {
+  static int unionDtorCalled;
+  InlineDtor::cnt = 0;
+  InlineDtor::dtorCalled = 0;
+  unionDtorCalled = 0;
+  {
+      union UnionDtor {
+          InlineDtor kind1;
+          char kind2;
+          ~UnionDtor() { unionDtorCalled++; }
+      };
+      UnionDtor u1{.kind1{}};
+      UnionDtor u2{.kind2{}};
+      auto u3 = new UnionDtor{.kind1{}};
+      auto u4 = new UnionDtor{.kind2{}};
+      delete u3;
+      delete u4;
+  }
+
+  clang_analyzer_eval(unionDtorCalled == 4); // expected-warning {{TRUE}}
+  clang_analyzer_eval(InlineDtor::dtorCalled != 4); // expected-warning {{TRUE}}
+  clang_analyzer_eval(InlineDtor::dtorCalled == 0); // expected-warning {{TRUE}}
+}

[vtjnash]

This roughly corresponds to 921bd20 in Sema, I think (@zygoloid). Maybe @tkremenek or @yronglin has done something in this part of the code recently too as reviewers and to merge this?

[gbaraldi]

Bump.
For reference, this is holding julia to an older LLVM version than needed ;)

[AaronBallman]

Thank you for the fix! The changes look correct to me, but I added the CFG and Analysis maintainers to give a final sign-off. Can you also add something to clang/docs/ReleaseNotes.rst so users know about the fix?

[steakhal]

LGTM, thanks.

[Xazax-hun]

Overall looks good to me, some nits inline

[Xazax-hun]

Could we also have a test where the custom union does not have a dtor declared for completeness? Or is that already covered somewhere?

[vtjnash]

If I don't declare a dtor, it seems to be a C++ syntax error, so I cannot test that, but I will try to fix the other nits

[Xazax-hun]

I think this is a wonderful test but I would not expect to find it in dtor-array as this has nothing to do with arrays. Would it be possible to move it to a new file? Also to add a same test with a union that does not declare a dtor?

[Xazax-hun]

LG, thanks!