courbet added inline comments.
================
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">;
----------------
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).
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
