Thread Safety Analysis: Add literal-based alias test#179041
Merged
Conversation
Member
|
@llvm/pr-subscribers-clang-analysis Author: Marco Elver (melver) ChangesPreviously, 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:
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;
|
Member
|
@llvm/pr-subscribers-clang Author: Marco Elver (melver) ChangesPreviously, 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:
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;
|
Contributor
Author
|
This PR possibly superseded by this PR: #148551 |
Member
|
I like the size of this PR. But I wanted to use the opportunity to fix up the existing You could still add the tests, although I think integer literals are already covered with #148551. (The context is different, but that shouldn't matter.) I'm fine either way. In any event, sorry that I forgot to land the other change, and thanks for putting in the work anyway. |
8a068dd to
013caa6
Compare
Add a simple literal-based alias test that shows that the recently fixed value-based literal comparison works when resolving aliases. NFC.
013caa6 to
0d4eea1
Compare
Contributor
Author
|
Reduced this PR to a simple NFC that just adds one of the literal-based alias tests, just to be sure there's no regression here in future. |
moar55
pushed a commit
to moar55/llvm-project
that referenced
this pull request
Feb 3, 2026
Add a simple literal-based alias test that shows that the recently fixed value-based literal comparison works when resolving aliases. NFC.
rishabhmadan19
pushed a commit
to rishabhmadan19/llvm-project
that referenced
this pull request
Feb 9, 2026
Add a simple literal-based alias test that shows that the recently fixed value-based literal comparison works when resolving aliases. NFC.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a simple literal-based alias test that shows that the recently fixed
value-based literal comparison works when resolving aliases.
NFC.