https://github.com/melver created https://github.com/llvm/llvm-project/pull/179041
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 <[email protected]> >From 8a068ddf07f76f67b5b16c4535cb0460c97cdf76 Mon Sep 17 00:00:00 2001 From: Marco Elver <[email protected]> Date: Sat, 31 Jan 2026 03:39:48 +0100 Subject: [PATCH] Thread Safety Analysis: Fix value-based comparison of literals in TIL 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 <[email protected]> --- .../clang/Analysis/Analyses/ThreadSafetyTIL.h | 11 +++++-- clang/lib/Analysis/ThreadSafetyTIL.cpp | 29 +++++++++++++++++++ .../SemaCXX/warn-thread-safety-analysis.cpp | 26 +++++++++++++++++ 3 files changed, 64 insertions(+), 2 deletions(-) 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; _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
