Thread Safety Analysis: Fix implicit member access in attributes#194457
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: Marco Elver (melver) ChangesSExprBuilder previously translated DeclRefExprs referring to FieldDecls as plain global references (til::LiteralPtr), ignoring the base object. This caused false positives when members were accessed implicitly (such as in C, or parameter attributes in C++) because the context of the parent object was lost. Fix this by using translateCXXThisExpr() to evaluate SelfArg into a base expression when translating a DeclRefExpr to a FieldDecl. This makes the analysis behave correctly for implicit member accesses in attributes. Assisted-by: Gemini 3 (for debugging and review) Full diff: https://github.com/llvm/llvm-project/pull/194457.diff 3 Files Affected:
diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp
index b43a986521f99..1eabfb6f57071 100644
--- a/clang/lib/Analysis/ThreadSafetyCommon.cpp
+++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp
@@ -374,6 +374,28 @@ til::SExpr *SExprBuilder::translate(const Stmt *S, CallingContext *Ctx) {
return new (Arena) til::Undefined(S);
}
+static const ValueDecl *getValueDeclFromSExpr(const til::SExpr *E) {
+ if (const auto *V = dyn_cast<til::Variable>(E))
+ return V->clangDecl();
+ if (const auto *Ph = dyn_cast<til::Phi>(E))
+ return Ph->clangDecl();
+ if (const auto *P = dyn_cast<til::Project>(E))
+ return P->clangDecl();
+ if (const auto *L = dyn_cast<til::LiteralPtr>(E))
+ return L->clangDecl();
+ return nullptr;
+}
+
+static bool hasAnyPointerType(const til::SExpr *E) {
+ auto *VD = getValueDeclFromSExpr(E);
+ if (VD && VD->getType()->isAnyPointerType())
+ return true;
+ if (const auto *C = dyn_cast<til::Cast>(E))
+ return C->castOpcode() == til::CAST_objToPtr;
+
+ return false;
+}
+
til::SExpr *SExprBuilder::translateDeclRefExpr(const DeclRefExpr *DRE,
CallingContext *Ctx) {
const auto *VD = cast<ValueDecl>(DRE->getDecl()->getCanonicalDecl());
@@ -408,6 +430,15 @@ til::SExpr *SExprBuilder::translateDeclRefExpr(const DeclRefExpr *DRE,
if (const auto *VarD = dyn_cast<VarDecl>(VD))
return translateVariable(VarD, Ctx);
+ if (const auto *FD = dyn_cast<FieldDecl>(VD); FD && Ctx && Ctx->SelfArg) {
+ til::SExpr *BE = translateCXXThisExpr(nullptr, Ctx);
+ til::SExpr *E = new (Arena) til::SApply(BE);
+ til::Project *P = new (Arena) til::Project(E, FD);
+ if (hasAnyPointerType(BE))
+ P->setArrow(true);
+ return P;
+ }
+
// For non-local variables, treat it as a reference to a named object.
return new (Arena) til::LiteralPtr(VD);
}
@@ -425,28 +456,6 @@ til::SExpr *SExprBuilder::translateCXXThisExpr(const CXXThisExpr *TE,
return SelfVar;
}
-static const ValueDecl *getValueDeclFromSExpr(const til::SExpr *E) {
- if (const auto *V = dyn_cast<til::Variable>(E))
- return V->clangDecl();
- if (const auto *Ph = dyn_cast<til::Phi>(E))
- return Ph->clangDecl();
- if (const auto *P = dyn_cast<til::Project>(E))
- return P->clangDecl();
- if (const auto *L = dyn_cast<til::LiteralPtr>(E))
- return L->clangDecl();
- return nullptr;
-}
-
-static bool hasAnyPointerType(const til::SExpr *E) {
- auto *VD = getValueDeclFromSExpr(E);
- if (VD && VD->getType()->isAnyPointerType())
- return true;
- if (const auto *C = dyn_cast<til::Cast>(E))
- return C->castOpcode() == til::CAST_objToPtr;
-
- return false;
-}
-
// Grab the very first declaration of virtual method D
static const CXXMethodDecl *getFirstVirtualDecl(const CXXMethodDecl *D) {
while (true) {
diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c
index 0fd382eb02b78..c1fcbace2d29d 100644
--- a/clang/test/Sema/warn-thread-safety-analysis.c
+++ b/clang/test/Sema/warn-thread-safety-analysis.c
@@ -32,6 +32,10 @@
// Simplified only for test purpose.
struct LOCKABLE Mutex {};
+struct Task {
+ struct Mutex mu;
+};
+
struct Foo {
struct Mutex *mu_;
int a_value GUARDED_BY(mu_);
@@ -42,6 +46,10 @@ struct Foo {
} bar;
int* a_ptr PT_GUARDED_BY(bar.other_mu);
+
+ struct Task *task;
+ int b_value GUARDED_BY(&task->mu);
+ int* b_ptr PT_GUARDED_BY(&task->mu);
};
struct LOCKABLE Lock {};
@@ -206,22 +214,29 @@ int main(void) {
a_ = 42;
}
- foo_.a_value = 0; // expected-warning {{writing variable 'a_value' requires holding mutex 'mu_' exclusively}}
- *foo_.a_ptr = 1; // expected-warning {{writing the value pointed to by 'a_ptr' requires holding mutex 'bar.other_mu' exclusively}}
-
+ foo_.a_value = 0; // expected-warning {{writing variable 'a_value' requires holding mutex 'foo_.mu_' exclusively}}
+ *foo_.a_ptr = 1; // expected-warning {{writing the value pointed to by 'a_ptr' requires holding mutex 'foo_.bar.other_mu' exclusively}}
+ foo_.b_value = 0; // expected-warning {{writing variable 'b_value' requires holding mutex 'foo_.task->mu' exclusively}}
+ *foo_.b_ptr = 1; // expected-warning {{writing the value pointed to by 'b_ptr' requires holding mutex 'foo_.task->mu' exclusively}}
mutex_exclusive_lock(foo_.bar.other_mu);
+ *foo_.a_ptr = 1;
mutex_exclusive_lock(foo_.bar.third_mu); // expected-warning{{mutex 'third_mu' must be acquired before 'other_mu'}}
mutex_exclusive_lock(foo_.mu_); // expected-warning{{mutex 'mu_' must be acquired before 'other_mu'}}
mutex_exclusive_unlock(foo_.mu_);
mutex_exclusive_unlock(foo_.bar.other_mu);
mutex_exclusive_unlock(foo_.bar.third_mu);
+ mutex_exclusive_lock(&foo_.task->mu);
+ foo_.b_value = 0;
+ *foo_.b_ptr = 1;
+ mutex_exclusive_unlock(&foo_.task->mu);
+
#ifdef LATE_PARSING
- late_parsing.a_value_defined_before = 1; // expected-warning{{writing variable 'a_value_defined_before' requires holding mutex 'a_mutex_defined_late' exclusively}}
+ late_parsing.a_value_defined_before = 1; // expected-warning{{writing variable 'a_value_defined_before' requires holding mutex 'late_parsing.a_mutex_defined_late' exclusively}}
late_parsing.a_ptr_defined_before = 0;
set_value(&late_parsing.a_value_defined_before, 0); // expected-warning{{calling function 'set_value' requires holding mutex 'foo_.mu_' exclusively}}
- // expected-warning@-1{{passing pointer to variable 'a_value_defined_before' requires holding mutex 'a_mutex_defined_late'}}
+ // expected-warning@-1{{passing pointer to variable 'a_value_defined_before' requires holding mutex 'late_parsing.a_mutex_defined_late'}}
mutex_exclusive_lock(late_parsing.a_mutex_defined_late);
mutex_exclusive_lock(late_parsing.a_mutex_defined_early); // expected-warning{{mutex 'a_mutex_defined_early' must be acquired before 'a_mutex_defined_late'}}
mutex_exclusive_unlock(late_parsing.a_mutex_defined_early);
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index 079e068ca7811..dedf7f9c38427 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -169,10 +169,10 @@ struct TestingMoreComplexAttributes {
} more_complex_atttributes;
void more_complex_attributes() {
- more_complex_atttributes.a = true; // expected-warning{{writing variable 'a' requires holding mutex 'lock' exclusively}}
- more_complex_atttributes.b = true; // expected-warning{{writing variable 'b' requires holding mutex 'strct.lock' exclusively}}
- *more_complex_atttributes.ptr_a = true; // expected-warning{{writing the value pointed to by 'ptr_a' requires holding mutex 'lock' exclusively}}
- *more_complex_atttributes.ptr_b = true; // expected-warning{{writing the value pointed to by 'ptr_b' requires holding mutex 'strct.lock' exclusively}}
+ more_complex_atttributes.a = true; // expected-warning{{writing variable 'a' requires holding mutex 'more_complex_atttributes..lock' exclusively}}
+ more_complex_atttributes.b = true; // expected-warning{{writing variable 'b' requires holding mutex 'more_complex_atttributes..strct.lock' exclusively}}
+ *more_complex_atttributes.ptr_a = true; // expected-warning{{writing the value pointed to by 'ptr_a' requires holding mutex 'more_complex_atttributes..lock' exclusively}}
+ *more_complex_atttributes.ptr_b = true; // expected-warning{{writing the value pointed to by 'ptr_b' requires holding mutex 'more_complex_atttributes..strct.lock' exclusively}}
more_complex_atttributes.lock.Lock();
more_complex_atttributes.lock1.Lock(); // expected-warning{{mutex 'lock1' must be acquired before 'lock'}}
@@ -3739,14 +3739,10 @@ void requireDecl(RelockableScope &scope) {
struct foo
{
Mutex mu;
- // expected-note@+1{{see attribute on parameter here}}
void require(RelockableScope &scope EXCLUSIVE_LOCKS_REQUIRED(mu));
void callRequire(){
RelockableScope scope(&mu);
- // TODO: False positive due to incorrect parsing of parameter attribute arguments.
require(scope);
- // expected-warning@-1{{calling function 'require' requires holding mutex 'mu' exclusively}}
- // expected-warning@-2{{mutex managed by 'scope' is 'mu' instead of 'mu'}}
}
};
@@ -7717,6 +7713,18 @@ void testAliasChainUnrelatedReassignment2(ContainerOfPtr *list) {
eb->mu.Unlock();
}
+struct GuardedByIndirection {
+ int data GUARDED_BY(foo_ptr->mu);
+ Foo *foo_ptr;
+};
+
+void testGuardedByIndirection(GuardedByIndirection *x) {
+ Foo *f = x->foo_ptr;
+ f->mu.Lock();
+ x->data = 42;
+ f->mu.Unlock();
+}
+
void testControlFlowDoWhile(Foo *f, int x) {
Foo *ptr = f;
|
|
cc @bvanassche |
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
Thank you! LGTM though this should have a release note in clang/docs/ReleaseNotes.rst so users know about the fix.
aaronpuchert
left a comment
aaronpuchert
left a comment
There was a problem hiding this comment.
I think this should be fixed in the AST, and not worked around in the TIL translation. Fields should never be referenced by a DeclRefExpr, but only by MemberExprs. It would also be good to keep a correspondence DeclRefExpr ↔ til::LiteralPtr, MemberExpr ↔ til::Project.
I know this is an issue in C, but this should better be fixed by introducing a built-in equivalent to CXXThisExpr.
On the one hand, yes (though I would argue it's not "fixed"; C has no such notion so this is purely a language extension at this point); it would make more sense to model this as a kind of member expression. On the other hand, that's a fairly significant amount of effort by comparison. But on the third hand, I think that effort may help in other circumstances like with bounds safety attributes which also want to be able to model fields which can implicitly reference other fields. CC @rapidsna in case there's already been investigation into this approach. |
Good points, but the AST refactor is probably out of scope for this PR. I've left the TIL fix with a FIXME hinting at the eventual AST-based fix. This fix here is quite narrow, and unless it interferes with anything else, I'd propose merging is a near-term fix so that users aren't blocked on this - this fix was in response to a report by a Linux kernel maintainer (we already expect needing to bump min. Clang version to use -Wthread-safety to 23, given the various issues that have been fixed after Clang 22's release). |
SExprBuilder previously translated DeclRefExprs referring to FieldDecls as plain global references (til::LiteralPtr), ignoring the base object. This caused false positives when members were accessed implicitly (such as in C, or parameter attributes in C++) because the context of the parent object was lost. Fix this by using translateCXXThisExpr() to evaluate SelfArg into a base expression when translating a DeclRefExpr to a FieldDecl. This makes the analysis behave correctly for implicit member accesses in attributes. Assisted-by: Gemini 3 (for debugging and review)
I'm comfortable with that. |
…m#194457) SExprBuilder previously translated DeclRefExprs referring to FieldDecls as plain global references (til::LiteralPtr), ignoring the base object. This caused false positives when members were accessed implicitly (such as in C, or parameter attributes in C++) because the context of the parent object was lost. Fix this by using translateCXXThisExpr() to evaluate SelfArg into a base expression when translating a DeclRefExpr to a FieldDecl. This makes the analysis behave correctly for implicit member accesses in attributes. Assisted-by: Gemini 3 (for debugging and review)
|
Thanks for addressing this issue! @AaronBallman @aaronpuchert Agreed that the proper fix here is to introduce a notion of this for C, rather than having |
…m#194457) SExprBuilder previously translated DeclRefExprs referring to FieldDecls as plain global references (til::LiteralPtr), ignoring the base object. This caused false positives when members were accessed implicitly (such as in C, or parameter attributes in C++) because the context of the parent object was lost. Fix this by using translateCXXThisExpr() to evaluate SelfArg into a base expression when translating a DeclRefExpr to a FieldDecl. This makes the analysis behave correctly for implicit member accesses in attributes. Assisted-by: Gemini 3 (for debugging and review)
…m#194457) SExprBuilder previously translated DeclRefExprs referring to FieldDecls as plain global references (til::LiteralPtr), ignoring the base object. This caused false positives when members were accessed implicitly (such as in C, or parameter attributes in C++) because the context of the parent object was lost. Fix this by using translateCXXThisExpr() to evaluate SelfArg into a base expression when translating a DeclRefExpr to a FieldDecl. This makes the analysis behave correctly for implicit member accesses in attributes. Assisted-by: Gemini 3 (for debugging and review)
Clang 23 introduces several major improvements: 1. Support for multiple arguments in the `guarded_by` and `pt_guarded_by` attributes [1]. This allows defining variables protected by multiple context locks, where read access requires holding at least one lock (shared or exclusive), and write access requires holding all of them exclusively. 2. Function pointer support [2]. We can now add attributes to function pointers just like we do on normal functions. 3. A fix to use arrays of locks [3]. Each index is now correctly treated as a separate lock instance. 4. A fix for implicit member access in attributes [4]. This allows to use __guarded_by(&foo->lock) correctly. Overall that makes it worthwhile bumping the compiler version instead of trying to make both Clang 22 and later work while supporting these new features. Signed-off-by: Marco Elver <elver@google.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Reviewed-by: Nathan Chancellor <nathan@kernel.org> Reviewed-by: Bart Van Assche <bvanassche@acm.org> Link: llvm/llvm-project#186838 [1] Link: llvm/llvm-project#191187 [2] Link: llvm/llvm-project#148551 [3] Link: llvm/llvm-project#194457 [4] Link: https://patch.msgid.link/20260515124426.2227783-1-elver@google.com
Clang 23 introduces several major improvements: 1. Support for multiple arguments in the `guarded_by` and `pt_guarded_by` attributes [1]. This allows defining variables protected by multiple context locks, where read access requires holding at least one lock (shared or exclusive), and write access requires holding all of them exclusively. 2. Function pointer support [2]. We can now add attributes to function pointers just like we do on normal functions. 3. A fix to use arrays of locks [3]. Each index is now correctly treated as a separate lock instance. 4. A fix for implicit member access in attributes [4]. This allows to use __guarded_by(&foo->lock) correctly. Overall that makes it worthwhile bumping the compiler version instead of trying to make both Clang 22 and later work while supporting these new features. Signed-off-by: Marco Elver <elver@google.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Reviewed-by: Nathan Chancellor <nathan@kernel.org> Reviewed-by: Bart Van Assche <bvanassche@acm.org> Link: llvm/llvm-project#186838 [1] Link: llvm/llvm-project#191187 [2] Link: llvm/llvm-project#148551 [3] Link: llvm/llvm-project#194457 [4] Link: https://patch.msgid.link/20260515124426.2227783-1-elver@google.com
5 participants
SExprBuilder previously translated DeclRefExprs referring to FieldDecls as plain global references (til::LiteralPtr), ignoring the base object. This caused false positives when members were accessed implicitly (such as in C, or parameter attributes in C++) because the context of the parent object was lost.
Fix this by using translateCXXThisExpr() to evaluate SelfArg into a base expression when translating a DeclRefExpr to a FieldDecl. This makes the analysis behave correctly for implicit member accesses in attributes.
Assisted-by: Gemini 3 (for debugging and review)