aaronpuchert added a comment.
Tried this on our code base, and the number of new warnings seems acceptable.
I'll still need to look through them in more detail, but there is one
suspicious warning that boils down to this:
> cat reference-bug.cpp
struct __attribute__((capability("mutex"))) Mutex {} mu;
int* p __attribute__((pt_guarded_by(mu)));
int& f() {
return *p;
}
> clang-16 -fsyntax-only -Wthread-safety-analysis reference-bug.cpp
> clang-16-D153131 -fsyntax-only -Wthread-safety-analysis reference-bug.cpp
reference-bug.cpp:5:11: warning: writing the value pointed to by 'p' requires
holding mutex 'mu' exclusively [-Wthread-safety-analysis]
return *p;
^
1 warning generated.
That we're warning here is correct, but the warning message is a bit off (we're
not quite writing here), and it's under `-Wthread-safety-analysis` instead of
`-Wthread-safety-reference`.
================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1046
def ThreadSafetyPrecise : DiagGroup<"thread-safety-precise">;
def ThreadSafetyReference : DiagGroup<"thread-safety-reference">;
+def ThreadSafetyReturn : DiagGroup<"thread-safety-return">;
----------------
courbet wrote:
> aaronpuchert wrote:
> > courbet wrote:
> > > aaronpuchert wrote:
> > > > Why not under `-Wthread-safety-reference`, as it's return-by-reference
> > > > that you're warning on? This seems too small for a separate flag to me.
> > > The main reason it so that we provide a soft transition period for users:
> > > If we put that in `-Wthread-safety-reference`, we'll start breaking
> > > compile for people who use `-Werror`, while a separate flag allows a
> > > transition period where people opt into the new feature.
> > Transition flags can end up resulting in more churn, assuming that we
> > eventually want to put this under `-Wthread-safety-reference`, because then
> > you have two categories of users:
> > * those that opted in will eventually have to remove the flag again, and
> > * those that didn't will get hard errors on updating the compiler at that
> > point.
> > Of course you might argue that the latter case can be prevented by
> > carefully reading the release notes, but we know how often that happens.
> >
> > I'd argue that if you're using `-Wthread-safety-reference`, you're already
> > opting into warnings on escaping references, and not warning on `return` is
> > a false negative.
> >
> > A separate flag would make sense to me if we want to keep it, for example
> > because this produces a substantial amount of false positives under some
> > circumstances. Did you try this on a larger code base that's using the
> > annotations? I could try it on our code, and maybe we can get some Googler
> > to test it on theirs, which is also heavily using Thread Safety Analysis.
> > (Though I'm not sure whether they use `-Wthread-safety-reference`.)
> I don't have a strong opinion for where the warning should go. We are indeed
> using `-Wthread-safety-reference`, though we're not enabling -Werror on
> these, so adding more warnings is fine for us.
>
> I've run the check on a small sample of our codebase (which I don;t claim to
> be representative, I can do a larger analysis if needed). The warnings are
> more or less evenly split between missing annotations and actual bugs. I
> don't think any of the things I've seen qualify as false positives.
>
> Among the missing annotations, most of the warnings are missing
> `ABSL_EXCLUSIVE_LOCKS_REQUIRED` on the function. In a small number of cases,
> the pattern is that a variable is lazily initialized under a lock and then
> returned by reference:
>
> ```
> struct LazyImmutableThing {
> const Thing& Get() {
> {
> MutexLock lock(&mutex_);
> thing_->Initialize();
> }
>
> return thing_;
> }
>
> Mutex mutex_;
> Thing thing_ GUARDED_BY(mutex_);
> };
> ```
>
> I consider this to be a missing annotation as the returned value is
> dynamically immutable, the proper fix would be `return
> TS_UNCHECKED_READ(thing_)`.
>
>
> Most actual bugs are along the lines of:
>
> ```
> struct S {
> T& Get() const {
> MutexLock lock(&mutex_);
> return obj_;
> }
>
> Mutex mutex_;
> T obj_ GUARDED_BY(mutex_);
> };
> ```
>
> though some are missing the guard altogether (`T& Get() const { return obj_;
> }`).
>
> There are a few possible fixes. In rough order of occurrence:
> - Return by value as the copy is not too expensive and memory ordering does
> not matter.
> - Let the caller take the lock and annotate with
> `ABSL_EXCLUSIVE_LOCKS_REQUIRED` when the `Get` method is not called too often.
> - Let `Get` take a callback and process the value under the lock instead of
> returning it (when ordering matters).
>
> In a small number of cases, the pattern is that a variable is lazily
> initialized under a lock and then returned by reference:
I wonder why that's safe, is the initialization guarded to happen only once?
Some kind of double-checked locking pattern perhaps? Otherwise it seems that
reads could happen in parallel to writes. If it's a checked initialization,
then I think the proper way to model this is:
* The initialization acquires a lock to exclude other initializations running
in parallel. Reads cannot happen, because the reference has not yet escaped.
* After initialization, we essentially acquire an implicit shared lock. This is
not tracked as a proper lock, but it doesn't need to: there are no more writes
until the end of lifetime, so nobody will acquire another exclusive lock.
One could model this by creating a mutex wrapper that can be locked once in
exclusive mode, and after that hands out shared locks to everybody who wants
one without keeping track. (As this is slightly off-topic, we don't need to
discuss this here though.)
Other than than, this matches what I'm seeing in our code.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153131/new/
https://reviews.llvm.org/D153131
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
