[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
Marco Elver via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 15 07:02:52 PST 2025
https://github.com/melver updated https://github.com/llvm/llvm-project/pull/123063
>From 6727047d25b8b72f8e23b03a84c0b23f6dad566a Mon Sep 17 00:00:00 2001
From: Marco Elver <elver at google.com>
Date: Wed, 15 Jan 2025 13:23:14 +0100
Subject: [PATCH] Thread Safety Analysis: Support warning on obtaining address
of guarded variables
Add the optional ability, via `-Wthread-safety-addressof`, to warn when
obtaining the address of guarded variables.
This is required to avoid false negatives in large C codebases, where
data structures are typically implemented through helpers that take
pointers to instances of a data structure.
We also argue that, while obtaining the address itself does not yet
constitute a potential race (in the presence of missing locking),
placing the requirement on the pointer-recipient to obtain locks to
access the pointed-to data is most likely poor style. This is analogous
to passing C++ references to guarded variables, which produces warnings
by default.
Given that existing codebases using `-Wthread-safety` likely have cases
where obtaining the pointer to a guarded variable is benign, the feature
is not enabled by default but requires explicit opt-in.
---
clang/docs/ReleaseNotes.rst | 3 ++
clang/docs/ThreadSafetyAnalysis.rst | 12 ++++-
.../clang/Analysis/Analyses/ThreadSafety.h | 3 ++
clang/include/clang/Basic/DiagnosticGroups.td | 1 +
.../clang/Basic/DiagnosticSemaKinds.td | 8 ++++
clang/lib/Analysis/ThreadSafety.cpp | 3 ++
clang/lib/Sema/AnalysisBasedWarnings.cpp | 26 ++++++++---
clang/test/Sema/warn-thread-safety-analysis.c | 11 +++++
.../SemaCXX/warn-thread-safety-analysis.cpp | 44 ++++++++++++++++++-
9 files changed, 102 insertions(+), 9 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 794943b24a003c..abcf0ef236cc2e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -788,6 +788,9 @@ Improvements to Clang's diagnostics
require(scope); // Warning! Requires mu1.
}
+- The :doc:`ThreadSafetyAnalysis` now supports ``-Wthread-safety-addressof``,
+ which enables warning if the address of guarded variables is obtained.
+
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/docs/ThreadSafetyAnalysis.rst b/clang/docs/ThreadSafetyAnalysis.rst
index 9c1c32e46989bc..c506844e7c94dc 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -515,8 +515,16 @@ Warning flags
+ ``-Wthread-safety-analysis``: The core analysis.
+ ``-Wthread-safety-precise``: Requires that mutex expressions match precisely.
This warning can be disabled for code which has a lot of aliases.
- + ``-Wthread-safety-reference``: Checks when guarded members are passed by reference.
-
+ + ``-Wthread-safety-reference``: Checks when guarded variables are passed by reference.
+
+* ``-Wthread-safety-addressof``: Warn when the address of guarded variables is
+ obtained (``&var``). Since obtaining the address of a variable doesn't
+ necessarily imply a read or write, the warning is off by default. In
+ codebases that prefer passing pointers rather than references (for C++
+ codebases), or passing pointers is ubiquitous (for C codebases), enabling
+ this warning will result in fewer false negatives; for example, where the
+ manipulation of common data structures is done via functions that take
+ pointers to instances of the data structure.
:ref:`negative` are an experimental feature, which are enabled with:
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 0fcf5bed1285a4..a61c60cbf531e9 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -54,6 +54,9 @@ enum ProtectedOperationKind {
/// Returning a pt-guarded variable by reference.
POK_PtReturnByRef,
+
+ /// Obtaining address of a variable (e.g. &x).
+ POK_AddressOf,
};
/// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 594e99a19b64d6..8bd5d043cefa80 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1126,6 +1126,7 @@ def ThreadSafetyReferenceReturn : DiagGroup<"thread-safety-reference-return">;
def ThreadSafetyReference : DiagGroup<"thread-safety-reference",
[ThreadSafetyReferenceReturn]>;
def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAddressof : DiagGroup<"thread-safety-addressof">;
def ThreadSafety : DiagGroup<"thread-safety",
[ThreadSafetyAttributes,
ThreadSafetyAnalysis,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 8be4f946dce1cc..8e980dc31e3003 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4140,6 +4140,14 @@ def warn_thread_safety_verbose : Warning<"thread safety verbose warning">,
def note_thread_warning_in_fun : Note<"thread warning in function %0">;
def note_guarded_by_declared_here : Note<"guarded_by declared here">;
+// Thread safety warnings on addressof
+def warn_addressof_requires_any_lock : Warning<
+ "obtaining address of variable %0 requires holding any mutex">,
+ InGroup<ThreadSafetyAddressof>, DefaultIgnore;
+def warn_addressof_requires_lock : Warning<
+ "obtaining address of variable %1 requires holding %0 '%2'">,
+ InGroup<ThreadSafetyAddressof>, DefaultIgnore;
+
// Dummy warning that will trigger "beta" warnings from the analysis if enabled.
def warn_thread_safety_beta : Warning<"thread safety beta warning">,
InGroup<ThreadSafetyBeta>, DefaultIgnore;
diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index bfaf1a0e7c7ffc..3f06b40a623475 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -2080,6 +2080,9 @@ void BuildLockset::VisitUnaryOperator(const UnaryOperator *UO) {
case UO_PreInc:
checkAccess(UO->getSubExpr(), AK_Written);
break;
+ case UO_AddrOf:
+ checkAccess(UO->getSubExpr(), AK_Read, POK_AddressOf);
+ break;
default:
break;
}
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 589869d0186575..ace36cbecb876f 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1983,11 +1983,21 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
void handleNoMutexHeld(const NamedDecl *D, ProtectedOperationKind POK,
AccessKind AK, SourceLocation Loc) override {
- assert((POK == POK_VarAccess || POK == POK_VarDereference) &&
- "Only works for variables");
- unsigned DiagID = POK == POK_VarAccess?
- diag::warn_variable_requires_any_lock:
- diag::warn_var_deref_requires_any_lock;
+ unsigned DiagID = 0;
+ switch (POK) {
+ case POK_VarAccess:
+ DiagID = diag::warn_variable_requires_any_lock;
+ break;
+ case POK_VarDereference:
+ DiagID = diag::warn_var_deref_requires_any_lock;
+ break;
+ case POK_AddressOf:
+ DiagID = diag::warn_addressof_requires_any_lock;
+ break;
+ default:
+ assert(false && "Only works for variables");
+ break;
+ }
PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID)
<< D << getLockKindFromAccessKind(AK));
Warnings.emplace_back(std::move(Warning), getNotes());
@@ -2006,6 +2016,9 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
case POK_VarDereference:
DiagID = diag::warn_var_deref_requires_lock_precise;
break;
+ case POK_AddressOf:
+ DiagID = diag::warn_addressof_requires_lock;
+ break;
case POK_FunctionCall:
DiagID = diag::warn_fun_requires_lock_precise;
break;
@@ -2042,6 +2055,9 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
case POK_VarDereference:
DiagID = diag::warn_var_deref_requires_lock;
break;
+ case POK_AddressOf:
+ DiagID = diag::warn_addressof_requires_lock;
+ break;
case POK_FunctionCall:
DiagID = diag::warn_fun_requires_lock;
break;
diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c
index 73b4e4798f6443..cc6af79aadb22b 100644
--- a/clang/test/Sema/warn-thread-safety-analysis.c
+++ b/clang/test/Sema/warn-thread-safety-analysis.c
@@ -1,5 +1,6 @@
// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta %s
// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta -fexperimental-late-parse-attributes -DLATE_PARSING %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta -Wthread-safety-addressof -fexperimental-late-parse-attributes -DLATE_PARSING -DCHECK_ADDRESSOF %s
#define LOCKABLE __attribute__ ((lockable))
#define SCOPED_LOCKABLE __attribute__ ((scoped_lockable))
@@ -133,7 +134,12 @@ int main(void) {
Foo_func3(5);
+#ifdef CHECK_ADDRESSOF
+ set_value(&a_, 0); // expected-warning{{calling function 'set_value' requires holding mutex 'foo_.mu_' exclusively}} \
+ expected-warning{{obtaining address of variable 'a_' requires holding mutex 'foo_.mu_'}}
+#else
set_value(&a_, 0); // expected-warning{{calling function 'set_value' requires holding mutex 'foo_.mu_' exclusively}}
+#endif
get_value(b_); // expected-warning{{calling function 'get_value' requires holding mutex 'foo_.mu_'}}
mutex_exclusive_lock(foo_.mu_);
set_value(&a_, 1);
@@ -180,6 +186,11 @@ int main(void) {
#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_ptr_defined_before = 0;
+# ifdef CHECK_ADDRESSOF
+ (void)&late_parsing.a_value_defined_before; // expected-warning{{obtaining address of variable 'a_value_defined_before' requires holding mutex 'a_mutex_defined_late'}}
+# else
+ (void)&late_parsing.a_value_defined_before; // no warning
+# endif
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 018d6d1bb258b5..ce5c08d32328c2 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -1,7 +1,9 @@
// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -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-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -Wthread-safety-addressof -fcxx-exceptions -DUSE_CAPABILITY=1 -DCHECK_ADDRESSOF %s
// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -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-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -Wthread-safety-addressof -fcxx-exceptions -DUSE_CAPABILITY=1 -DCHECK_ADDRESSOF %s
// FIXME: should also run %clang_cc1 -fsyntax-only -verify -Wthread-safety -std=c++11 -Wc++98-compat %s
// FIXME: should also run %clang_cc1 -fsyntax-only -verify -Wthread-safety %s
@@ -4863,6 +4865,11 @@ class Data {
int dat;
};
+class DataWithAddrOf : public Data {
+public:
+ void* operator&() const;
+};
+
class DataCell {
public:
@@ -4900,10 +4907,15 @@ class Foo {
a = (*datap2_).getValue(); // \
// expected-warning {{reading the value pointed to by 'datap2_' requires holding mutex 'mu_'}}
+ // Calls operator&, and does not obtain the address.
+ (void)&data_ao_; // expected-warning {{reading variable 'data_ao_' requires holding mutex 'mu_'}}
+ (void)__builtin_addressof(data_ao_); // expected-warning {{passing variable 'data_ao_' by reference requires holding mutex 'mu_'}}
+
mu_.Lock();
data_.setValue(1);
datap1_->setValue(1);
datap2_->setValue(1);
+ (void)&data_ao_;
mu_.Unlock();
mu_.ReaderLock();
@@ -4911,6 +4923,7 @@ class Foo {
datap1_->setValue(0); // reads datap1_, writes *datap1_
a = datap1_->getValue();
a = datap2_->getValue();
+ (void)&data_ao_;
mu_.Unlock();
}
@@ -4939,11 +4952,29 @@ class Foo {
data_++; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
--data_; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
data_--; // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
+#ifdef CHECK_ADDRESSOF
+ (void)&data_; // expected-warning {{obtaining address of variable 'data_' requires holding mutex 'mu_'}}
+ (void)&datap1_; // expected-warning {{obtaining address of variable 'datap1_' requires holding mutex 'mu_'}}
+#else
+ (void)&data_; // no warning
+ (void)&datap1_; // no warning
+#endif
+ (void)(&*datap1_); // expected-warning {{reading variable 'datap1_' requires holding mutex 'mu_'}}
data_[0] = 0; // expected-warning {{reading variable 'data_' requires holding mutex 'mu_'}}
(*datap2_)[0] = 0; // expected-warning {{reading the value pointed to by 'datap2_' requires holding mutex 'mu_'}}
+ (void)&datap2_; // no warning
+ (void)(&*datap2_); // expected-warning {{reading the value pointed to by 'datap2_' requires holding mutex 'mu_'}}
data_(); // expected-warning {{reading variable 'data_' requires holding mutex 'mu_'}}
+
+ mu_.Lock();
+ (void)&data_;
+ mu_.Unlock();
+
+ mu_.ReaderLock();
+ (void)&data_;
+ mu_.Unlock();
}
// const operator tests
@@ -4962,6 +4993,7 @@ class Foo {
Data data_ GUARDED_BY(mu_);
Data* datap1_ GUARDED_BY(mu_);
Data* datap2_ PT_GUARDED_BY(mu_);
+ DataWithAddrOf data_ao_ GUARDED_BY(mu_);
};
} // end namespace GuardedNonPrimitiveTypeTest
@@ -5870,7 +5902,11 @@ class Foo {
}
void ptr_test() {
- int *b = &a;
+#ifdef CHECK_ADDRESSOF
+ int *b = &a; // expected-warning {{obtaining address of variable 'a' requires holding mutex 'mu'}}
+#else
+ int *b = &a; // no warning
+#endif
*b = 0; // no expected warning yet
}
@@ -6089,7 +6125,11 @@ class Return {
}
Foo *returns_ptr() {
- return &foo; // FIXME -- Do we want to warn on this ?
+#ifdef CHECK_ADDRESSOF
+ return &foo; // expected-warning {{obtaining address of variable 'foo' requires holding mutex 'mu'}}
+#else
+ return &foo; // no warning
+#endif
}
Foo &returns_ref2() {
More information about the cfe-commits
mailing list