[clang analysis][thread-safety] Handle return-by-reference...

[courbet]

Summary

...of guarded variables, when the function is not marked as requiring locks:

class Return {
  Mutex mu;
  Foo foo GUARDED_BY(mu);

  Foo &returns_ref_locked() {
    MutexLock lock(&mu);
    return foo;  // BAD
  }

  Foo &returns_ref_locks_required() SHARED_LOCKS_REQUIRED(mu) {
    return foo;  // OK
  }
};

This is implemented as -Wthread-safety-return and not part of
-Wthread-safety for now.

[aaronpuchert]

Thanks for working on this! Someone recently pointed out to me that we have a gap there.

clang/include/clang/Basic/DiagnosticGroups.td

1046 ↗

(On Diff #532116)

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.

clang/include/clang/Basic/DiagnosticSemaKinds.td

3809

Or do you expect more warnings on return?

clang/lib/Analysis/ThreadSafety.cpp

2157–2159

Wouldn't it be more straightforward to check the actual return type? We have the FunctionDecl and could store it in ThreadSafetyAnalyzer instead of CurrentMethod.

2160

You're presumably collecting them because automatic destructor calls are after return in the CFG, right?

If that's the case, can't we immediately check against the declared exit set? It should be known before we walk the CFG, unless I'm missing something.

[aaronpuchert]

clang/lib/Analysis/ThreadSafety.cpp

2163

Also wondering why we're doing this—no other visitor function seems to bother the VisitorBase = ConstStmtVisitor<BuildLockset>. Are these not just empty fallbacks?

[courbet]

Address review comments

[courbet]

Thanks.

clang/include/clang/Basic/DiagnosticGroups.td

1046 ↗

(On Diff #532116)

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.

clang/include/clang/Basic/DiagnosticSemaKinds.td

3809

We could do pointers too, but arguably pointers and references are the same.

clang/lib/Analysis/ThreadSafety.cpp

2157–2159

Good point. I've also added better checking and diagnostics for const (shared) vs mutable (exclusive) locks, with more tests.

2160

You're presumably collecting them because automatic destructor calls are after return in the CFG, right?

Exactly.

If that's the case, can't we immediately check against the declared exit set? It should be known before we walk the CFG, unless I'm missing something.

Given how the code was written I was under the impression that we only knew the entry set after walking the whole CFG (we're getting ExpectedExitSet after we walk the CFG). But now I see that we're actually adressing the entry blok beforehand. Thanks for the suggestion, this makes the code much simpler indeed !

2163

The base code is hard to read because i't full of macros, but it looks like it't probably empty indeed - done.

[courbet]

format

[aaronpuchert]

clang/include/clang/Basic/DiagnosticGroups.td

1046 ↗

(On Diff #532116)

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.)

[courbet]

clang/include/clang/Basic/DiagnosticGroups.td

1046 ↗

(On Diff #532116)

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).

[courbet]

Put the new warnings in -Wtread-safety-reference

[aaronpuchert]

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.

clang/include/clang/Basic/DiagnosticGroups.td

1046 ↗

(On Diff #532116)

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.

[courbet]

Add a specific message for return-by-ref of pt_guarded_by variable.

[courbet]

In D153131#4456019, @aaronpuchert wrote:

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.

Right. I was relying on the fallback "imprecise" warning for this one. I added a pt_garded value just like for pass_by_value and added a test.

[courbet]

clang/include/clang/Basic/DiagnosticGroups.td

1046 ↗

(On Diff #532116)

I wonder why that's safe, is the initialization guarded to happen only once? Some kind of double-checked locking pattern perhaps?

Yes, it looks like this:

const T& get() const {
    if (!value_set_.load(std::memory_order_acquire)) {
      MutexLock lock(&lock_);
      if (!value_set_.load(std::memory_order_relaxed)) {
        value_ = ComputeValue();
        value_set_.store(true, std::memory_order_release);
      }
    }
    return value_;
  }

I ended up silencing the return with a comment:

// `value_` is set once an for all, it won't change after this function returns.
 return ABSL_TS_UNCHECKED_READ(value_);

I agree that this is not very principled, but it is simple :)

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.

This is a cool idea. If I understand correctly, it does mean that the *caller* of get has to grab a untracked shared lock ?

[aaronpuchert]

Sorry for letting this collect dust. I think it's a valuable addition, and looks pretty good, except that I think we should use the expected exit set instead of the entry set. These can be legitimately different for appropriately annotated functions, i.e. with acquire_(shared_)capability or release_(shared_)capability.

Apart from the bug mentioned earlier, which should now be fixed, this looks good on our code base. I have to admit a separate flag could be nice for migration, but it should be included by default in -Wthread-safety-reference. But I'm not sure if we need it.

clang/include/clang/Basic/DiagnosticGroups.td

1046 ↗

(On Diff #532116)

This looks like the Double-checked locking pattern. I've come to the conclusion that guarded_by annotations are not appropriate for them (just copying what I wrote to someone internally):

• The initialization flag (here value_set_) is read before acquiring the mutex, which means it can't be protected by the mutex.
• The content (here value_), if already initialized, is also read without acquiring the mutex, so it can't be protected by it. The content, if only readable, is "protected" by atomic ordering: the initialization must have a release barrier on (or before) writing the initialization flag, which synchronizes with an acquire barrier on (or after) the initial read of the initialization flag. If the content is writable, it will need to be synchronized on its own.
• The mutex might protect data being used in the initialization. Other than that it's just a performance optimization: having multiple threads initialize the same object would be wasteful. Especially since the pattern makes only sense for expensive initialization. (Expensive either in time or memory consumption.)
• Lastly, this pattern is usually used in a single function, so there is no risk of forgetting the mutex.

clang/lib/Analysis/ThreadSafety.cpp

44

I wonder where we're using that, is this the leftover of an earlier version?

1248

1546–1547

Shouldn't it be the (expected) exit set if we're talking about return? Also I'd suggest (Function)EntryFSet (or with Exit).

2298

Maybe it makes sense to keep an assertion here like assert(*SortedGraph->begin() == &CFGraph->getEntry());.

2308

2500–2518

Here we build the ExpectedExitSet. You might have to move this if we're using it earlier.

clang/test/SemaCXX/warn-thread-safety-analysis.cpp

5630

For the entry/exit set issue, can you add a function that acquires a mutex (and doesn't release it), returning something protected by the mutex? And maybe one that releases but doesn't acquire.

[courbet]

• Check return values against the exit set rather than the entry set, add unit tests.
• Address other cosmetic review comments

[courbet]

I think we should use the expected exit set instead of the entry set.

Indeed, I've added a few tests to check this. Let me know if you see any other tests that might be valuable.

[aaronpuchert]

Looks good to me, but let's wait a few days in case @aaron.ballman has anything to add.

@rupprecht, in case you're still doing integration at Google, perhaps you can test this patch. If you're seeing too many new warnings, we might introduce a flag to turn this off (temporarily).

clang/lib/Analysis/ThreadSafety.cpp

1546

Or drop the parenthesized part.

2304–2305

You might want to do the * -> & in a separate commit.

[courbet]

Rebase on NFC changes

[courbet]

Thanks for the review.

clang/lib/Analysis/ThreadSafety.cpp

2304–2305

SG, done in https://github.com/llvm/llvm-project/pull/66750

[courbet]

rebase

[aaronpuchert]

Looks still good to me. As I wrote on D153132, I don't think we need it anymore, but if you disagree I think I can accept it as well.

[courbet]

In D153131#4651074, @aaronpuchert wrote:

Looks still good to me. As I wrote on D153132, I don't think we need it anymore, but if you disagree I think I can accept it as well.

Sorry, I misunderstood the last comment as an endorsement of the change. I've reverted the base commit and I'll rebase this on main without the changes.

[courbet]

In D153131#4651198, @courbet wrote:

In D153131#4651074, @aaronpuchert wrote:

Looks still good to me. As I wrote on D153132, I don't think we need it anymore, but if you disagree I think I can accept it as well.

Sorry, I misunderstood the last comment as an endorsement of the change. I've reverted the base commit and I'll rebase this on main without the changes.

After trying a rebase I think the code is better with the base change. See my comments in https://reviews.llvm.org/D153132.

[aeubanks]

This is finding lots of real issues in code, which is awesome, but could I request that this be put under a separate warning flag so we can toggle off just the new functionality and turn it on as we clean our codebase? e.g. -W[no-]thread-safety-analysis-return

edit: now I see the previous discussion on a separate subflag. on Chromium's codebase this is hitting lots of times and is too much to clean up all at once since some of it is hard to fix as somebody who's not an expert in the codebase.

[aaronpuchert]

In D153131#4653345, @aeubanks wrote:

This is finding lots of real issues in code, which is awesome, but could I request that this be put under a separate warning flag so we can toggle off just the new functionality and turn it on as we clean our codebase? e.g. -W[no-]thread-safety-analysis-return

Fine for me, but we might want to remove it again after one or two releases. I'm not sure how to communicate that this is just a “transitory” flag.

And it should be included by default in -Wthread-safety-reference, so that users of that flag see the new warnings/errors, and can demote them to warnings while fixing them. To emphasize the subflag status, I'd suggest something like -Wthread-safety-reference-return.

[courbet]

In D153131#4653362, @aaronpuchert wrote:

In D153131#4653345, @aeubanks wrote:

This is finding lots of real issues in code, which is awesome, but could I request that this be put under a separate warning flag so we can toggle off just the new functionality and turn it on as we clean our codebase? e.g. -W[no-]thread-safety-analysis-return

Fine for me, but we might want to remove it again after one or two releases. I'm not sure how to communicate that this is just a “transitory” flag.

And it should be included by default in -Wthread-safety-reference, so that users of that flag see the new warnings/errors, and can demote them to warnings while fixing them. To emphasize the subflag status, I'd suggest something like -Wthread-safety-reference-return.

I also had some push back internally on adding this to the existing flag. I'm going to add -Wthread-safety-reference-return, can we start by not temporarily including it in -Wthread-safety-reference so that we can see how much work it it to fix those warnings ?

[aaronpuchert]

In D153131#4653412, @courbet wrote:

I also had some push back internally on adding this to the existing flag. I'm going to add -Wthread-safety-reference-return, can we start by not temporarily including it in -Wthread-safety-reference so that we can see how much work it it to fix those warnings ?

Can you elaborate on this? What's the reasoning? Here are two reasons for having it as part of -Wthread-safety-reference right from the beginning:

• -Wthread-safety-reference is already separate from -Wthread-safety-analysis because passing a reference does not imply an access. If you have the warning you're arguably already opting into this, and I don't see much of a difference between passing via parameter versus passing by return.
• Most users don't follow all reviews or read the release notes in detail and won't notice the new flag until it shows up in their build log. So we'd just lose time.

Since warning messages always indicate the warning flag and thus make disabling it easy, I don't see an issue with enabling it right away as part of -Wthread-safety-reference.

Lastly, this doesn't seem complicated enough to warrant extended beta testing.

[aeubanks]

If people are passing -Wthread-safety-reference, there was clearly some value in the previous checks and it would be unfortunate to turn them off while fixing the codebase.
I'm not super familiar with flag families and if what I'm proposing is easily doable, but I think ideally we would keep this new functionality turned on by default under -Wthread-safety-reference and make just this new functionality toggleable under a subflag -W[no-]thread-safety-reference-return.

[courbet]

In D153131#4653456, @aaronpuchert wrote:

In D153131#4653412, @courbet wrote:

I also had some push back internally on adding this to the existing flag. I'm going to add -Wthread-safety-reference-return, can we start by not temporarily including it in -Wthread-safety-reference so that we can see how much work it it to fix those warnings ?

Can you elaborate on this? What's the reasoning? Here are two reasons for having it as part of -Wthread-safety-reference right from the beginning:

• -Wthread-safety-reference is already separate from -Wthread-safety-analysis because passing a reference does not imply an access. If you have the warning you're arguably already opting into this, and I don't see much of a difference between passing via parameter versus passing by return.
• Most users don't follow all reviews or read the release notes in detail and won't notice the new flag until it shows up in their build log. So we'd just lose time.

Since warning messages always indicate the warning flag and thus make disabling it easy, I don't see an issue with enabling it right away as part of -Wthread-safety-reference.

Lastly, this doesn't seem complicated enough to warrant extended beta testing.

We have a large number of users of -Werror -Wthread-safety-analysis internally. When we make the new warnings part of that flag we cannot integrate because we're breaking all these users. If we don't integrate we can't run the new analysis to see what we would need to fix.

Introducing a new flag allows us to:

• keep the current analysis running for users of -Wthread-safety-analysis.
• progressively add -Wthread-safety-analysis-reference-return to these users across the codebase, fixing them or disabling analysis as needed.

[aaronpuchert]

In D153131#4653564, @courbet wrote:

We have a large number of users of -Werror -Wthread-safety-analysis internally. When we make the new warnings part of that flag we cannot integrate because we're breaking all these users.

The proposal was to include it in -Wthread-safety-reference, not -Wthread-safety-analysis. See https://clang.llvm.org/docs/DiagnosticsReference.html#wthread-safety for the existing flags and their relations.

If we don't integrate we can't run the new analysis to see what we would need to fix.

Can you not add -Wno-error=thread-safety-reference-return together with the integration? Or are there too many places adding it independently?

Introducing a new flag allows us to:

• keep the current analysis running for users of -Wthread-safety-analysis.
• progressively add -Wthread-safety-analysis-reference-return to these users across the codebase, fixing them or disabling analysis as needed.

That is true, but these advantages seem to apply to a small number of users only (those aware of the new flag). If you integrate Clang trunk, it would be Ok if you leave it off by default for a couple of weeks, but turn it on before the next release.

I'm not generally against new flags, but this is more of a "gap closing" than a new feature, so an on-by-default (under -Wthread-safety-reference, not -Wthread-safety-analysis) warning should be the right choice. Changes that result in new warnings are not uncommon, and often we don't create a new flag for them at all. Here it's Ok due to the large number of warnings, but it fits too well into -Wthread-safety-reference to not be triggered by that.

[courbet]

In D153131#4653664, @aaronpuchert wrote:

In D153131#4653564, @courbet wrote:

We have a large number of users of -Werror -Wthread-safety-analysis internally. When we make the new warnings part of that flag we cannot integrate because we're breaking all these users.

The proposal was to include it in -Wthread-safety-reference, not -Wthread-safety-analysis. See https://clang.llvm.org/docs/DiagnosticsReference.html#wthread-safety for the existing flags and their relations.

Sorry, I meant -Wthread-safety-reference.

If we don't integrate we can't run the new analysis to see what we would need to fix.

Can you not add -Wno-error=thread-safety-reference-return together with the integration? Or are there too many places adding it independently?

Yes, we have way too many instances. I'm going to discuss with people dealing with integrates to see whether disabling the new flag globally is a possibility.

[thakis]

FWIW, the standard procedure for adding new functionality to existing warnings is (assuming that it makes the warning fire a lot, else no extra group is needed):

• Add it in a subgroup with its own flag
• Enable it by default

The reasoning is that people who aren't ready for the warning yet can then turn it off with the new flag, and everyone becomes aware of the new warning. If it's off-by-default, nobody will ever know about it.

(Of course, only warnings that are useful and high-signal should be added in the first place, so this is assuming that it's a warning with a very low false positive rate.)

[courbet]

I've updated https://github.com/llvm/llvm-project/pull/68572 to do as suggested.

To sum up: we have a new flag -Wthread-safety-reference-return, which is on by default under -Wthread-safety-reference.

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

enum ProtectedOperationKind {
 
   /// Making a function call (e.g. fool())
   POK_FunctionCall,
 
   /// Passing a guarded variable by reference.
   POK_PassByRef,
 
   /// Passing a pt-guarded variable by reference.
-  POK_PtPassByRef
+  POK_PtPassByRef,
+
+  /// Returning a guarded variable by reference.
+  POK_ReturnByRef,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
 /// example, it is an error to write a variable protected by shared version of a
 /// mutex.
 enum LockKind {
   /// Shared/reader lock of a mutex.
   LK_Shared,

clang/include/clang/Basic/DiagnosticSemaKinds.td

 // Thread safety warnings negative capabilities
 def warn_acquire_requires_negative_cap : Warning<
   "acquiring %0 '%1' requires negative capability '%2'">,
   InGroup<ThreadSafetyNegative>, DefaultIgnore;
 def warn_fun_requires_negative_cap : Warning<
   "calling function %0 requires negative capability '%1'">,
   InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
 
-// Thread safety warnings on pass by reference
+// Thread safety warnings on pass/return by reference
 def warn_guarded_pass_by_reference : Warning<
   "passing variable %1 by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup<ThreadSafetyReference>, DefaultIgnore;
 def warn_pt_guarded_pass_by_reference : Warning<
   "passing the value that %1 points to by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup<ThreadSafetyReference>, DefaultIgnore;
+def warn_guarded_return_by_reference : Warning<
+  "returning variable %1 by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup<ThreadSafetyReference>, DefaultIgnore;
 
 // Imprecise thread safety warnings
 def warn_variable_requires_lock : Warning<
   "%select{reading|writing}3 variable %1 requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
 def warn_var_deref_requires_lock : Warning<
   "%select{reading|writing}3 the value pointed to by %1 requires "
   "holding %0 %select{'%2'|'%2' exclusively}3">,
   InGroup<ThreadSafetyAnalysis>, DefaultIgnore;

clang/lib/Analysis/ThreadSafety.cpp

 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/ImmutableMap.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
class ThreadSafetyAnalyzer {
   friend class BuildLockset;
   friend class threadSafety::BeforeSet;
 
   llvm::BumpPtrAllocator Bpa;
   threadSafety::til::MemRegionRef Arena;
   threadSafety::SExprBuilder SxBuilder;
 
   ThreadSafetyHandler &Handler;
-  const CXXMethodDecl *CurrentMethod;
+  const FunctionDecl *CurrentFunction;
   LocalVariableMap LocalVarMap;
   FactManager FactMan;
   std::vector<CFGBlockInfo> BlockInfo;
 
   BeforeSet *GlobalBeforeSet;
 
   void warnIfMutexNotHeld(const FactSet &FSet, const NamedDecl *D,
                           const Expr *Exp, AccessKind AK, Expr *MutexExp,
if (const auto *LP = dyn_cast<til::LiteralPtr>(SExp)) {
     if (isa<CXXRecordDecl>(VD->getDeclContext()))
       return false;
     // Global variables are always in scope.
     return true;
   }
 
   // Members are in scope from methods of the same class.
   if (const auto *P = dyn_cast<til::Project>(SExp)) {
-    if (!CurrentMethod)
+    if (CurrentFunction == nullptr || !isa<CXXMethodDecl>(CurrentFunction))
       return false;
     const ValueDecl *VD = P->clangDecl();
-    return VD->getDeclContext() == CurrentMethod->getDeclContext();
+    return VD->getDeclContext() == CurrentFunction->getDeclContext();
   }
 
   return false;
 }
 
 /// Add a new lock to the lockset, warning if the lock is already there.
 /// \param ReqAttr -- true if this is part of an initial Requires attribute.
 void ThreadSafetyAnalyzer::addLock(FactSet &FSet,

 namespace {
 
 /// We use this class to visit different types of expressions in
 /// CFGBlocks, and build up the lockset.
 /// An expression may cause us to add or remove locks from the lockset, or else
 /// output error messages related to missing locks.
 /// FIXME: In future, we may be able to not inherit from a visitor.
 class BuildLockset : public ConstStmtVisitor<BuildLockset> {
-  using VisitorBase = ConstStmtVisitor<BuildLockset>;
   friend class ThreadSafetyAnalyzer;
 
   ThreadSafetyAnalyzer *Analyzer;
   FactSet FSet;
+  // The fact set for the function (i.e., its entry block).
+  const FactSet &FunctionFSet;
   /// Maps constructed objects to `this` placeholder prior to initialization.
   llvm::SmallDenseMap<const Expr *, til::LiteralPtr *> ConstructedObjects;
   LocalVariableMap::Context LVarCtx;
   unsigned CtxIndex;
 
   // helper functions
 
   void checkAccess(const Expr *Exp, AccessKind AK,
void handleCall(const Expr *Exp, const NamedDecl *D,
                   til::LiteralPtr *Self = nullptr,
                   SourceLocation Loc = SourceLocation());
   void examineArguments(const FunctionDecl *FD,
                         CallExpr::const_arg_iterator ArgBegin,
                         CallExpr::const_arg_iterator ArgEnd,
                         bool SkipFirstParam = false);
 
 public:
-  BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info)
-      : VisitorBase(), Analyzer(Anlzr), FSet(Info.EntrySet),
-        LVarCtx(Info.EntryContext), CtxIndex(Info.EntryIndex) {}
+  BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info,
+               const FactSet &FunctionFSet)
+      : ConstStmtVisitor<BuildLockset>(), Analyzer(Anlzr), FSet(Info.EntrySet),
+        FunctionFSet(FunctionFSet), LVarCtx(Info.EntryContext),
+        CtxIndex(Info.EntryIndex) {}
 
   void VisitUnaryOperator(const UnaryOperator *UO);
   void VisitBinaryOperator(const BinaryOperator *BO);
   void VisitCastExpr(const CastExpr *CE);
   void VisitCallExpr(const CallExpr *Exp);
   void VisitCXXConstructExpr(const CXXConstructExpr *Exp);
   void VisitDeclStmt(const DeclStmt *S);
   void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *Exp);
+  void VisitReturnStmt(const ReturnStmt *S);
 };
 
 } // namespace
 
 /// Warn if the LSet does not contain a lock sufficient to protect access
 /// of at least the passed in AccessKind.
 void ThreadSafetyAnalyzer::warnIfMutexNotHeld(
     const FactSet &FSet, const NamedDecl *D, const Expr *Exp, AccessKind AK,
if (auto Object =
             ConstructedObjects.find(UnpackConstruction(Exp->getSubExpr()));
         Object != ConstructedObjects.end()) {
       Object->second->setClangDecl(ExtD);
       ConstructedObjects.erase(Object);
     }
   }
 }
 
+void BuildLockset::VisitReturnStmt(const ReturnStmt *S) {
+  if (Analyzer->CurrentFunction == nullptr)
+    return;
+  const Expr *RetVal = S->getRetValue();
+  if (!RetVal)
+    return;
+
+  // If returning by reference, check that the function requires the appropriate
+  // capabilities.
+  const QualType ReturnType =
+      Analyzer->CurrentFunction->getReturnType().getCanonicalType();
+  if (ReturnType->isLValueReferenceType()) {
+    Analyzer->checkAccess(
+        FunctionFSet, RetVal,
+        ReturnType->getPointeeType().isConstQualified() ? AK_Read : AK_Written,
+        POK_ReturnByRef);
+  }
+}
+
 /// Given two facts merging on a join point, possibly warn and decide whether to
 /// keep or replace.
 ///
 /// \param CanModify Whether we can replace \p A by \p B.
 /// \return  false if we should keep \p A, true if we should take \p B.
 bool ThreadSafetyAnalyzer::join(const FactEntry &A, const FactEntry &B,
                                 bool CanModify) {
   if (A.kind() != B.kind()) {
void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
   if (!walker.init(AC))
     return;
 
   // AC.dumpCFG(true);
   // threadSafety::printSCFG(walker);
 
   CFG *CFGraph = walker.getGraph();
   const NamedDecl *D = walker.getDecl();
-  const auto *CurrentFunction = dyn_cast<FunctionDecl>(D);
-  CurrentMethod = dyn_cast<CXXMethodDecl>(D);
+  CurrentFunction = dyn_cast<FunctionDecl>(D);
 
   if (D->hasAttr<NoThreadSafetyAnalysisAttr>())
     return;
 
   // FIXME: Do something a bit more intelligent inside constructor and
   // destructor code.  Constructors and destructors must assume unique access
   // to 'this', so checks on member variable access is disabled, but we should
   // still enable checks on other objects.
   if (isa<CXXConstructorDecl>(D))
     return;  // Don't check inside constructors.
   if (isa<CXXDestructorDecl>(D))
     return;  // Don't check inside destructors.
 
   Handler.enterFunction(CurrentFunction);
 
   BlockInfo.resize(CFGraph->getNumBlockIDs(),
     CFGBlockInfo::getEmptyBlockInfo(LocalVarMap));
 
   // We need to explore the CFG via a "topological" ordering.
   // That way, we will be guaranteed to have information about required
   // predecessor locksets when exploring a new block.
   const PostOrderCFGView *SortedGraph = walker.getSortedGraph();
   PostOrderCFGView::CFGBlockSet VisitedBlocks(CFGraph);
 
+  CFGBlockInfo *Initial = &BlockInfo[CFGraph->getEntry().getBlockID()];
+  CFGBlockInfo *Final = &BlockInfo[CFGraph->getExit().getBlockID()];
+
   // Mark entry block as reachable
   BlockInfo[CFGraph->getEntry().getBlockID()].Reachable = true;
 
   // Compute SSA names for local variables
   LocalVarMap.traverseCFG(CFGraph, SortedGraph, BlockInfo);
 
   // Fill in source locations for all CFGBlocks.
   findBlockLocations(CFGraph, SortedGraph, BlockInfo);
 
   CapExprSet ExclusiveLocksAcquired;
   CapExprSet SharedLocksAcquired;
   CapExprSet LocksReleased;
 
   // Add locks from exclusive_locks_required and shared_locks_required
   // to initial lockset. Also turn off checking for lock and unlock functions.
   // FIXME: is there a more intelligent way to check lock/unlock functions?
   if (!SortedGraph->empty() && D->hasAttrs()) {
-    const CFGBlock *FirstBlock = *SortedGraph->begin();
-    FactSet &InitialLockset = BlockInfo[FirstBlock->getBlockID()].EntrySet;
+    FactSet &InitialLockset = Initial->EntrySet;
 
     CapExprSet ExclusiveLocksToAdd;
     CapExprSet SharedLocksToAdd;
 
     SourceLocation Loc = D->getLocation();
     for (const auto *Attr : D->attrs()) {
       Loc = Attr->getLocation();
       if (const auto *A = dyn_cast<RequiresCapabilityAttr>(Attr)) {
for (CFGBlock::const_pred_iterator PI = CurrBlock->pred_begin(),
                 : LEK_LockedSomePredecessors);
       }
     }
 
     // Skip rest of block if it's not reachable.
     if (!CurrBlockInfo->Reachable)
       continue;
 
-    BuildLockset LocksetBuilder(this, *CurrBlockInfo);
+    BuildLockset LocksetBuilder(this, *CurrBlockInfo, Initial->EntrySet);
 
     // Visit all the statements in the basic block.
     for (const auto &BI : *CurrBlock) {
       switch (BI.getKind()) {
         case CFGElement::Statement: {
           CFGStmt CS = BI.castAs<CFGStmt>();
           LocksetBuilder.Visit(CS.getStmt());
           break;
for (CFGBlock::const_succ_iterator SI = CurrBlock->succ_begin(),
       CFGBlock *FirstLoopBlock = *SI;
       CFGBlockInfo *PreLoop = &BlockInfo[FirstLoopBlock->getBlockID()];
       CFGBlockInfo *LoopEnd = &BlockInfo[CurrBlockID];
       intersectAndWarn(PreLoop->EntrySet, LoopEnd->ExitSet, PreLoop->EntryLoc,
                        LEK_LockedSomeLoopIterations);
     }
   }
 
-  CFGBlockInfo *Initial = &BlockInfo[CFGraph->getEntry().getBlockID()];
-  CFGBlockInfo *Final   = &BlockInfo[CFGraph->getExit().getBlockID()];
-
   // Skip the final check if the exit block is unreachable.
   if (!Final->Reachable)
     return;
 
   // By default, we expect all locks held on entry to be held on exit.
   FactSet ExpectedExitSet = Initial->EntrySet;
 
   // Adjust the expected exit set by adding or removing locks, as declared
   // by *-LOCK_FUNCTION and UNLOCK_FUNCTION.  The intersect below will then
   // issue the appropriate warning.
   // FIXME: the location here is not quite right.
   for (const auto &Lock : ExclusiveLocksAcquired)
     ExpectedExitSet.addLock(FactMan, std::make_unique<LockableFactEntry>(
                                          Lock, LK_Exclusive, D->getLocation()));
   for (const auto &Lock : SharedLocksAcquired)
     ExpectedExitSet.addLock(FactMan, std::make_unique<LockableFactEntry>(
                                          Lock, LK_Shared, D->getLocation()));
   for (const auto &Lock : LocksReleased)
     ExpectedExitSet.removeLock(FactMan, Lock);
 
   // FIXME: Should we call this function for all blocks which exit the function?
   intersectAndWarn(ExpectedExitSet, Final->ExitSet, Final->ExitLoc,
                    LEK_LockedAtEndOfFunction, LEK_NotLockedAtEndOfFunction);
 
   Handler.leaveFunction(CurrentFunction);
 }
 
 /// Check a function's CFG for thread-safety violations.
 ///
 /// We traverse the blocks in the CFG, compute the set of mutexes that are held
 /// at the end of each block, and issue warnings for thread safety violations.

clang/lib/Sema/AnalysisBasedWarnings.cpp

if (PossibleMatch) {
           DiagID = diag::warn_fun_requires_lock_precise;
           break;
         case POK_PassByRef:
           DiagID = diag::warn_guarded_pass_by_reference;
           break;
         case POK_PtPassByRef:
           DiagID = diag::warn_pt_guarded_pass_by_reference;
           break;
+        case POK_ReturnByRef:
+          DiagID = diag::warn_guarded_return_by_reference;
+          break;
       }
       PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind
                                                        << D
                                                        << LockName << LK);
       PartialDiagnosticAt Note(Loc, S.PDiag(diag::note_found_mutex_near_match)
                                         << *PossibleMatch);
       if (Verbose && POK == POK_VarAccess) {
         PartialDiagnosticAt VNote(D->getLocation(),
if (PossibleMatch) {
           DiagID = diag::warn_fun_requires_lock;
           break;
         case POK_PassByRef:
           DiagID = diag::warn_guarded_pass_by_reference;
           break;
         case POK_PtPassByRef:
           DiagID = diag::warn_pt_guarded_pass_by_reference;
           break;
+        case POK_ReturnByRef:
+          DiagID = diag::warn_guarded_return_by_reference;
+          break;
       }
       PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind
                                                        << D
                                                        << LockName << LK);
       if (Verbose && POK == POK_VarAccess) {
         PartialDiagnosticAt Note(D->getLocation(),
                                  S.PDiag(diag::note_guarded_by_declared_here));
         Warnings.emplace_back(std::move(Warning), getNotes(Note));

clang/test/SemaCXX/warn-thread-safety-analysis.cpp

void test1() {
     write1(*foosp.get());
     write2(10, *foosp.get());
     read1(*foosp.get());
     read2(10, *foosp.get());
     destroy(mymove(*foosp.get()));
   }
 };
 
+class Return {
+  Mutex mu;
+  Foo foo GUARDED_BY(mu);
+
+  Foo returns_value_locked() {
+    MutexLock lock(&mu);
+    return foo;
+  }
+
+  Foo returns_value_locks_required() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+    return foo;
+  }
+
+  Foo returns_value_not_locked() {
+    return foo;               // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}}
+  }
+
+  Foo &returns_ref() {
+    return foo;               // expected-warning {{returning variable 'foo' by reference requires holding mutex 'mu'}}
+  }
+
+  Foo &returns_ref_locked() {
+    MutexLock lock(&mu);
+    return foo;               // expected-warning {{returning variable 'foo' by reference requires holding mutex 'mu'}}
+  }
+
+  Foo &returns_ref_shared_locks_required() SHARED_LOCKS_REQUIRED(mu) {
+    return foo;               // expected-warning {{returning variable 'foo' by reference requires holding mutex 'mu' exclusively}}
+  }
+
+  Foo &returns_ref_exclusive_locks_required() EXCLUSIVE_LOCKS_REQUIRED(mu) {
+    return foo;
+  }
+
+  const Foo &returns_constref_shared_locks_required() SHARED_LOCKS_REQUIRED(mu) {
+    return foo;
+  }
+
+  Foo *returns_ptr() {
+    return &foo;              // FIXME -- Do we want to warn on this ?
+  }
+};
+
 
 }  // end namespace PassByRefTest
 
 
 namespace AcquiredBeforeAfterText {
 
 class Foo {
   Mutex mu1 ACQUIRED_BEFORE(mu2, mu3);
   Mutex mu2;
   Mutex mu3;
 
   void test1() {
     mu1.Lock();