Skip to content

Commit de10e44

Browse files
melvermelver
committed
Thread Safety Analysis: Support warning on passing/returning 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: #127396
1 parent 3c8c0d4 commit de10e44

File tree

9 files changed

+276
-13
lines changed

9 files changed

+276
-13
lines changed

‎clang/docs/ReleaseNotes.rst‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,12 @@ Improvements to Clang's diagnostics
199199
- Diagnostics on chained comparisons (``a < b < c``) are now an error by default. This can be disabled with
200200
``-Wno-error=parentheses``.
201201

202+
- The :doc:`ThreadSafetyAnalysis` now supports ``-Wthread-safety-pointer``,
203+
which enables warning on passing or returning pointers to guarded variables
204+
as function arguments or return value respectively. Note that
205+
:doc:`ThreadSafetyAnalysis` still does not perform alias analysis. The
206+
feature will be default-enabled with ``-Wthread-safety`` in a future release.
207+
202208
Improvements to Clang's time-trace
203209
----------------------------------
204210

‎clang/docs/ThreadSafetyAnalysis.rst‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -515,8 +515,12 @@ Warning flags
515515
+ ``-Wthread-safety-analysis``: The core analysis.
516516
+ ``-Wthread-safety-precise``: Requires that mutex expressions match precisely.
517517
This warning can be disabled for code which has a lot of aliases.
518-
+ ``-Wthread-safety-reference``: Checks when guarded members are passed by reference.
518+
+ ``-Wthread-safety-reference``: Checks when guarded members are passed or
519+
returned by reference.
519520

521+
* ``-Wthread-safety-pointer``: Checks when passing or returning pointers to
522+
guarded variables, or pointers to guarded data, as function argument or
523+
return value respectively.
520524

521525
:ref:`negative` are an experimental feature, which are enabled with:
522526

‎clang/include/clang/Analysis/Analyses/ThreadSafety.h‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,18 @@ enum ProtectedOperationKind {
5454

5555
/// Returning a pt-guarded variable by reference.
5656
POK_PtReturnByRef,
57+
58+
/// Passing pointer to a guarded variable.
59+
POK_PassPointer,
60+
61+
/// Passing a pt-guarded pointer.
62+
POK_PtPassPointer,
63+
64+
/// Returning pointer to a guarded variable.
65+
POK_ReturnPointer,
66+
67+
/// Returning a pt-guarded pointer.
68+
POK_PtReturnPointer,
5769
};
5870

5971
/// This enum distinguishes between different kinds of lock actions. For

‎clang/include/clang/Basic/DiagnosticGroups.td‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1156,6 +1156,7 @@ def ThreadSafetyPrecise : DiagGroup<"thread-safety-precise">;
11561156
def ThreadSafetyReferenceReturn : DiagGroup<"thread-safety-reference-return">;
11571157
def ThreadSafetyReference : DiagGroup<"thread-safety-reference",
11581158
[ThreadSafetyReferenceReturn]>;
1159+
def ThreadSafetyPointer : DiagGroup<"thread-safety-pointer">;
11591160
def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">;
11601161
def ThreadSafety : DiagGroup<"thread-safety",
11611162
[ThreadSafetyAttributes,

‎clang/include/clang/Basic/DiagnosticSemaKinds.td‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4126,6 +4126,24 @@ def warn_pt_guarded_return_by_reference : Warning<
41264126
"%select{'%2'|'%2' exclusively}3">,
41274127
InGroup<ThreadSafetyReferenceReturn>, DefaultIgnore;
41284128

4129+
// Thread safety warnings on pointer passing
4130+
def warn_guarded_pass_pointer : Warning<
4131+
"passing pointer to variable %1 requires holding %0 "
4132+
"%select{'%2'|'%2' exclusively}3">,
4133+
InGroup<ThreadSafetyPointer>, DefaultIgnore;
4134+
def warn_pt_guarded_pass_pointer : Warning<
4135+
"passing pointer %1 requires holding %0 "
4136+
"%select{'%2'|'%2' exclusively}3">,
4137+
InGroup<ThreadSafetyPointer>, DefaultIgnore;
4138+
def warn_guarded_return_pointer : Warning<
4139+
"returning pointer to variable %1 requires holding %0 "
4140+
"%select{'%2'|'%2' exclusively}3">,
4141+
InGroup<ThreadSafetyPointer>, DefaultIgnore;
4142+
def warn_pt_guarded_return_pointer : Warning<
4143+
"returning pointer %1 requires holding %0 "
4144+
"%select{'%2'|'%2' exclusively}3">,
4145+
InGroup<ThreadSafetyPointer>, DefaultIgnore;
4146+
41294147
// Imprecise thread safety warnings
41304148
def warn_variable_requires_lock : Warning<
41314149
"%select{reading|writing}3 variable %1 requires holding %0 "

‎clang/lib/Analysis/ThreadSafety.cpp‎

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1794,11 +1794,24 @@ void ThreadSafetyAnalyzer::checkPtAccess(const FactSet &FSet, const Expr *Exp,
17941794
}
17951795
}
17961796

1797-
// Pass by reference warnings are under a different flag.
1797+
// Pass by reference/pointer warnings are under a different flag.
17981798
ProtectedOperationKind PtPOK = POK_VarDereference;
1799-
if (POK == POK_PassByRef) PtPOK = POK_PtPassByRef;
1800-
if (POK == POK_ReturnByRef)
1799+
switch (POK) {
1800+
case POK_PassByRef:
1801+
PtPOK = POK_PtPassByRef;
1802+
break;
1803+
case POK_ReturnByRef:
18011804
PtPOK = POK_PtReturnByRef;
1805+
break;
1806+
case POK_PassPointer:
1807+
PtPOK = POK_PtPassPointer;
1808+
break;
1809+
case POK_ReturnPointer:
1810+
PtPOK = POK_PtReturnPointer;
1811+
break;
1812+
default:
1813+
break;
1814+
}
18021815

