[Analysis] Make ThreadSafety correctly handle base class destructors

[boomanaiden154]

After the landing of #169320, the clang CFG analyses are able to do slightly more analysis around destructors. This results in thread safety also seeing slightly more destructors. This exposed a bug in ThreadSafety, where we would call getDestructorDecl, which can return nullptr for base class destructors, but not do a null pointer check, resulting in a segmentation fault.

This patch fixes the issue by adding a null pointer check and adds a regression test so this gets caught before downstream integration testing in the future.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Aiden Grossman (boomanaiden154)

Changes

After the landing of #169320, the clang CFG analyses are able to do slightly more analysis around destructors. This results in thread safety also seeing slightly more destructors. This exposed a bug in ThreadSafety, where we would call getDestructorDecl, which can return nullptr for base class destructors, but not do a null pointer check, resulting in a segmentation fault.

This patch fixes the issue by adding a null pointer check and adds a regression test so this gets caught before downstream integration testing in the future.

---

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

2 Files Affected:

• (modified) clang/lib/Analysis/ThreadSafety.cpp (+1-1)
• (added) clang/test/SemaCXX/no-warn-thread-safety-analysis.cpp (+12)

diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 77750cf89d7a7..a25bd6007d5ed 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -2820,7 +2820,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
         case CFGElement::AutomaticObjectDtor: {
           CFGAutomaticObjDtor AD = BI.castAs<CFGAutomaticObjDtor>();
           const auto *DD = AD.getDestructorDecl(AC.getASTContext());
-          if (!DD->hasAttrs())
+          if (!DD || !DD->hasAttrs())
             break;
 
           LocksetBuilder.handleCall(
diff --git a/clang/test/SemaCXX/no-warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/no-warn-thread-safety-analysis.cpp
new file mode 100644
index 0000000000000..5b1964301fce7
--- /dev/null
+++ b/clang/test/SemaCXX/no-warn-thread-safety-analysis.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-pointer -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-pointer -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-pointer -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-pointer -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s
+// expected-no-diagnostics
+
+struct foo {
+  ~foo();
+};
+struct bar : foo {};
+struct baz : bar {};
+baz foobar(baz a) { return a; }

[usx95]

Thanks for the quick fix !

[melver]

Why was a new test file created for this and not just added to the existing suite: https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/warn-thread-safety-analysis.cpp ?

[boomanaiden154]

There weren't any existing cases in there that used expected-no-diagnostics, and I've seen similar cases split similarly between positive and negative checks like this.