[clang] Thread Safety Analysis: Fix value-based comparison of literals in TIL (PR #179041)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Marco Elver (melver)

<details>
<summary>Changes</summary>

Previously, til::Literal::compare() always returned true, effectively treating all literals (such as array indices 0 and 1) as identical. This led to false positives when different elements of an array were accessed or locked.

Fix it by adding compareLiterals() to perform value-based comparison for common literal types. This ensures that distinct literals are correctly distinguished.

Reported-by: Bart Van Assche <bvanassche@<!-- -->acm.org>

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


3 Files Affected:

- (modified) clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h (+9-2) 
- (modified) clang/lib/Analysis/ThreadSafetyTIL.cpp (+29) 
- (modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+26) 


``````````diff
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h
index 14c5b679428a3..718fc342bd9cc 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h
@@ -556,13 +556,20 @@ class Literal : public SExpr {
 
   template <class C>
   typename C::CType compare(const Literal* E, C& Cmp) const {
-    // TODO: defer actual comparison to LiteralT
-    return Cmp.trueResult();
+    // If the expressions are different AST nodes, try to compare their values.
+    if (Cexpr && E->Cexpr && Cexpr != E->Cexpr) {
+      if (compareLiterals(Cexpr, E->Cexpr))
+        return Cmp.trueResult();
+    }
+    return Cmp.comparePointers(Cexpr, E->Cexpr);
   }
 
 private:
   const ValueType ValType;
   const Expr *Cexpr = nullptr;
+
+  // Compare two literal expressions for value equality.
+  static bool compareLiterals(const Expr *E1, const Expr *E2);
 };
 
 // Derived class for literal values, which stores the actual value.
diff --git a/clang/lib/Analysis/ThreadSafetyTIL.cpp b/clang/lib/Analysis/ThreadSafetyTIL.cpp
index dcee891222a33..b032e389ddf1a 100644
--- a/clang/lib/Analysis/ThreadSafetyTIL.cpp
+++ b/clang/lib/Analysis/ThreadSafetyTIL.cpp
@@ -7,6 +7,8 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang/Analysis/Analyses/ThreadSafetyTIL.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/Basic/LLVM.h"
 #include <cassert>
 #include <cstddef>
@@ -54,6 +56,33 @@ SExpr* Future::force() {
   return Result;
 }
 
+bool Literal::compareLiterals(const Expr *E1, const Expr *E2) {
+  if (E1->getStmtClass() != E2->getStmtClass())
+    return false;
+  switch (E1->getStmtClass()) {
+  case Stmt::IntegerLiteralClass:
+    return cast<IntegerLiteral>(E1)->getValue() ==
+           cast<IntegerLiteral>(E2)->getValue();
+  case Stmt::CharacterLiteralClass:
+    return cast<CharacterLiteral>(E1)->getValue() ==
+           cast<CharacterLiteral>(E2)->getValue();
+  case Stmt::CXXBoolLiteralExprClass:
+    return cast<CXXBoolLiteralExpr>(E1)->getValue() ==
+           cast<CXXBoolLiteralExpr>(E2)->getValue();
+  case Stmt::StringLiteralClass:
+    return cast<StringLiteral>(E1)->getString() ==
+           cast<StringLiteral>(E2)->getString();
+  case Stmt::FloatingLiteralClass:
+    return cast<FloatingLiteral>(E1)->getValue().bitwiseIsEqual(
+        cast<FloatingLiteral>(E2)->getValue());
+  case Stmt::CXXNullPtrLiteralExprClass:
+  case Stmt::GNUNullExprClass:
+    return true;
+  default:
+    return false;
+  }
+}
+
 unsigned BasicBlock::addPredecessor(BasicBlock *Pred) {
   unsigned Idx = Predecessors.size();
   Predecessors.reserveCheck(1, Arena);
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index 466135a1d9cef..0caee7ccbe5c4 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -7536,6 +7536,32 @@ void testNestedAcquire(Container *c) EXCLUSIVE_LOCK_FUNCTION(&c->foo.mu) {
   buf->mu.Lock();
 }
 
+// Test that array subscripts are not ignored.
+void testArrayOfContainers1() {
+  Container array[10];
+
+  Foo *ptr1 = &array[0].foo;
+  Foo *ptr2 = &array[1].foo;
+  ptr1->mu.Lock();
+  ptr2->mu.Lock();
+  array[0].foo.data = 0;
+  array[1].foo.data = 1;
+  ptr2->mu.Unlock();
+  ptr1->mu.Unlock();
+}
+
+// Test that we don't confuse different indices or constants.
+void testArrayOfContainers2() {
+  Container array[2];
+
+  Foo *ptr = &array[0].foo;
+  ptr->mu.Lock();
+  array[0].foo.data = 0;
+  array[1].foo.data = 1;  // expected-warning{{writing variable 'data' requires holding mutex 'array[1].foo.mu'}} \
+                          // expected-note{{found near match 'array[0].foo.mu'}}
+  ptr->mu.Unlock();
+}
+
 struct ContainerOfPtr {
   Foo *foo_ptr;
   ContainerOfPtr *next;

``````````

</details>


https://github.com/llvm/llvm-project/pull/179041