18031816
const ValueDecl *D = getValueDecl(Exp);
18041817
if (!D || !D->hasAttrs())
@@ -2145,6 +2158,8 @@ void BuildLockset::examineArguments(const FunctionDecl *FD,
21452158
QualType Qt = (*Param)->getType();
21462159
if (Qt->isReferenceType())
21472160
checkAccess(*Arg, AK_Read, POK_PassByRef);
2161+
else if (Qt->isPointerType())
2162+
checkPtAccess(*Arg, AK_Read, POK_PassPointer);
21482163
}
21492164
}
21502165

@@ -2286,15 +2301,20 @@ void BuildLockset::VisitReturnStmt(const ReturnStmt *S) {
22862301
if (!RetVal)
22872302
return;
22882303

2289-
// If returning by reference, check that the function requires the appropriate
2290-
// capabilities.
2304+
// If returning by reference or pointer, check that the function requires the
2305+
// appropriate capabilities.
22912306
const QualType ReturnType =
22922307
Analyzer->CurrentFunction->getReturnType().getCanonicalType();
22932308
if (ReturnType->isLValueReferenceType()) {
22942309
Analyzer->checkAccess(
22952310
FunctionExitFSet, RetVal,
22962311
ReturnType->getPointeeType().isConstQualified() ? AK_Read : AK_Written,
22972312
POK_ReturnByRef);
2313+
} else if (ReturnType->isPointerType()) {
2314+
Analyzer->checkPtAccess(
2315+
FunctionExitFSet, RetVal,
2316+
ReturnType->getPointeeType().isConstQualified() ? AK_Read : AK_Written,
2317+
POK_ReturnPointer);
22982318
}
22992319
}
23002320

‎clang/lib/Sema/AnalysisBasedWarnings.cpp‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1977,6 +1977,18 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
19771977
case POK_PtReturnByRef:
19781978
DiagID = diag::warn_pt_guarded_return_by_reference;
19791979
break;
1980+
case POK_PassPointer:
1981+
DiagID = diag::warn_guarded_pass_pointer;
1982+
break;
1983+
case POK_PtPassPointer:
1984+
DiagID = diag::warn_pt_guarded_pass_pointer;
1985+
break;
1986+
case POK_ReturnPointer:
1987+
DiagID = diag::warn_guarded_return_pointer;
1988+
break;
1989+
case POK_PtReturnPointer:
1990+
DiagID = diag::warn_pt_guarded_return_pointer;
1991+
break;
19801992
}
19811993
PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind
19821994
<< D
@@ -2013,6 +2025,18 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
20132025
case POK_PtReturnByRef:
20142026
DiagID = diag::warn_pt_guarded_return_by_reference;
20152027
break;
2028+
case POK_PassPointer:
2029+
DiagID = diag::warn_guarded_pass_pointer;
2030+
break;
2031+
case POK_PtPassPointer:
2032+
DiagID = diag::warn_pt_guarded_pass_pointer;
2033+
break;
2034+
case POK_ReturnPointer:
2035+
DiagID = diag::warn_guarded_return_pointer;
2036+
break;
2037+
case POK_PtReturnPointer:
2038+
DiagID = diag::warn_pt_guarded_return_pointer;
2039+
break;
20162040
}
20172041
PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind
20182042
<< D

‎clang/test/Sema/warn-thread-safety-analysis.c‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta %s
2-
// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-beta -fexperimental-late-parse-attributes -DLATE_PARSING %s
1+
// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-pointer -Wthread-safety-beta %s
2+
// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wthread-safety-pointer -Wthread-safety-beta -fexperimental-late-parse-attributes -DLATE_PARSING %s
33

44
#define LOCKABLE __attribute__ ((lockable))
55
#define SCOPED_LOCKABLE __attribute__ ((scoped_lockable))
@@ -137,7 +137,9 @@ int main(void) {
137137
Foo_func3(5);
138138

139139
set_value(&a_, 0); // expected-warning{{calling function 'set_value' requires holding mutex 'foo_.mu_' exclusively}}
140+
// expected-warning@-1{{passing pointer to variable 'a_' requires holding mutex 'foo_.mu_'}}
140141
get_value(b_); // expected-warning{{calling function 'get_value' requires holding mutex 'foo_.mu_'}}
142+
// expected-warning@-1{{passing pointer 'b_' requires holding mutex 'foo_.mu_'}}
141143
mutex_exclusive_lock(foo_.mu_);
142144
set_value(&a_, 1);
143145
mutex_unlock(foo_.mu_);
@@ -199,6 +201,8 @@ int main(void) {
199201
#ifdef LATE_PARSING
200202
late_parsing.a_value_defined_before = 1; // expected-warning{{writing variable 'a_value_defined_before' requires holding mutex 'a_mutex_defined_late' exclusively}}
201203
late_parsing.a_ptr_defined_before = 0;
204+
set_value(&late_parsing.a_value_defined_before, 0); // expected-warning{{calling function 'set_value' requires holding mutex 'foo_.mu_' exclusively}}
205+
// expected-warning@-1{{passing pointer to variable 'a_value_defined_before' requires holding mutex 'a_mutex_defined_late'}}
202206
mutex_exclusive_lock(late_parsing.a_mutex_defined_late);
203207
mutex_exclusive_lock(late_parsing.a_mutex_defined_early); // expected-warning{{mutex 'a_mutex_defined_early' must be acquired before 'a_mutex_defined_late'}}
204208
mutex_exclusive_unlock(late_parsing.a_mutex_defined_early);

0 commit comments

Comments
 (0)