[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
https://github.com/melver edited https://github.com/llvm/llvm-project/pull/123063 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
bwendling wrote: Our current `counted_by` implementation is for flexible array members only, which are always at the end of the struct (or supposed to be). When we start applying them to pointers in structs (coming soon), we'll need to use that flag. However, I think that Apple is __finally__ starting to release some of their features, and I hope that the flag will go away as it'll always be on. https://github.com/llvm/llvm-project/pull/123063 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
melver wrote: > Doesn't `counted_by` have the same requirements? If not, why does guarded_by? It does, AFAIK. > Sure, I can imagine where there might be pushback, but perhaps its worthwhile > to see how far you can get with either marking structs that don't need > `-fexperimental-late-parse-attributes`, or propose such member declaration > re-orderings. At least that would let you land something upstream and begin > protecting the kernel/finding bugs. Fair point, in the worst case individual TUs could still add the flag if needed. https://github.com/llvm/llvm-project/pull/123063 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
nickdesaulniers wrote: > > Doesn't `counted_by` have the same requirements? If not, why does > > guarded_by? > > It does, AFAIK. But counted_by _isn't_ using `-fexperimental-late-parse-attributes` IIUC. Do they have the same problem then of needing to reorder member declarations? cc @kees @isanbard @bwendling https://github.com/llvm/llvm-project/pull/123063 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
melver wrote: > Can any of the members in the structs be reorganized to put the mutex member > declaration BEFORE the members they guard? Probably not always, but perhaps > that's possible for most structures? It's an option I considered, but I can already hear "what is this crap ... NACK". In many cases it might be possible, but where data layout or cacheline organization is important for performance, definitely not an option. https://github.com/llvm/llvm-project/pull/123063 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
nickdesaulniers wrote: Looks like the kernel patches [use](https://github.com/google/kernel-sanitizers/commit/81883e1fa377d866c288192b6eb8334bcf10f38f) `-fexperimental-late-parse-attributes`? :( https://github.com/llvm/llvm-project/pull/123063 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
nickdesaulniers wrote: Can any of the members in the structs be reorganized to put the mutex member declaration BEFORE the members they guard? Probably not always, but perhaps that's possible for most structures? https://github.com/llvm/llvm-project/pull/123063 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
melver wrote: > Looks like the kernel patches > [use](https://github.com/google/kernel-sanitizers/commit/81883e1fa377d866c288192b6eb8334bcf10f38f) > `-fexperimental-late-parse-attributes`? :( Yeah, no way around it if we want to have guarded_by in structs work properly. If the problem is cost, still planning to allow it to be disabled via Kconfig option. https://github.com/llvm/llvm-project/pull/123063 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
melver wrote: FWIW, the Linux kernel integration (draft, WIP) currently lives here: https://github.com/google/kernel-sanitizers/tree/cap-analysis It currently enables -Wthread-safety-addressof if available. Thus far, I have not found any false positives due to -Wthread-safety-addressof in the 2 subsystems I converted over (more to follow). And I want to re-iterate that without -Wthread-safety-addressof, the feature's coverage is significantly reduced, and I predict it to be one of the first complaints in later review. Kindly take another look. cc @bvanassche https://github.com/llvm/llvm-project/pull/123063 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
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
Date: Wed, 15 Jan 2025 13:23:14 +0100
Subject: [PATCH 1/3] 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/DiagnosticSemaKin
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
https://github.com/melver edited https://github.com/llvm/llvm-project/pull/123063 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
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
Date: Wed, 15 Jan 2025 13:23:14 +0100
Subject: [PATCH 1/3] 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/DiagnosticSemaKin
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
melver wrote:
> We had a discussion on https://reviews.llvm.org/D52395 that might be relevant
> here. To quote @delesley:
>
> > When you pass an object to a function, either by pointer or by reference,
> > no actual load from memory has yet occurred. Thus, there is a real risk of
> > false positives; what you are saying is that the function _might_ read or
> > write from protected memory, not that it actually will.
Right. Though in the absence of better pointer tracking / alias analysis, I
believe there's no way to avoid resulting false positives with pointers. It's
something that a user of Wthread-safety-addressof would opt-in explicitly -
documentation needs elaboration on this perhaps.
> Another aspect is that we don't check if the lock is still held when we
> dereference the pointer, so there are also false negatives:
>
> ```c++
> Mutex mu;
> int x GUARDED_BY(mu);
>
> void f() {
> mu.Lock();
> int *p = &x;
> mu.Unlock();
> *p = 0;
> }
> ```
Good point - though I'd prefer few false negatives + false positives over no
checks at all. It's a tradeoff, and therefore we definitely shouldn't enable
Wthread-safety-addressof by default.
> You use the analogy of C++ references. Interestingly, we don't warn on
> "taking a reference", partly because no such thing exists. (You can only
> initialize something of reference type with an expression.) We warn on
> passing a variable by reference _to a function_, or in other words,
> initializing a parameter of reference type with a guarded variable. (Since
> https://reviews.llvm.org/D153131, we also warn on returning a reference when
> the lock is no longer held after return. Note that the initialization of the
> reference might still happen under lock!)
>
> However, we also track references in a way that pointers are not tracked. The
> reason is probably that references are immutable (that is, `T&` is
> semantically the same thing as `T* const`).
>
> ```c++
> void g() {
> mu.Lock();
> int &ref = x;
> mu.Unlock();
> ref = 0; // warning: writing variable 'x' requires holding mutex 'mu'
> exclusively
> }
> ```
>
> If you "take the reference" outside of the lock and do the assignment inside,
> there is no warning. Because the assignment is what needs the lock, not
> taking the address.
Good points, and I missed that reference handling is much better in this regard.
> However, with functions it's somewhat reasonable to assume that the function
> accesses through the pointer, and that the pointer doesn't escape. (This is
> of course not always true, but often enough.) So I wonder whether we should
> restrict this to pointers passed as function arguments.
That's a reasonable option to make it more conservative, although I'm not sure
it's necessary (yet).
> But if the number of false positives is manageable, we can also leave it like
> you implemented it. Did you try it out on some code base that uses thread
> safety annotations?
I'm working on bringing Wthread-safety to the Linux kernel. The strategy chosen
is to convert one subsystem (or single TU) at a time, based on an opt-in basis.
Since Linux already has a rather special dialect of C, those that would choose
to opt in their TUs would opt into another variant of Linux's C dialect (one
with Wthread-safety enabled).
My experience in converting a subsystem I maintain is that applying
Wthread-safety already requires small refactors to avoid false positives, and
I'm not opposed to add additional constraints via Wthread-safety-addressof.
Without additional checking of "obtaining address of guarded variables", I find
that Wthread-safety barely provides benefits, as passing pointers to
datastructures is so common.
Resolving this will improve the usefulness, and ultimately the chances of us
succeeding to upstream this to the Linux kernel.
I don't think we will ever convert 100% of the kernel to use Wthread-safety,
but the plan is that willing subsystem maintainers opt in and deal with
warnings produced by Wthread-safety. Having Wthread-safety-addressof will add
additional coverage. For cases where it produces too many false positives, I
intend to add a simple option that can enable/disable addressof checking for
individual TUs. Or if almost all places where obtaining the address of a
particular lock-guarded variable is _not_ actually under a lock (but still
correct), leaving off `guarded_by` might simply be the right choice.
https://github.com/llvm/llvm-project/pull/123063
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
aaronpuchert wrote:
We had a discussion on https://reviews.llvm.org/D52395 that might be relevant
here. To quote @delesley:
> When you pass an object to a function, either by pointer or by reference, no
> actual load from memory has yet occurred. Thus, there is a real risk of false
> positives; what you are saying is that the function *might* read or write
> from protected memory, not that it actually will.
Another aspect is that we don't check if the lock is still held when we
dereference the pointer, so there are also false negatives:
```c++
Mutex mu;
int x GUARDED_BY(mu);
void f() {
mu.Lock();
int *p = &x;
mu.Unlock();
*p = 0;
}
```
You use the analogy of C++ references. Interestingly, we don't warn on "taking
a reference", partly because no such thing exists. (You can only initialize
something of reference type with an expression.) We warn on passing a variable
by reference *to a function*, or in other words, initializing a parameter of
reference type with a guarded variable. (Since
https://reviews.llvm.org/D153131, we also warn on returning a reference when
the lock is no longer held after return. Note that the initialization of the
reference might still happen under lock!)
However, we also track references in a way that pointers are not tracked. The
reason is probably that references are immutable (that is, `T&` is semantically
the same thing as `T* const`).
```c++
void g() {
mu.Lock();
int &ref = x;
mu.Unlock();
ref = 0; // warning: writing variable 'x' requires holding mutex 'mu'
exclusively
}
```
If you "take the reference" outside of the lock and do the assignment inside,
there is no warning. Because the assignment is what needs the lock, not taking
the address.
However, with functions it's somewhat reasonable to assume that the function
accesses through the pointer, and that the pointer doesn't escape. (This is of
course not always true, but often enough.) So I wonder whether we should
restrict this to pointers passed as function arguments.
But if the number of false positives is manageable, we can also leave it like
you implemented it. Did you try it out on some code base that uses thread
safety annotations?
https://github.com/llvm/llvm-project/pull/123063
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
melver wrote: For additional background: I'm trying to work out how to bring -Wthread-safety to the Linux kernel. Since -fexperimental-late-parse-attributes made the feature practical for complex C structs, I started to look at a strategy to enable the feature. The current strategy is to create an opt-in mechanism (per subsystem), and since the constraints imposed by Wthread-safety already require subtly restructuring most code that would opt-in, I think having Wthread-safety-addressof part of the initial constraints imposed by Wthread-safety would be very helpful. In converting one subsystem I maintain, I was disappointed that things like `list_add(¬e, &guarded_list)` aren't caught. In general, I believe that the situation is analogous to C++ reference passing, and thus -Wthread-safety-addressof would be the right choice for new C projects. https://github.com/llvm/llvm-project/pull/123063 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
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
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.t
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff 87750c9de4b7bd71539bfadd61c10317235da138
60e3b1d7c2dd7f9c070b25c25ff546adb1a68143 --extensions h,c,cpp --
clang/include/clang/Analysis/Analyses/ThreadSafety.h
clang/lib/Analysis/ThreadSafety.cpp clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/test/Sema/warn-thread-safety-analysis.c
clang/test/SemaCXX/warn-thread-safety-analysis.cpp
``
View the diff from clang-format here.
``diff
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp
b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index bb009de228..ace36cbecb 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1985,18 +1985,18 @@ class ThreadSafetyReporter : public
clang::threadSafety::ThreadSafetyHandler {
AccessKind AK, SourceLocation Loc) override {
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;
+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));
``
https://github.com/llvm/llvm-project/pull/123063
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Marco Elver (melver)
Changes
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/123063.diff
9 Files Affected:
- (modified) clang/docs/ReleaseNotes.rst (+3)
- (modified) clang/docs/ThreadSafetyAnalysis.rst (+10-2)
- (modified) clang/include/clang/Analysis/Analyses/ThreadSafety.h (+3)
- (modified) clang/include/clang/Basic/DiagnosticGroups.td (+1)
- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+8)
- (modified) clang/lib/Analysis/ThreadSafety.cpp (+3)
- (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+21-5)
- (modified) clang/test/Sema/warn-thread-safety-analysis.c (+11)
- (modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+42-2)
``diff
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
[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)
https://github.com/melver created
https://github.com/llvm/llvm-project/pull/123063
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.
>From 60e3b1d7c2dd7f9c070b25c25ff546adb1a68143 Mon Sep 17 00:00:00 2001
From: Marco Elver
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 loc
