Thread Safety Analysis: Improved pointer handling #127396
Closed
Conversation
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
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: Marco Elver (melver) ChangesIntroduce Adding checks for pointer passing 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. Patch is 21.44 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/127396.diff 9 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index efaacdf18d50a..830433853437d 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -143,6 +143,11 @@ Improvements to Clang's diagnostics
- A statement attribute applied to a ``case`` label no longer suppresses
'bypassing variable initialization' diagnostics (#84072).
+- The :doc:`ThreadSafetyAnalysis` now supports ``-Wthread-safety-pointer``
+ (with ``-Wthread-safety-beta``), which enables warning on passing or
+ returning pointers to guarded variables as function arguments or return value
+ respectively.
+
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/docs/ThreadSafetyAnalysis.rst b/clang/docs/ThreadSafetyAnalysis.rst
index 9c1c32e46989b..1e7950067f608 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -515,7 +515,8 @@ 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 members are passed or
+ returned by reference.
:ref:`negative` are an experimental feature, which are enabled with:
@@ -528,6 +529,9 @@ for a period of time, after which they are migrated into the standard analysis.
* ``-Wthread-safety-beta``: New features. Off by default.
+ + ``-Wthread-safety-pointer``: Checks when passing or returning pointers to
+ guarded variables, or pointers to guarded data, as function argument or
+ return value respectively.
.. _negative:
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 0fcf5bed1285a..20b75c46593e0 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -54,6 +54,18 @@ enum ProtectedOperationKind {
/// Returning a pt-guarded variable by reference.
POK_PtReturnByRef,
+
+ /// Passing pointer to a guarded variable.
+ POK_PassPointer,
+
+ /// Passing a pt-guarded pointer.
+ POK_PtPassPointer,
+
+ /// Returning pointer to a guarded variable.
+ POK_ReturnPointer,
+
+ /// Returning a pt-guarded pointer.
+ POK_PtReturnPointer,
};
/// 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 05e39899e6f25..94c44cd199ed9 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1156,6 +1156,7 @@ def ThreadSafetyPrecise : DiagGroup<"thread-safety-precise">;
def ThreadSafetyReferenceReturn : DiagGroup<"thread-safety-reference-return">;
def ThreadSafetyReference : DiagGroup<"thread-safety-reference",
[ThreadSafetyReferenceReturn]>;
+def ThreadSafetyPointer : DiagGroup<"thread-safety-pointer">;
def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">;
def ThreadSafety : DiagGroup<"thread-safety",
[ThreadSafetyAttributes,
@@ -1163,7 +1164,8 @@ def ThreadSafety : DiagGroup<"thread-safety",
ThreadSafetyPrecise,
ThreadSafetyReference]>;
def ThreadSafetyVerbose : DiagGroup<"thread-safety-verbose">;
-def ThreadSafetyBeta : DiagGroup<"thread-safety-beta">;
+def ThreadSafetyBeta : DiagGroup<"thread-safety-beta",
+ [ThreadSafetyPointer]>;
// Warnings and notes related to the function effects system which underlies
// the nonblocking and nonallocating attributes.
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f10af8f5bd6b2..ad5310145c841 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4135,6 +4135,24 @@ def warn_pt_guarded_return_by_reference : Warning<
"%select{'%2'|'%2' exclusively}3">,
InGroup<ThreadSafetyReferenceReturn>, DefaultIgnore;
+// Thread safety warnings on pointer passing
+def warn_guarded_pass_pointer : Warning<
+ "passing pointer to variable %1 requires holding %0 "
+ "%select{'%2'|'%2' exclusively}3">,
+ InGroup<ThreadSafetyPointer>, DefaultIgnore;
+def warn_pt_guarded_pass_pointer : Warning<
+ "passing pointer %1 requires holding %0 "
+ "%select{'%2'|'%2' exclusively}3">,
+ InGroup<ThreadSafetyPointer>, DefaultIgnore;
+def warn_guarded_return_pointer : Warning<
+ "returning pointer to variable %1 requires holding %0 "
+ "%select{'%2'|'%2' exclusively}3">,
+ InGroup<ThreadSafetyPointer>, DefaultIgnore;
+def warn_pt_guarded_return_pointer : Warning<
+ "returning pointer %1 requires holding %0 "
+ "%select{'%2'|'%2' exclusively}3">,
+ InGroup<ThreadSafetyPointer>, DefaultIgnore;
+
// Imprecise thread safety warnings
def warn_variable_requires_lock : Warning<
"%select{reading|writing}3 variable %1 requires holding %0 "
diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index bfaf1a0e7c7ff..949960bd4a6fe 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1719,9 +1719,16 @@ void ThreadSafetyAnalyzer::checkAccess(const FactSet &FSet, const Expr *Exp,
}
if (const auto *UO = dyn_cast<UnaryOperator>(Exp)) {
- // For dereferences
- if (UO->getOpcode() == UO_Deref)
+ switch (UO->getOpcode()) {
+ case UO_Deref: // dereference
checkPtAccess(FSet, UO->getSubExpr(), AK, POK);
+ break;
+ case UO_AddrOf: // addressof
+ checkAccess(FSet, UO->getSubExpr(), AK, POK);
+ break;
+ default:
+ break;
+ }
return;
}
@@ -1785,9 +1792,22 @@ void ThreadSafetyAnalyzer::checkPtAccess(const FactSet &FSet, const Expr *Exp,
// Pass by reference warnings are under a different flag.
ProtectedOperationKind PtPOK = POK_VarDereference;
- if (POK == POK_PassByRef) PtPOK = POK_PtPassByRef;
- if (POK == POK_ReturnByRef)
+ switch (POK) {
+ case POK_PassByRef:
+ PtPOK = POK_PtPassByRef;
+ break;
+ case POK_ReturnByRef:
PtPOK = POK_PtReturnByRef;
+ break;
+ case POK_PassPointer:
+ PtPOK = POK_PtPassPointer;
+ break;
+ case POK_ReturnPointer:
+ PtPOK = POK_PtReturnPointer;
+ break;
+ default:
+ break;
+ }
const ValueDecl *D = getValueDecl(Exp);
if (!D || !D->hasAttrs())
@@ -2132,8 +2152,17 @@ void BuildLockset::examineArguments(const FunctionDecl *FD,
for (auto Arg = ArgBegin; Param != Params.end() && Arg != ArgEnd;
++Param, ++Arg) {
QualType Qt = (*Param)->getType();
- if (Qt->isReferenceType())
+ if (Qt->isReferenceType()) {
checkAccess(*Arg, AK_Read, POK_PassByRef);
+ } else if (Qt->isPointerType()) {
+ const auto *Exp = (*Arg)->IgnoreImplicit()->IgnoreParenCasts();
+ if (const auto *UO = dyn_cast<UnaryOperator>(Exp)) {
+ if (UO->getOpcode() == UO_AddrOf)
+ checkAccess(Exp, AK_Read, POK_PassPointer);
+ } else {
+ checkPtAccess(Exp, AK_Read, POK_PassPointer);
+ }
+ }
}
}
@@ -2284,6 +2313,23 @@ void BuildLockset::VisitReturnStmt(const ReturnStmt *S) {
FunctionExitFSet, RetVal,
ReturnType->getPointeeType().isConstQualified() ? AK_Read : AK_Written,
POK_ReturnByRef);
+ } else if (ReturnType->isPointerType()) {
+ const auto *Exp = RetVal->IgnoreImplicit()->IgnoreParenCasts();
+ if (const auto *UO = dyn_cast<UnaryOperator>(Exp)) {
+ if (UO->getOpcode() == UO_AddrOf) {
+ Analyzer->checkAccess(FunctionExitFSet, Exp,
+ ReturnType->getPointeeType().isConstQualified()
+ ? AK_Read
+ : AK_Written,
+ POK_ReturnPointer);
+ }
+ } else {
+ Analyzer->checkPtAccess(FunctionExitFSet, Exp,
+ ReturnType->getPointeeType().isConstQualified()
+ ? AK_Read
+ : AK_Written,
+ POK_ReturnPointer);
+ }
}
}
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 589869d018657..f0ea6cfbf33f9 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2021,6 +2021,18 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
case POK_PtReturnByRef:
DiagID = diag::warn_pt_guarded_return_by_reference;
break;
+ case POK_PassPointer:
+ DiagID = diag::warn_guarded_pass_pointer;
+ break;
+ case POK_PtPassPointer:
+ DiagID = diag::warn_pt_guarded_pass_pointer;
+ break;
+ case POK_ReturnPointer:
+ DiagID = diag::warn_guarded_return_pointer;
+ break;
+ case POK_PtReturnPointer:
+ DiagID = diag::warn_pt_guarded_return_pointer;
+ break;
}
PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind
<< D
@@ -2057,6 +2069,18 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
case POK_PtReturnByRef:
DiagID = diag::warn_pt_guarded_return_by_reference;
break;
+ case POK_PassPointer:
+ DiagID = diag::warn_guarded_pass_pointer;
+ break;
+ case POK_PtPassPointer:
+ DiagID = diag::warn_pt_guarded_pass_pointer;
+ break;
+ case POK_ReturnPointer:
+ DiagID = diag::warn_guarded_return_pointer;
+ break;
+ case POK_PtReturnPointer:
+ DiagID = diag::warn_pt_guarded_return_pointer;
+ break;
}
PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind
<< D
diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c
index 73b4e4798f644..66bfecb8d9c6d 100644
--- a/clang/test/Sema/warn-thread-safety-analysis.c
+++ b/clang/test/Sema/warn-thread-safety-analysis.c
@@ -134,7 +134,9 @@ int main(void) {
Foo_func3(5);
set_value(&a_, 0); // expected-warning{{calling function 'set_value' requires holding mutex 'foo_.mu_' exclusively}}
+ // expected-warning@-1{{passing pointer to variable 'a_' requires holding mutex 'foo_.mu_'}}
get_value(b_); // expected-warning{{calling function 'get_value' requires holding mutex 'foo_.mu_'}}
+ // expected-warning@-1{{passing pointer 'b_' requires holding mutex 'foo_.mu_'}}
mutex_exclusive_lock(foo_.mu_);
set_value(&a_, 1);
mutex_unlock(foo_.mu_);
@@ -180,6 +182,8 @@ 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;
+ 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'}}
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 018d6d1bb258b..976e135d2df43 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -4863,6 +4863,10 @@ class Data {
int dat;
};
+class DataWithAddrOf : public Data {
+public:
+ const Data* operator&() const;
+};
class DataCell {
public:
@@ -4873,6 +4877,7 @@ class DataCell {
};
+void showData(const Data* d);
void showDataCell(const DataCell& dc);
@@ -4944,6 +4949,14 @@ class Foo {
(*datap2_)[0] = 0; // expected-warning {{reading the value pointed to by 'datap2_' requires holding mutex 'mu_'}}
data_(); // expected-warning {{reading variable 'data_' requires holding mutex 'mu_'}}
+
+ // Calls operator&, and does not take 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_'}}
+ showData(&data_ao_); // expected-warning {{reading variable 'data_ao_' requires holding mutex 'mu_'}}
+ (void)&data_; // no warning
+ (void)datap2_; // no warning
+ showData(&data_); // expected-warning {{passing pointer to variable 'data_' requires holding mutex 'mu_'}}
}
// const operator tests
@@ -4955,6 +4968,10 @@ class Foo {
//showDataCell(*datap2_); // xpected-warning {{reading the value pointed to by 'datap2_' requires holding mutex 'mu_'}}
int a = data_[0]; // expected-warning {{reading variable 'data_' requires holding mutex 'mu_'}}
+
+ (void)&data_ao_; // expected-warning {{reading variable 'data_ao_' requires holding mutex 'mu_'}}
+ showData(&data_ao_); // expected-warning {{reading variable 'data_ao_' requires holding mutex 'mu_'}}
+ showData(&data_); // expected-warning {{passing pointer to variable 'data_' requires holding mutex 'mu_'}}
}
private:
@@ -4962,6 +4979,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
@@ -5911,8 +5929,12 @@ T&& mymove(T& f);
void copy(Foo f);
void write1(Foo& f);
void write2(int a, Foo& f);
+void write3(Foo* f);
+void write4(Foo** f);
void read1(const Foo& f);
void read2(int a, const Foo& f);
+void read3(const Foo* f);
+void read4(Foo* const* f);
void destroy(Foo&& f);
void operator/(const Foo& f, const Foo& g);
@@ -5942,19 +5964,24 @@ class Bar {
Foo foo GUARDED_BY(mu);
Foo foo2 GUARDED_BY(mu);
Foo* foop PT_GUARDED_BY(mu);
+ Foo* foop2 GUARDED_BY(mu);
SmartPtr<Foo> foosp PT_GUARDED_BY(mu);
// test methods.
void mwrite1(Foo& f);
void mwrite2(int a, Foo& f);
+ void mwrite3(Foo* f);
void mread1(const Foo& f);
void mread2(int a, const Foo& f);
+ void mread3(const Foo* f);
// static methods
static void smwrite1(Foo& f);
static void smwrite2(int a, Foo& f);
+ static void smwrite3(Foo* f);
static void smread1(const Foo& f);
static void smread2(int a, const Foo& f);
+ static void smread3(const Foo* f);
void operator<<(const Foo& f);
@@ -6017,6 +6044,107 @@ class Bar {
read2(10, *foosp.get());
destroy(mymove(*foosp.get()));
}
+
+ void test_pass_pointer() {
+ (void)&foo; // no warning
+ (void)foop; // no warning
+
+ write3(&foo); // expected-warning {{passing pointer to variable 'foo' requires holding mutex 'mu'}}
+ write3(foop); // expected-warning {{passing pointer 'foop' requires holding mutex 'mu'}}
+ write3(&*foop); // expected-warning {{passing pointer 'foop' requires holding mutex 'mu'}}
+ write4((Foo **)&foo); // expected-warning {{passing pointer to variable 'foo' requires holding mutex 'mu'}}
+ write4(&foop); // no warning
+ read3(&foo); // expected-warning {{passing pointer to variable 'foo' requires holding mutex 'mu'}}
+ read3(foop); // expected-warning {{passing pointer 'foop' requires holding mutex 'mu'}}
+ read3(&*foop); // expected-warning {{passing pointer 'foop' requires holding mutex 'mu'}}
+ read4((Foo **)&foo); // expected-warning {{passing pointer to variable 'foo' requires holding mutex 'mu'}}
+ read4(&foop); // no warning
+ mwrite3(&foo); // expected-warning {{passing pointer to variable 'foo' requires holding mutex 'mu'}}
+ mwrite3(foop); // expected-warning {{passing pointer 'foop' requires holding mutex 'mu'}}
+ mwrite3(&*foop); // expected-warning {{passing pointer 'foop' requires holding mutex 'mu'}}
+ mread3(&foo); // expected-warning {{passing pointer to variable 'foo' requires holding mutex 'mu'}}
+ mread3(foop); // expected-warning {{passing pointer 'foop' requires holding mutex 'mu'}}
+ mread3(&*foop); // expected-warning {{passing pointer 'foop' requires holding mutex 'mu'}}
+ smwrite3(&foo); // expected-warning {{passing pointer to variable 'foo' requires holding mutex 'mu'}}
+ smwrite3(foop); // expected-warning {{passing pointer 'foop' requires holding mutex 'mu'}}
+ smwrite3(&*foop); // expected-warning {{passing pointer 'foop' requires holding mutex 'mu'}}
+ smread3(&foo); // expected-warning {{passing pointer to variable 'foo' requires holding mutex 'mu'}}
+ smread3(foop); // expected-warning {{passing pointer 'foop' requires holding mutex 'mu'}}
+ smread3(&*foop); // expected-warning {{passing pointer 'foop' requires holding mutex 'mu'}}
+
+ write3(foop2); // expected-warning {{reading variable 'foop2' requires holding mutex 'mu'}}
+ write3(&*foop2); // expected-warning {{reading variable 'foop2' requires holding mutex 'mu'}}
+ write4(&foop2); // expected-warning {{passing pointer to variable 'foop2' requires holding mutex 'mu'}}
+ read3(foop2); // expected-warning {{reading variable 'foop2' requires holding mutex 'mu'}}
+ read3(&*foop2); // expected-warning {{reading variable 'foop2' requires holding mutex 'mu'}}
+ read4(&foop2); // expected-warning {{passing pointer to variable 'foop2' requires holding mutex 'mu'}}
+ mwrite3(foop2); // expected-warning {{reading variable 'foop2' requires holding mutex 'mu'}}
+ mwrite3(&*foop2); // expected-warning {{reading variable 'foop2' requires holding mutex 'mu'}}
+ mread3(foop2); // expected-warning {{reading variable 'foop2' requires holding mutex 'mu'}}
+ mread3(&*foop2); // expected-warning {{reading variable 'foop2' requires holding mutex 'mu'}}
+ smwrite3(foop2); // expected-warning {{reading variable 'foop2' requires holding mutex 'mu'}}
+ smwrite3(&*foop2); // expected-warning {{reading variable 'foop2' requires holding mutex 'mu'}}
+ smread3(foop2); // expected-warning {{reading variable 'foop2' requires holding mutex 'mu'}}
+ smread3(&*foop2); // expected-warning {{reading variable 'foop2' requires holding mutex 'mu'}}
+
+ mu.Lock();
+ write3(&foo);
+ write3(foop);
+ write3(&*foop);
+ write3(foop2);
+ write4(&foop2);
+ read3(&foo);
+ read3(foop);
+ read3(&*foop);
+ read3(foop2);
+ read4(&foop2);
+ mwrite3(&foo);
+ mwrite3(foop);
+ mwrite3(&*foop);
+ mwrite3(foop2);
+ mread3(&foo);
+ mread3(foop);
+ mread3(&*foop);
+ mread3(foop2);
+ smwrite3(&foo);
+ smwrite3(foop);
+ smwrite3(&*foop);
+ smwrite3(foop2);
+ smread3(&foo);
+ smread3(foop);
+ smread3(&*foop);
+ smread3(foop2);
+ mu.Unlock();
+
+ mu.ReaderLock();
+ write3(&foo);
+ write3(foop);
+ write3(&*foop);
+ write3(foop2);
+ write4(&foop2);
+ read3(&foo);
+ read3(foop);
+ read3(&*foop);
+ read3(foop2);
+ read4(&foop2);
+ mwrite3(&foo);
+ mwrite3(foop);
+ mwrite3(&*foop);
+ mwrite3(foop2);
+ mread3(&foo);
+ mread3(foop);
+ mread3(&*foop);
+ mread3(foop2);
+ smwrite3(&foo);
+ smwrite3(foop);
+ smwrite3(&*foop);
+ smwrite3(foop2);
+ smread3(&foo);
+ smread3(foop);
+ smread3(&*foop);
+ s...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
melver
force-pushed
the
thread-safety-analysis-2
branch
2 times, most recently
from
13e86fc to
6683e2a
Compare
melver
changed the title
|
(Side-note: I cannot figure out what the canonical way to do "stacked commits" is for LLVM these days, so I'm just doing this the "old" way i.e. plain and simple normal git commits which I will commit separately after PR review.) |
|
FWIW, I'm ready for sending a v2 series to the Linux kernel mailing list: https://git.kernel.org/pub/scm/linux/kernel/git/melver/linux.git/log/?h=cap-analysis/dev But I'd like to sort out this PR first before sending the v2 series. |
|
As an additional data point --- I patched in these commits and have been testing with them on my C codebase and I'm a strong supporter of this independent of the Linux kernel annotation work (which is, of course, also awesome). |
|
Gentle ping. |
|
I need to commit this by end of day Wednesday - if I should wait a little longer, let me know so I can plan around it. |
Sorry, but I'm at C standards meetings this week, so I don't think I'll be able to review it by then. CC @aaronpuchert who maybe can help? |
Thanks, and no worries. I'm also asking someone else here to help review, as we might also want to enable this in more codebases (not just kernel, which was my initial usecase). |
There was a problem hiding this comment.
I only have a few comments about documenting the caveats (no alias analysis).
The actual code changes look very simple and this looks like a clear improvement that would catch many useful cases.
I don't have much experience with this code and would still advise to wait until someone more familiar with the code takes a look. But happy to give a general LGTM after the documentation is improved.
melver
force-pushed
the
thread-safety-analysis-2
branch
from
6683e2a to
7a84fda
Compare
Added the additional test and mentioned the (remaining) lack of alias analysis. |
There was a problem hiding this comment.
I think this looks very good! I just have some minor remarks.
Thanks to @aoates for trying this out, this is always appreciated!
And sorry for the delay.
Correctly analyze expressions where the address of a guarded variable is taken and immediately dereferenced, such as (*(type-specifier *)&x). Previously, such patterns would result in false negatives. Pull Request: llvm#127396
melver
force-pushed
the
thread-safety-analysis-2
branch
from
7a84fda to
59a5de5
Compare
Thanks for the review! I addressed the comments, PTAL. Note, I think for now it might be safer to not enable by default yet, but I've made a note (and hinted at in changelog) that we're planning to default enable in future, so that should give folks enough time to deal with new findings without breaking things just yet. Looking forward to landing this, as I'm sure it'll also uncover new findings in existing codebases using -Wthread-safety already. |
… to guarded variables Introduce `-Wthread-safety-pointer` to warn when passing or returning pointers to guarded variables or guarded data. This is is analogous to `-Wthread-safety-reference`, which performs similar checks for C++ references. Adding checks for pointer passing 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. The feature is planned to be enabled by default under `-Wthread-safety` in the next release cycle. This gives time for early adopters to address new findings. Pull Request: llvm#127396
melver
force-pushed
the
thread-safety-analysis-2
branch
from
59a5de5 to
de996cd
Compare
aaronpuchert
approved these changes
Feb 26, 2025
There was a problem hiding this comment.
Looks good to me, and thanks for the contribution!
Note, I think for now it might be safer to not enable by default yet, but I've made a note (and hinted at in changelog) that we're planning to default enable in future, so that should give folks enough time to deal with new findings without breaking things just yet.
If it's off by default it won't be used by a lot of people, and just the separate flag should be enough to unbreak the build, should it be necessary. But I'm also fine if you'd rather leave it opt-in for a bit and collect experiences before we include it in -Wthread-safety.
melver
added a commit
that referenced
this pull request
Feb 26, 2025
Correctly analyze expressions where the address of a guarded variable is taken and immediately dereferenced, such as (*(type-specifier *)&x). Previously, such patterns would result in false negatives. Pull Request: #127396
melver
added a commit
that referenced
this pull request
Feb 26, 2025
… to guarded variables Introduce `-Wthread-safety-pointer` to warn when passing or returning pointers to guarded variables or guarded data. This is is analogous to `-Wthread-safety-reference`, which performs similar checks for C++ references. Adding checks for pointer passing 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. The feature is planned to be enabled by default under `-Wthread-safety` in the next release cycle. This gives time for early adopters to address new findings. Pull Request: #127396
|
Committed!
Thanks for your review!
We'll be trying this on some internal codebases as well, and we can't yet force the cleanups to happen right away. But I expect in ~3-6 months we can flip it to default on. |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Feb 26, 2025
…ference Correctly analyze expressions where the address of a guarded variable is taken and immediately dereferenced, such as (*(type-specifier *)&x). Previously, such patterns would result in false negatives. Pull Request: llvm/llvm-project#127396
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Feb 26, 2025
…ng pointers to guarded variables Introduce `-Wthread-safety-pointer` to warn when passing or returning pointers to guarded variables or guarded data. This is is analogous to `-Wthread-safety-reference`, which performs similar checks for C++ references. Adding checks for pointer passing 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. The feature is planned to be enabled by default under `-Wthread-safety` in the next release cycle. This gives time for early adopters to address new findings. Pull Request: llvm/llvm-project#127396
melver
added a commit
to google/kernel-sanitizers
that referenced
this pull request
Feb 27, 2025
…ty analysis Capability analysis is a C language extension, which enables statically checking that user-definable "capabilities" are acquired and released where required. An obvious application is lock-safety checking for the kernel's various synchronization primitives (each of which represents a "capability"), and checking that locking rules are not violated. Clang originally called the feature "Thread Safety Analysis" [1], with some terminology still using the thread-safety-analysis-only names. This was later changed and the feature became more flexible, gaining the ability to define custom "capabilities". Its foundations can be found in "capability systems", used to specify the permissibility of operations to depend on some capability being held (or not held). [1] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html [2] https://www.cs.cornell.edu/talc/papers/capabilities.pdf Because the feature is not just able to express capabilities related to synchronization primitives, the naming chosen for the kernel departs from Clang's initial "Thread Safety" nomenclature and refers to the feature as "Capability Analysis" to avoid confusion. The implementation still makes references to the older terminology in some places, such as `-Wthread-safety` being the warning enabled option that also still appears in diagnostic messages. See more details in the kernel-doc documentation added in this and the subsequent changes. A Clang version that supports -Wthread-safety-pointer is recommended, but not required: llvm/llvm-project#127396 Signed-off-by: Marco Elver <[email protected]> --- v2: * New -Wthread-safety feature rename to -Wthread-safety-pointer (was -Wthread-safety-addressof). * Introduce __capability_unsafe() function attribute. * Rename __var_guarded_by to simply __guarded_by. The initial idea was to be explicit if the variable or pointed-to data is guarded by, but having a shorter attribute name is likely better long-term. * Rename __ref_guarded_by to __pt_guarded_by (pointed-to guarded by).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
2 dependent commits:
Prior discussion: #123063