Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

[aaronpuchert]

Summary

When support for copy elision was initially added in e97654b2f2807, it
was taking attributes from a constructor call, although that constructor
call is actually not involved. It seems more natural to use attributes
on the function returning the scoped capability, which is where it's
actually coming from. This would also support a number of interesting
use cases, like producing different scope kinds without the need for tag
types, or producing scopes from a private mutex.

Changing the behavior was surprisingly difficult: we were not handling
CXXConstructorExpr calls like regular calls but instead handled them
through the DeclStmt they're contained in. This was based on the
assumption that constructors are basically only called in variable
declarations (not true because of temporaries), and that variable
declarations necessitate constructors (not true with C++17 anymore).

Untangling this required separating construction from assigning a
variable name. When a call produces an object, we use a placeholder
til::LiteralPtr for this, and we collect the call expression and
placeholder in a map. Later when going through a DeclStmt, we look up
the call expression and set the placeholder to the new VarDecl.

The change has a couple of nice side effects:

• We don't miss constructor calls not contained in DeclStmts anymore, allowing patterns like MutexLock{&mu}, requiresMutex(); The scoped lock temporary will be destructed at the end of the full statement, so it protects the following call without the need for a scope, but with the ability to unlock in case of an exception.
• We support lifetime extension of temporaries. While unusual, one can now write const MutexLock &scope = MutexLock(&mu); and have it behave as expected.
• Destructors used to be handled in a weird way: since there is no expression in the AST for implicit destructor calls, we instead provided a made-up DeclRefExpr to the variable being destructed, and passed that instead of a CallExpr. Then later in translateAttrExpr there was special code that knew that destructor expressions worked a bit different.
• We were producing dummy DeclRefExprs in a number of places, this has been eliminated. We now use til::SExprs instead.

Technically this could break existing code, but the current handling
seems unexpected enough to justify this change.

[aaronpuchert]

clang/docs/ThreadSafetyAnalysis.rst

935

The NO_THREAD_SAFETY_ANALYSIS here is due to a false positive warning because the scope is not destructed in the function body. Maybe we should postpone documentation until this is resolved.

clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h

653–654

For placeholders we're comparing identity.

clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h

629

Perhaps no warning will ever print this, but I'm not entirely sure.

clang/lib/Analysis/ThreadSafety.cpp

1223

Placeholders are always local variables.

1532

This is essentially an std::map<const Expr *, til::LiteralPtr *>, but since it only keeps objects between construction and assignment to a variable or temporary destruction, I thought that a small vector would be more appropriate. However, there is no guarantee that this doesn't grow in size significantly: temporaries with trivial destructors (which don't show up in the CFG) would never be removed from this list. That's unlikely for properly annotated scopes, but not impossible.

1791–1793

In case you're wondering why we need this for records with ScopedLockableAttr: otherwise we get failures in SelfConstructorTest:

class SelfLock {
public:
  SelfLock()  EXCLUSIVE_LOCK_FUNCTION(mu_);
  ~SelfLock() UNLOCK_FUNCTION(mu_);

  void foo() EXCLUSIVE_LOCKS_REQUIRED(mu_);

  Mutex mu_;
};

void test() {
  SelfLock s;
  s.foo();
}

For s.foo() we need s.mu, and to see that we've acquired this we need to plugin s for the placeholder that we used for construction, although SelfLock is not a scoped lockable.

Though we could restrict this to functions that have actual thread safety attributes (instead of other attributes).

2122–2134

This is not strictly required, but it enables the lifetime_extension test below. It could also be a separate change, but having it here demonstrates that the approach can handle this case as well.

2418–2436

Scopes as temporaries are probably unusual, but as the temporary test demonstrates they could be useful. In any event, if we recognize acquisition via CXXConstructExprs not contained in a DeclStmt, we should probably also remove locks when they disappear again.

[aaronpuchert]

Adding @rsmith because this essentially reverts rGe97654b2f28072ad9123006c05e03efd82852982, and @efriedma because it partially reverts D76943. No need for a detailed review, but I'd like to know if this would break use cases that you're interested in.

[aaronpuchert]

clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h

435

By the way, I considered using this, but it didn't seem wholly appropriate. Our placeholder can't be lazily evaluated, we simply don't know initially. And later we do know and can just plug in the VarDecl.

clang/lib/Analysis/ThreadSafety.cpp

1791–1793

Sorry, records without ScopedLockableAttr of course.

[aaronpuchert]

Ping.

[aaron.ballman]

Thank you for your patience on the review! I've taken a cursory look through and I'm still thinking about the patch. I've not seen anything that's really worrying. But this is pretty dense stuff and @delesley has way more experience with TIL, so I'm hoping he might be able to take a peek as well.

clang/docs/ThreadSafetyAnalysis.rst

935

I don't know that there's a lot of value to documenting it if we have to disable thread safety analysis, so my instinct is to hold off on the documentation for the moment.

clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h

435

Hmmm, naively, it seems like FS_pending is "we don't know initially" and FS_done is "now we do know and can plugin in the VarDecl". But I agree it seems a little different from lazy evaluation in that it's not a performance optimization.

clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h

629

Should we assert here instead?

clang/lib/Analysis/ThreadSafety.cpp

1532

Would it make more sense to use a SmallDenseMap here?

2419

clang/lib/Analysis/ThreadSafetyCommon.cpp

339–342

[aaronpuchert]

clang/docs/ThreadSafetyAnalysis.rst

935

Ok, then let's leave it out for now.

clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h

435

Yes, it seems quite close. But my reading is that calling result is supposed to force the evaluation, even when we might not be able to provide a name yet.

clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h

629

I played around a bit and it is actually possible to run into this in contrived examples. I'll push some tests so that you can judge for yourself how we should print this.

clang/lib/Analysis/ThreadSafety.cpp

1532

I can't find an awful lot of documentation about this, but judging from the name and a quick glance at the code I think that might be just what I'm looking for. Thanks for the tip!

2419

I was imitating lines 2405/2411, but auto is of course fine. (The code above might actually predate C++11.)

clang/lib/Analysis/ThreadSafetyCommon.cpp

339–342

No doubt referring to https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return. I was trying to emphasize the dual nature of llvm::PointerUnion<const Expr *, til::SExpr *>, but I can certainly also drop the else.

[aaronpuchert]

Use SmallDenseMap plus some minor changes.

[aaronpuchert]

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

4254–4260

Here we're printing <temporary>, because we don't have a name for this object.

[aaronpuchert]

I was under the impression that we've already switched to C++17, but the Windows pre-submit build failed with:

C:\ws\w9\llvm-project\premerge-checks\clang\lib\Analysis\ThreadSafety.cpp(2107): error C2429: language feature 'init-statements in if/switch' requires compiler flag '/std:c++17'
C:\ws\w9\llvm-project\premerge-checks\clang\lib\Analysis\ThreadSafety.cpp(2120): error C2429: language feature 'init-statements in if/switch' requires compiler flag '/std:c++17'
C:\ws\w9\llvm-project\premerge-checks\clang\lib\Analysis\ThreadSafety.cpp(2418): error C2429: language feature 'init-statements in if/switch' requires compiler flag '/std:c++17'

Perhaps I should move the init statements out again?

[aaron.ballman]

In D129755#3777038, @aaronpuchert wrote:

I was under the impression that we've already switched to C++17, but the Windows pre-submit build failed with:

C:\ws\w9\llvm-project\premerge-checks\clang\lib\Analysis\ThreadSafety.cpp(2107): error C2429: language feature 'init-statements in if/switch' requires compiler flag '/std:c++17'
C:\ws\w9\llvm-project\premerge-checks\clang\lib\Analysis\ThreadSafety.cpp(2120): error C2429: language feature 'init-statements in if/switch' requires compiler flag '/std:c++17'
C:\ws\w9\llvm-project\premerge-checks\clang\lib\Analysis\ThreadSafety.cpp(2418): error C2429: language feature 'init-statements in if/switch' requires compiler flag '/std:c++17'

Perhaps I should move the init statements out again?

No, I've filed https://github.com/google/llvm-premerge-checks/issues/416 to find out what's going on with the Windows precommit CI bot.

The changes LGTM, though it'd still be great if @delesley had some time to put a second set of eyes on the changes. If we don't hear from him by Tue, I think we should go ahead and land this. Please be sure to add a release note for the changes!

clang/lib/Analysis/ThreadSafetyCommon.cpp

339–342

I don't have a strong opinion.

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

4254–4260

Ah thank you, this example makes a lot of sense. I think printing <temporary> is quite reasonable.

[aaronpuchert]

In D129755#3779997, @aaron.ballman wrote:

Please be sure to add a release note for the changes!

Any opinion as to what the release note might say? I'm asking since we dropped the documentation changes.

Perhaps I should add them back in, but without function bodies (thus eliminating the need to add NO_THREAD_SAFETY_ANALYSIS)? Something like this:

// Same as constructors, but without tag types. (Requires C++17 copy elision.)
static MutexLocker Lock(Mutex *mu) ACQUIRE(mu);
static MutexLocker Adopt(Mutex *mu) REQUIRES(mu);
static MutexLocker ReaderLock(Mutex *mu) ACQUIRE_SHARED(mu);
static MutexLocker AdoptReaderLock(Mutex *mu) REQUIRES_SHARED(mu);
static MutexLocker DeferLock(Mutex *mu) EXCLUDES(mu);

Then I could write in the note that this is now possible, and annotations on move constructors are without effect.

[aaron.ballman]

In D129755#3796799, @aaronpuchert wrote:

In D129755#3779997, @aaron.ballman wrote:

Please be sure to add a release note for the changes!

Any opinion as to what the release note might say? I'm asking since we dropped the documentation changes.

No strong opinion, but Technically this could break existing code, but the current handling seems unexpected enough to justify this change. suggests we should be mentioning to users what we changed and what code could theoretically break.

Perhaps I should add them back in, but without function bodies (thus eliminating the need to add NO_THREAD_SAFETY_ANALYSIS)? Something like this:

// Same as constructors, but without tag types. (Requires C++17 copy elision.)
static MutexLocker Lock(Mutex *mu) ACQUIRE(mu);
static MutexLocker Adopt(Mutex *mu) REQUIRES(mu);
static MutexLocker ReaderLock(Mutex *mu) ACQUIRE_SHARED(mu);
static MutexLocker AdoptReaderLock(Mutex *mu) REQUIRES_SHARED(mu);
static MutexLocker DeferLock(Mutex *mu) EXCLUDES(mu);

Then I could write in the note that this is now possible, and annotations on move constructors are without effect.

I'd be okay with that!

[aaronpuchert]

Add trimmed-down documentation back in and a release note.

[aaron.ballman]

LGTM!

[chapuni]

clang/lib/Analysis/ThreadSafety.cpp

1794

'inserted' is used only here.

[aaronpuchert]

clang/lib/Analysis/ThreadSafety.cpp

1794

Correct, is that an issue? Perhaps -Wunused-variable in Release builds?

Otherwise I believe it's correct—we don't need the iterator and we should generally not insert expressions twice, hence the assertion.

[aaron.ballman]

clang/lib/Analysis/ThreadSafety.cpp

1794

Yeah, that's the trouble -- this breaks release builds using -Werror. You should add [[maybe_unused]] to the declaration (as an NFC commit).

[hans]

We're hitting a false positive in grpc after this:

> ../../third_party/grpc/src/src/core/lib/gprpp/ref_counted_ptr.h:335:31: error: calling function 'TlsSessionKeyLoggerCache' requires holding mutex 'g_tls_session_key_log_cache_mu' exclusively [-Werror,-Wthread-safety-analysis]
>   return RefCountedPtr<T>(new T(std::forward<Args>(args)...));
>                               ^
> ../../third_party/grpc/src/src/core/tsi/ssl/key_logging/ssl_key_logging.cc:121:26: note: in instantiation of function template specialization 'grpc_core::MakeRefCounted<tsi::TlsSessionKeyLoggerCache>' requested here
>      cache = grpc_core::MakeRefCounted<TlsSessionKeyLoggerCache>();
>                         ^


The code looks like this:

grpc_core::RefCountedPtr<TlsSessionKeyLogger> TlsSessionKeyLoggerCache::Get(
    std::string tls_session_key_log_file_path) {
  gpr_once_init(&g_cache_mutex_init, do_cache_mutex_init);
  GPR_DEBUG_ASSERT(g_tls_session_key_log_cache_mu != nullptr);
  if (tls_session_key_log_file_path.empty()) {
    return nullptr;
  }
  {
    grpc_core::MutexLock lock(g_tls_session_key_log_cache_mu);        <---------- holding the mutex
    grpc_core::RefCountedPtr<TlsSessionKeyLoggerCache> cache;
    if (g_cache_instance == nullptr) {
      // This will automatically set g_cache_instance.
      cache = grpc_core::MakeRefCounted<TlsSessionKeyLoggerCache>();      <------ line 121



lock is holding a MutexLock (I assume that's an exclusive thing) on g_tls_session_key_log_cache_mu.

See https://bugs.chromium.org/p/chromium/issues/detail?id=1372394#c4 for how to reproduce.

I've reverted this in https://github.com/llvm/llvm-project/commit/a4afa2bde6f4db215ddd3267a8d11c04367812e5 in the meantime.

[aaron.ballman]

In D129755#3842729, @hans wrote:

We're hitting a false positive in grpc after this:

> ../../third_party/grpc/src/src/core/lib/gprpp/ref_counted_ptr.h:335:31: error: calling function 'TlsSessionKeyLoggerCache' requires holding mutex 'g_tls_session_key_log_cache_mu' exclusively [-Werror,-Wthread-safety-analysis]
>   return RefCountedPtr<T>(new T(std::forward<Args>(args)...));
>                               ^
> ../../third_party/grpc/src/src/core/tsi/ssl/key_logging/ssl_key_logging.cc:121:26: note: in instantiation of function template specialization 'grpc_core::MakeRefCounted<tsi::TlsSessionKeyLoggerCache>' requested here
>      cache = grpc_core::MakeRefCounted<TlsSessionKeyLoggerCache>();
>                         ^


The code looks like this:

grpc_core::RefCountedPtr<TlsSessionKeyLogger> TlsSessionKeyLoggerCache::Get(
    std::string tls_session_key_log_file_path) {
  gpr_once_init(&g_cache_mutex_init, do_cache_mutex_init);
  GPR_DEBUG_ASSERT(g_tls_session_key_log_cache_mu != nullptr);
  if (tls_session_key_log_file_path.empty()) {
    return nullptr;
  }
  {
    grpc_core::MutexLock lock(g_tls_session_key_log_cache_mu);        <---------- holding the mutex
    grpc_core::RefCountedPtr<TlsSessionKeyLoggerCache> cache;
    if (g_cache_instance == nullptr) {
      // This will automatically set g_cache_instance.
      cache = grpc_core::MakeRefCounted<TlsSessionKeyLoggerCache>();      <------ line 121



lock is holding a MutexLock (I assume that's an exclusive thing) on g_tls_session_key_log_cache_mu.

See https://bugs.chromium.org/p/chromium/issues/detail?id=1372394#c4 for how to reproduce.

I've reverted this in https://github.com/llvm/llvm-project/commit/a4afa2bde6f4db215ddd3267a8d11c04367812e5 in the meantime.

I'm not opposed to a revert here, but reverting on (plausible) assumptions of this being a false positive without discussion or a reproducer beyond "compile chromium" comes across as a somewhat aggressive revert. Especially given that we knew this had the potential to break code and we were okay with that code being broken (as stated in the commit message). No community bots went red from this change and it's possible this could have been fixed forward/explained as a desired break with less churn to the codebase. Nothing to be done for it now (and it wasn't wrong per se), just something to keep in mind for future reverts -- a bit of discussion before a revert like this would be appropriate.

[hans]

We also hit a different case: https://bugs.chromium.org/p/chromium/issues/detail?id=1372394#c6

[gulfem]

We also started seeing -Wthread-safety-precise error in our Fuchsia code.
https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8800959115965408001/overview
I'm trying to verify with our team whether it is a false positive, but I just wanted to give you heads up!

[aaronpuchert]

In D129755#3842729, @hans wrote:

We're hitting a false positive in grpc after this:

> ../../third_party/grpc/src/src/core/lib/gprpp/ref_counted_ptr.h:335:31: error: calling function 'TlsSessionKeyLoggerCache' requires holding mutex 'g_tls_session_key_log_cache_mu' exclusively [-Werror,-Wthread-safety-analysis]
>   return RefCountedPtr<T>(new T(std::forward<Args>(args)...));
>                               ^
> ../../third_party/grpc/src/src/core/tsi/ssl/key_logging/ssl_key_logging.cc:121:26: note: in instantiation of function template specialization 'grpc_core::MakeRefCounted<tsi::TlsSessionKeyLoggerCache>' requested here
>      cache = grpc_core::MakeRefCounted<TlsSessionKeyLoggerCache>();
>                         ^


The code looks like this:

grpc_core::RefCountedPtr<TlsSessionKeyLogger> TlsSessionKeyLoggerCache::Get(
    std::string tls_session_key_log_file_path) {
  gpr_once_init(&g_cache_mutex_init, do_cache_mutex_init);
  GPR_DEBUG_ASSERT(g_tls_session_key_log_cache_mu != nullptr);
  if (tls_session_key_log_file_path.empty()) {
    return nullptr;
  }
  {
    grpc_core::MutexLock lock(g_tls_session_key_log_cache_mu);        <---------- holding the mutex
    grpc_core::RefCountedPtr<TlsSessionKeyLoggerCache> cache;
    if (g_cache_instance == nullptr) {
      // This will automatically set g_cache_instance.
      cache = grpc_core::MakeRefCounted<TlsSessionKeyLoggerCache>();      <------ line 121



lock is holding a MutexLock (I assume that's an exclusive thing) on g_tls_session_key_log_cache_mu.

See https://bugs.chromium.org/p/chromium/issues/detail?id=1372394#c4 for how to reproduce.

I've reverted this in https://github.com/llvm/llvm-project/commit/a4afa2bde6f4db215ddd3267a8d11c04367812e5 in the meantime.

This doesn't look like a false positive to me. The documentation states that no inlining is being done, so unless grpc_core::MakeRefCounted (or a specialization) has a require_capability attribute, the lock doesn't count as locked in the function body. That has always been the case.

The only thing that changed is that we now also consider CXXConstructExpr outside of DeclStmts, which is arguably an improvement.

In D129755#3842744, @hans wrote:

We also hit a different case: https://bugs.chromium.org/p/chromium/issues/detail?id=1372394#c6

From the logs:

../../extensions/browser/api/content_settings/content_settings_store.cc(75,49): note: mutex acquired here
  std::unique_ptr<base::AutoLock> auto_lock(new base::AutoLock(lock_));
                                                ^

That seems to be precisely the example that the documentation says doesn't work. The constructor of std::unique_ptr has no thread safety annotations and so we have a constructor call without a matching destructor call. (The destructor is called indirectly from the destructor of unique_ptr.) We see this now because we've started looking at constructor calls that don't initialize a local variable.

The documentation also lists ways to make this work (such as having more powerful scope types).

I'll give you some time to reply before I reland, but I can't see a bug here. For the next time, as already pointed out, give the involved people some time to reply before you revert (unless it's a crash). We have to be able to continue working on the compiler without having Google revert changes all the time because we produce new warnings.

[aaronpuchert]

In D129755#3843144, @gulfem wrote:

We also started seeing -Wthread-safety-precise error in our Fuchsia code.
https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8800959115965408001/overview
I'm trying to verify with our team whether it is a false positive, but I just wanted to give you heads up!

Both places have

Cursor cursor(DiscardableVmosLock::Get(), ...);
AssertHeld(cursor.lock_ref());

At the end of the scope we get

error: calling function '~VmoCursor' requires holding mutex 'lock_' exclusively [-Werror,-Wthread-safety-precise]
note: found near match 'cursor.lock_'

Presumably Cursor is some kind of alias to VmoCursor, as we don't look at base destructors yet. Since the code is not easily searchable for me, can you look up the annotations on DiscardableVmosLock::Get, the constructor of Cursor/VmoCursor being used here, Cursor::lock_ref, and AssertHeld?

Edit: the second error seems to be same that @hans was posting. That's a true positive sadly, we didn't emit a warning previously by accident.

[aaronpuchert]

In D129755#3843206, @aaronpuchert wrote:

Presumably Cursor is some kind of alias to VmoCursor, as we don't look at base destructors yet. Since the code is not easily searchable for me, can you look up the annotations on DiscardableVmosLock::Get, the constructor of Cursor/VmoCursor being used here, Cursor::lock_ref, and AssertHeld?

Nevermind, I think I've found it: lock_ref has TA_RET_CAP(lock_), so cursor.lock_ref() translates to cursor.lock_. So we have that lock. The destructor ~VmoCursor has TA_REQ(lock_) referring to the same lock. So that looks like a bug, I think we're missing the substitution somehow. Need to look into it.

[gulfem]

In D129755#3843206, @aaronpuchert wrote:

In D129755#3843144, @gulfem wrote:

We also started seeing -Wthread-safety-precise error in our Fuchsia code.
https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8800959115965408001/overview
I'm trying to verify with our team whether it is a false positive, but I just wanted to give you heads up!

Both places have

Cursor cursor(DiscardableVmosLock::Get(), ...);
AssertHeld(cursor.lock_ref());

At the end of the scope we get

error: calling function '~VmoCursor' requires holding mutex 'lock_' exclusively [-Werror,-Wthread-safety-precise]
note: found near match 'cursor.lock_'

Presumably Cursor is some kind of alias to VmoCursor, as we don't look at base destructors yet. Since the code is not easily searchable for me, can you look up the annotations on DiscardableVmosLock::Get, the constructor of Cursor/VmoCursor being used here, Cursor::lock_ref, and AssertHeld?

https://cs.opensource.google/fuchsia/fuchsia/+/main:zircon/kernel/vm/vm_cow_pages.cc
It looks like Cursor is an alias to VmoCursor.
https://cs.opensource.google/fuchsia/fuchsia/+/main:zircon/kernel/vm/include/vm/vm_cow_pages.h;drc=0f6485b1f7f6eb73f9faa4653f34def3be827d15;l=1373

Edit: the second error seems to be same that @hans was posting. That's a true positive sadly, we didn't emit a warning previously by accident.

Yes, that is the same error.

[hans]

In D129755#3843146, @aaronpuchert wrote:

In D129755#3842729, @hans wrote:

We're hitting a false positive in grpc after this:

> ../../third_party/grpc/src/src/core/lib/gprpp/ref_counted_ptr.h:335:31: error: calling function 'TlsSessionKeyLoggerCache' requires holding mutex 'g_tls_session_key_log_cache_mu' exclusively [-Werror,-Wthread-safety-analysis]
>   return RefCountedPtr<T>(new T(std::forward<Args>(args)...));
>                               ^
> ../../third_party/grpc/src/src/core/tsi/ssl/key_logging/ssl_key_logging.cc:121:26: note: in instantiation of function template specialization 'grpc_core::MakeRefCounted<tsi::TlsSessionKeyLoggerCache>' requested here
>      cache = grpc_core::MakeRefCounted<TlsSessionKeyLoggerCache>();
>                         ^


The code looks like this:

grpc_core::RefCountedPtr<TlsSessionKeyLogger> TlsSessionKeyLoggerCache::Get(
    std::string tls_session_key_log_file_path) {
  gpr_once_init(&g_cache_mutex_init, do_cache_mutex_init);
  GPR_DEBUG_ASSERT(g_tls_session_key_log_cache_mu != nullptr);
  if (tls_session_key_log_file_path.empty()) {
    return nullptr;
  }
  {
    grpc_core::MutexLock lock(g_tls_session_key_log_cache_mu);        <---------- holding the mutex
    grpc_core::RefCountedPtr<TlsSessionKeyLoggerCache> cache;
    if (g_cache_instance == nullptr) {
      // This will automatically set g_cache_instance.
      cache = grpc_core::MakeRefCounted<TlsSessionKeyLoggerCache>();      <------ line 121



lock is holding a MutexLock (I assume that's an exclusive thing) on g_tls_session_key_log_cache_mu.

See https://bugs.chromium.org/p/chromium/issues/detail?id=1372394#c4 for how to reproduce.

I've reverted this in https://github.com/llvm/llvm-project/commit/a4afa2bde6f4db215ddd3267a8d11c04367812e5 in the meantime.

This doesn't look like a false positive to me. The documentation states that no inlining is being done, so unless grpc_core::MakeRefCounted (or a specialization) has a require_capability attribute, the lock doesn't count as locked in the function body. That has always been the case.

Thanks for looking into this so quickly.

I'd still call it a false positive since the mutex is in fact being held. However I now understand that this is due to a pre-existing limitation in the analysis.

How would you suggest the developers work around this?

In D129755#3842744, @hans wrote:

We also hit a different case: https://bugs.chromium.org/p/chromium/issues/detail?id=1372394#c6

From the logs:

../../extensions/browser/api/content_settings/content_settings_store.cc(75,49): note: mutex acquired here
  std::unique_ptr<base::AutoLock> auto_lock(new base::AutoLock(lock_));
                                                ^

That seems to be precisely the example that the documentation says doesn't work. The constructor of std::unique_ptr has no thread safety annotations and so we have a constructor call without a matching destructor call. (The destructor is called indirectly from the destructor of unique_ptr. We see this now because of the constructor call that doesn't initialize a local variable.

The documentation also lists ways to make this work (such as having more powerful scope types).

I see. I looked through the docs but it wasn't clear what you referred too. How would one change base::AutoLock in this case? Or do the developers need to avoid unique_ptr, or is there something else?

I'll give you some time to reply before I reland, but I can't see a bug here. For the next time, as already pointed out, give the involved people some time to reply before you revert (unless it's a crash). We have to be able to continue working on the compiler without having Google revert changes all the time because we produce new warnings.

We've seen reports here of various breakages from two different projects in short time after your patch. The general policy is to revert to green, so I think that was the right call. Typically, new warnings are introduced behind new flags so that users can adopt them incrementally, though I realize that might be tricky in this instance. In any case, perhaps we should give other projects some time to assess the impact of this before relanding?

This also seems like the kind of disruptive change we'd want to announce on Discourse, as discussed in Aaron's recent RFC. Probably should be mentioned in the "Potentially Breaking Changes" section of the release notes too.

[aaronpuchert]

• Pass til::LiteralPtr *Self for automatic object destructors into handling of require_capability and locks_excluded attributes.
• Add [[maybe_unused]] attribute to silence -Wunused-variable warinng in Release builds.
• A few more tests.

[aaronpuchert]

In D129755#3843144, @gulfem wrote:

We also started seeing -Wthread-safety-precise error in our Fuchsia code.
https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8800959115965408001/overview
I'm trying to verify with our team whether it is a false positive, but I just wanted to give you heads up!

This was a genuine bug which I believe to be fixed now (see the comments below). Thanks for the report!

I'd appreciate if you could test this, but it's not necessary as I could reproduce the bug and the test seems to show it fixed.

In D129755#3843716, @hans wrote:

Thanks for looking into this so quickly.

I'd still call it a false positive since the mutex is in fact being held. However I now understand that this is due to a pre-existing limitation in the analysis.

It's a deliberate limitation. In the compiler we can't really look into function definitions (for a start, they might not be visible), so we have to rely on annotations.

How would you suggest the developers work around this?

Perhaps you could have the constructor call directly inline in the same function, and use move construction? Like

cache = grpc_core::MakeRefCounted<TlsSessionKeyLoggerCache>(TlsSessionKeyLoggerCache());

Then the default constructor is called where we know the lock to be held. With a bit of luck the move is optimized out.

Another solution would be to specialize grpc_core::MakeRefCounted<TlsSessionKeyLoggerCache> and add the attribute on the specialization, but then you'll have to duplicate the implementation.

Lastly, of course __attribute__((no_thread_safety_analysis)) on grpc_core::MakeRefCounted should also do the job.

I looked through the docs but it wasn't clear what you referred too.

The section "No inlining" has this example:

Thread safety analysis is strictly intra-procedural, just like ordinary type checking. It relies only on the declared attributes of a function, and will not attempt to inline any method calls. As a result, code such as the following will not work:

template<class T>
class AutoCleanup {
  T* object;
  void (T::*mp)();

public:
  AutoCleanup(T* obj, void (T::*imp)()) : object(obj), mp(imp) { }
  ~AutoCleanup() { (object->*mp)(); }
};

Mutex mu;
void foo() {
  mu.Lock();
  AutoCleanup<Mutex>(&mu, &Mutex::Unlock);
  // ...
}  // Warning, mu is not unlocked.

How would one change base::AutoLock in this case? Or do the developers need to avoid unique_ptr, or is there something else?

Composing scoped locks through generic ADTs is incompatible with Thread safety analysis, as operations on the scopes are now hidden behind the ADT. So yes, anything like unique_ptr<AutoLock> should be avoided.

I've been working on supporting more complex RAII types that allow relocking (D49885) and deferred locking (D81332). This should allow to replace something like optional<AutoLock>. In your example it's a bit more difficult, as the AutoLock* is then passed to some object that's returned, so it likely leaves the function. Such data-flow-like patterns are not supported yet and will probably not be for a long time. (@delesley pointed out in a talk years ago that this would require some kind of dependent type system.) So my advice here would simply be to add the previously mentioned no_thread_safety_analysis attribute. For now this is simply intractable, so you're not losing anything.

We've seen reports here of various breakages from two different projects in short time after your patch. The general policy is to revert to green, so I think that was the right call. Typically, new warnings are introduced behind new flags so that users can adopt them incrementally, though I realize that might be tricky in this instance. In any case, perhaps we should give other projects some time to assess the impact of this before relanding?

For conceptual changes I'd always put them behind a new flag, in fact we have -Wthread-safety-beta for this. We are aware that this might break some code, but the previous behavior was so strange that it would be a pity to leave it. (We were using annotations on a function that doesn't do anything interesting and isn't even called.)

The two cases that you reported boil down to us previously not looking into some constructor calls, which is arguably just an inconsistency. Here is what it boils down to:

struct X {
    X() ANNOTATION();
};

void f() {
  X x;       // Annotation was always taken into account.
  X x = X(); // Annotation was previously taken into account only with -std=c++17 (guaranteed copy elisision) or newer, but not before.
  X();       // Annotation was previously not taken into account.
  new X();   // Dito.
}

This also seems like the kind of disruptive change we'd want to announce on Discourse, as discussed in Aaron's recent RFC. Probably should be mentioned in the "Potentially Breaking Changes" section of the release notes too.

I'll extend the release note to mention that as a side effect this looks into more constructor calls and might thus produce additional warnings consistent with how we behave otherwise.

I'm not sure if this counts as "potentially breaking change" since it only changes an optional diagnostic. But I'd defer to @aaron.ballman on this.

I can also post to Discourse, but Thread Safety Analysis is a bit of a niche topic and previous posts that I've seen didn't attract much attention.

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

1620

The previous change had

warning: calling function '~DestructorRequires' requires holding mutex 'mu' exclusively
note: found near match 'rd.mu'

here which is now gone. This is basically the issue posted by @gulfem.

1630–1631

Similar to the reported issue, here we were not reporting warnings. Underlying reason is the same missing substitution.

[gulfem]

In D129755#3844059, @aaronpuchert wrote:

In D129755#3843144, @gulfem wrote:

We also started seeing -Wthread-safety-precise error in our Fuchsia code.
https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8800959115965408001/overview
I'm trying to verify with our team whether it is a false positive, but I just wanted to give you heads up!

This was a genuine bug which I believe to be fixed now (see the comments below). Thanks for the report!

I'd appreciate if you could test this, but it's not necessary as I could reproduce the bug and the test seems to show it fixed.

I verified that it fixes the following error in our zircon kernel, and thanks for the fix!

[89054/268252] CXX kernel_x64/obj/zircon/kernel/vm/vm.vm_cow_pages.cc.o
FAILED: kernel_x64/obj/zircon/kernel/vm/vm.vm_cow_pages.cc.o
../../../recipe_cleanup/clang72pd81kv/bin/clang++ -MD -MF kernel_x64/obj/zircon/kernel/vm/vm.vm_cow_pages.cc.o.d -o kernel_x64/obj/zircon/kernel/vm/vm.vm_cow_pages.cc.o -D_LIBCPP_DISABLE_VISIBILITY...
../../zircon/kernel/vm/vm_cow_pages.cc:6400:3: error: calling function '~VmoCursor' requires holding mutex 'lock_' exclusively [-Werror,-Wthread-safety-precise]
  }
  ^
../../zircon/kernel/vm/vm_cow_pages.cc:6400:3: note: found near match 'cursor.lock_'
../../zircon/kernel/vm/vm_cow_pages.cc:6483:10: error: calling function '~VmoCursor' requires holding mutex 'lock_' exclusively [-Werror,-Wthread-safety-precise]
  return total_pages_discarded;
         ^
../../zircon/kernel/vm/vm_cow_pages.cc:6483:10: note: found near match 'cursor.lock_'

[aaron.ballman]

In D129755#3842744, @hans wrote:

I'll give you some time to reply before I reland, but I can't see a bug here. For the next time, as already pointed out, give the involved people some time to reply before you revert (unless it's a crash). We have to be able to continue working on the compiler without having Google revert changes all the time because we produce new warnings.

We've seen reports here of various breakages from two different projects in short time after your patch. The general policy is to revert to green, so I think that was the right call.

I don't think it was the wrong call per se, but it was an aggressive revert IMO. Chromium tends to revert changes very quickly and without discussion, and I'm hoping we can change that slightly to include more up-front discussion before a revert. It's one thing to revert when the patch author isn't responsive, but reverting immediately and without discussion when the commit message explicitly states there is a change in behavior isn't how I would expect the revert policy to be interpreted.

Typically, new warnings are introduced behind new flags so that users can adopt them incrementally, though I realize that might be tricky in this instance. In any case, perhaps we should give other projects some time to assess the impact of this before relanding?

Other projects won't have the time to assess the impact of these changes because the changes were reverted (almost nobody applies one-off patches to try them out without there being something else driving that experiment). I think the changes should be re-landed so people can get us that feedback.

This also seems like the kind of disruptive change we'd want to announce on Discourse, as discussed in Aaron's recent RFC. Probably should be mentioned in the "Potentially Breaking Changes" section of the release notes too.

+1!

[aaron.ballman]

Adding the clang-vendors group for awareness of potential disruptions from these changes.

[hans]

In D129755#3849540, @aaron.ballman wrote:

In D129755#3842744, @hans wrote:

I'll give you some time to reply before I reland, but I can't see a bug here. For the next time, as already pointed out, give the involved people some time to reply before you revert (unless it's a crash). We have to be able to continue working on the compiler without having Google revert changes all the time because we produce new warnings.

We've seen reports here of various breakages from two different projects in short time after your patch. The general policy is to revert to green, so I think that was the right call.

I don't think it was the wrong call per se, but it was an aggressive revert IMO. Chromium tends to revert changes very quickly and without discussion, and I'm hoping we can change that slightly to include more up-front discussion before a revert. It's one thing to revert when the patch author isn't responsive, but reverting immediately and without discussion when the commit message explicitly states there is a change in behavior isn't how I would expect the revert policy to be interpreted.

In general I think us reverting fast is a good thing. Reverts and relands are cheap, and reverting early prevents more people from being exposed to breakages, which are expensive to investigate. Minimizing the amount time where the source tree is in a bad state seems like a good thing to me, and reverts also tend to become harder and more disruptive with time as more work lands on top.

I certainly don't want us to come across as aggressive though, and since for this patch it turned out that the new false positives were expected, in hindsight I probably shouldn't have been so quick to revert.

Since the bug that Gulfem reported is fixed, relanding sounds good to me.

[aaron.ballman]

In D129755#3850175, @hans wrote:

In D129755#3849540, @aaron.ballman wrote:

In D129755#3842744, @hans wrote:

I'll give you some time to reply before I reland, but I can't see a bug here. For the next time, as already pointed out, give the involved people some time to reply before you revert (unless it's a crash). We have to be able to continue working on the compiler without having Google revert changes all the time because we produce new warnings.

We've seen reports here of various breakages from two different projects in short time after your patch. The general policy is to revert to green, so I think that was the right call.

I don't think it was the wrong call per se, but it was an aggressive revert IMO. Chromium tends to revert changes very quickly and without discussion, and I'm hoping we can change that slightly to include more up-front discussion before a revert. It's one thing to revert when the patch author isn't responsive, but reverting immediately and without discussion when the commit message explicitly states there is a change in behavior isn't how I would expect the revert policy to be interpreted.

In general I think us reverting fast is a good thing. Reverts and relands are cheap, and reverting early prevents more people from being exposed to breakages, which are expensive to investigate. Minimizing the amount time where the source tree is in a bad state seems like a good thing to me, and reverts also tend to become harder and more disruptive with time as more work lands on top.

In general, yes. Quick reverts for real problems are definitely appreciated (and our general policy to boot)!

I certainly don't want us to come across as aggressive though, and since for this patch it turned out that the new false positives were expected, in hindsight I probably shouldn't have been so quick to revert.

Yup! I'm mostly just asking to raise awareness on the review before reverting in cases where it's not a build error (or other stop-the-world-this-is-obviously-wrong situation) and none of our community bots have gone red. Chromium is a downstream and we definitely want to make sure the ToT is usable for downstreams regardless of when they pull code, but downstreams should not have an expectation that any disturbance to their code base qualifies as something to revert without discussion. (We've had similar tension from in-tree downstreams as well, such as libc++ and lldb, so we're trying to find a happy medium between "instant revert of conforming changes without warning" and "leaving the ToT in a broken state too long and building up frustrating debt from that.")

Since the bug that Gulfem reported is fixed, relanding sounds good to me.

Thanks!

[aaronpuchert]

Add sentence to release notes that we now look into more constructor calls and that this could lead to additional warnings being emitted.

[aaronpuchert]

@hans, where you able to fix or work around the warnings? I'd like to land this again, but if you need more time it can also wait a bit.

[hans]

In D129755#3850731, @aaronpuchert wrote:

@hans, where you able to fix or work around the warnings? I'd like to land this again, but if you need more time it can also wait a bit.

The work-around for gRPC is still pending, but we can turn off the warning for that library if we need to, so no need to wait.

[aaronpuchert]

@aaron.ballman, would like some feedback on the release notes. Should I additionally write something under "Potentially Breaking Changes", or is it enough to mention this under "Improvements to Clang's diagnostics"? Though I guess we could also add this later on if we get more complaints that this breaks things.

[thesamesam]

In D129755#3853809, @aaronpuchert wrote:

@aaron.ballman, would like some feedback on the release notes. Should I additionally write something under "Potentially Breaking Changes", or is it enough to mention this under "Improvements to Clang's diagnostics"? Though I guess we could also add this later on if we get more complaints that this breaks things.

I think it's sufficient in "Improvements to Clang's diagnostics". We're not looking to document every externally observable change in "Potentially Breaking Changes", IMO. Thanks for asking. We could definitely shift it later if needed.

[aaron.ballman]

In D129755#3853904, @thesamesam wrote:

In D129755#3853809, @aaronpuchert wrote:

@aaron.ballman, would like some feedback on the release notes. Should I additionally write something under "Potentially Breaking Changes", or is it enough to mention this under "Improvements to Clang's diagnostics"? Though I guess we could also add this later on if we get more complaints that this breaks things.

I think it's sufficient in "Improvements to Clang's diagnostics". We're not looking to document every externally observable change in "Potentially Breaking Changes", IMO. Thanks for asking. We could definitely shift it later if needed.

+1 to this -- I think it's defensible either way. It's not that this is a *breaking* change, but it is a potentially disruptive one. If we get many more folks raising questions about code changes, we can move the release note then. I think what you have now is fine by me.

[rupprecht]

I'm seeing a fair number of breakages from this patch (not really sure how many we truly have, I've hit ~5-10 so far in widely used libraries, but I suspect we have far more in the long tail). They're all valid (most are just adding missing thread safety annotations, etc., so far one breakage caught a real bug), but it would help if we could disable just these new ones. How invasive/annoying would it be to carve out a subwarning for this category of bugs and call it something like -Wthread-safety-elision?

[aaronpuchert]

In D129755#3865342, @rupprecht wrote:

I'm seeing a fair number of breakages from this patch (not really sure how many we truly have, I've hit ~5-10 so far in widely used libraries, but I suspect we have far more in the long tail).

That might be an argument to move up our release note to "Potentially breaking changes", but we can also decide that around the next release.

How invasive/annoying would it be to carve out a subwarning for this category of bugs and call it something like -Wthread-safety-elision?

Are you seeing warnings because of the different treatment of copy-elided construction, or because we've started to consider CXXConstructorCalls outside of the initializer of a DeclStmt? In any event, this change is quite fundamental and doesn't make it easy to selectively switch back to the old behavior:

• For copy elision we'd need to stop adding the scoped capability in BuildLockset::handleCall, then in BuildLockset::VisitDeclStmt add back in the code building the fake constructor call. Then there might be inconsistencies.
• If it's the additional constructor calls, we'd have to change BuildLockset::VisitCXXConstructExpr and BuildLockset::VisitDeclStmt so that we continue to only consider some constructor calls. This would be easier, but would result in a rather odd warning flag.

[rupprecht]

In D129755#3866213, @aaronpuchert wrote:

Are you seeing warnings because of the different treatment of copy-elided construction, or because we've started to consider CXXConstructorCalls outside of the initializer of a DeclStmt?

I'm not familiar with internal Clang APIs so I'm not entirely sure how to answer that, but I can share sketches of the breakages I've addressed so far:

1. (2x) Returning a MutexLock-like structure, e.g.

MutexLock Lock() {
  return MutexLock(&mu);
}

this is documented in the test as a known false positive. Is that something you plan to address some day, or is it beyond the limits of thread safety analysis? (Or theoretically possible, but then compile times would be too much if you have to do more analysis?) Anyway, I applied __attribute__((no_thread_safety_analysis)) to the method.

2. (3x) deferred/offloaded unlocking, e.g.

void Func() {
  auto *lock = new MutexLock(&mu);
  auto cleanup = [lock]() { delete lock; };
  cleanup();
} // Error that mu is not unlocked

I assume this is beyond the scope of thread safety analysis -- cleanup() in the internal case actually happens in a different TU (the "cleanup" gets passed to some generic scheduler thingy), which is even more complex than this trivial example. In one of the cases the unlocking would happen on some other thread, which I guess is a little suspicious, but should still be guaranteed to execute (I didn't dig _too_ deep there). Anyway, I also suppressed analysis here.

3. Missed lock annotation on the constructor, take 1

struct Foo {
  explicit Foo(Mutex *mu) EXCLUSIVE_LOCKS_REQUIRED(mu);
};

Something Func() {
  SomethingLikeMutexLock lock(&mu);
  lock.Release(); // !!!
  return Get(Foo(&mu));
}

Glaringly obvious bug that wasn't being caught before this patch. Moving the call to Foo(&mu) earlier while the lock is still held fixes the bug.

4. Missed lock annotation on the constructor, take 2

struct Foo {
  explicit Foo(Mutex *mu) EXCLUSIVE_LOCKS_REQUIRED(mu);
};
void X() {
  auto lock = MutexLock(&mu);
  auto f = MakeFoo();
}
Foo Y() {
  return Foo(&mu);
}

X grabs the lock, but Y warns that we don't have mu held. In this case, mu->AssertHeld() suffices (the mutex pattern is a little more complicated, and the pattern is used elsewhere, but was missed in this method).

5. Alternate std::make_unique implementation (this one still has me puzzled)

template <typename T, typename... Args>
inline std::unique_ptr<T> MyMakeUnique(Args &&...args) {
  return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
}

static Mutex y_mu;

class Z {
 public:
  Z() EXCLUSIVE_LOCKS_REQUIRED(y_mu) {}
};

std::unique_ptr<Z> Y() {
  auto lock = MutexLock(&y_mu);
  return MyMakeUnique<Z>();
}

Returning std::make_unique<Z>() instead of the custom helper works. Why? No idea. MyMakeUnique is a c++11 compatibility helper -- std::unique_ptr is in c++11 but std::make_unique is only in c++14 -- and fortunately this code doesn't need it anymore. The implementation seems to be identical to the one in libc++, so my best guess is threading analysis has some special handling for some std:: methods.

I might have a better answer in a day or two of how widespread this is beyond just the core files that seem to make the world break when we enable this. We can just fix the bugs it if it's only a few of them, but it might be difficult if we have too many.

[aaronpuchert]

Thanks for the detailed write-up, very much appreciated.

In D129755#3866887, @rupprecht wrote:

1. (2x) Returning a MutexLock-like structure, e.g.

MutexLock Lock() {
  return MutexLock(&mu);
}

this is documented in the test as a known false positive. Is that something you plan to address some day, or is it beyond the limits of thread safety analysis? (Or theoretically possible, but then compile times would be too much if you have to do more analysis?)

I'm planning to address this, but don't have a good idea how to solve it yet. Conceptually it should definitely be possible though.

Anyway, I applied __attribute__((no_thread_safety_analysis)) to the method.

This seems about right for now. I filed bug 58480, which you could link to in a comment or subscribe to for being notified of a fix.

2. (3x) deferred/offloaded unlocking, e.g.

void Func() {
  auto *lock = new MutexLock(&mu);
  auto cleanup = [lock]() { delete lock; };
  cleanup();
} // Error that mu is not unlocked

I assume this is beyond the scope of thread safety analysis -- cleanup() in the internal case actually happens in a different TU (the "cleanup" gets passed to some generic scheduler thingy), which is even more complex than this trivial example. In one of the cases the unlocking would happen on some other thread, which I guess is a little suspicious, but should still be guaranteed to execute (I didn't dig _too_ deep there). Anyway, I also suppressed analysis here.

Yes, this is pretty much out of scope, at least for the foreseeable future. This is also documented: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-inlining.

3. Missed lock annotation on the constructor, take 1

struct Foo {
  explicit Foo(Mutex *mu) EXCLUSIVE_LOCKS_REQUIRED(mu);
};

Something Func() {
  SomethingLikeMutexLock lock(&mu);
  lock.Release(); // !!!
  return Get(Foo(&mu));
}

Glaringly obvious bug that wasn't being caught before this patch. Moving the call to Foo(&mu) earlier while the lock is still held fixes the bug.

Glad to hear that!

4. Missed lock annotation on the constructor, take 2

struct Foo {
  explicit Foo(Mutex *mu) EXCLUSIVE_LOCKS_REQUIRED(mu);
};
void X() {
  auto lock = MutexLock(&mu);
  auto f = MakeFoo();
}
Foo Y() {
  return Foo(&mu);
}

X grabs the lock, but Y warns that we don't have mu held. In this case, mu->AssertHeld() suffices (the mutex pattern is a little more complicated, and the pattern is used elsewhere, but was missed in this method).

Possibly you could also "propagate" the lock requirement onto MakeFoo like this:

Foo MakeFoo() EXCLUSIVE_LOCKS_REQUIRED(mu) {
  return Foo(&mu);
}

But sometimes this isn't possible for encapsulation reasons, in which case AssertHeld is indeed a good workaround.

5. Alternate std::make_unique implementation (this one still has me puzzled)

template <typename T, typename... Args>
inline std::unique_ptr<T> MyMakeUnique(Args &&...args) {
  return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
}

static Mutex y_mu;

class Z {
 public:
  Z() EXCLUSIVE_LOCKS_REQUIRED(y_mu) {}
};

std::unique_ptr<Z> Y() {
  auto lock = MutexLock(&y_mu);
  return MyMakeUnique<Z>();
}

Returning std::make_unique<Z>() instead of the custom helper works. Why? No idea. MyMakeUnique is a c++11 compatibility helper -- std::unique_ptr is in c++11 but std::make_unique is only in c++14 -- and fortunately this code doesn't need it anymore. The implementation seems to be identical to the one in libc++, so my best guess is threading analysis has some special handling for some std:: methods.

We had a similar case earlier in the thread. Basically the problem is that MyMakeUnique calls the constructor of Z without holding the lock (statically). I don't think that we have special handling for std::, but it's possible that the warning is suppressed in system headers.

Apart from the solutions mentioned above, manually inlining the make_unique/MyMakeUnique should do the job, i.e.

std::unique_ptr<Z> Y() {
  auto lock = MutexLock(&y_mu);
  return std::unique_ptr<Z>(new Z());
}

Of course this has downsides of its own, but we can't really do "inlining" in a compiler warning and so it's either the warning or (slightly) more verbose code.

I might have a better answer in a day or two of how widespread this is beyond just the core files that seem to make the world break when we enable this. We can just fix the bugs it if it's only a few of them, but it might be difficult if we have too many.

The good news is that for now we've only seen the second category of issues, for which a flag to restore the old behavior would be feasible. I can't say for certain whether that would make all the issues here disappear, but it definitely seems so.

[rupprecht]

In D129755#3869081, @aaronpuchert wrote:

In D129755#3866887, @rupprecht wrote:

I might have a better answer in a day or two of how widespread this is beyond just the core files that seem to make the world break when we enable this. We can just fix the bugs it if it's only a few of them, but it might be difficult if we have too many.

The good news is that for now we've only seen the second category of issues, for which a flag to restore the old behavior would be feasible. I can't say for certain whether that would make all the issues here disappear, but it definitely seems so.

We're about done with our cleanup. Our fix count is at 34, and should be final unless there are surprises. I'm not sure if others would benefit from having this warning pushed to a subflag, but we don't need it anymore.

While making some fixes, I ran across a strange false positive which I filed as https://github.com/llvm/llvm-project/issues/58535. I think it's another situation where the code is too complex for thread analysis to be feasible, but I also didn't see it documented as one of the common cases that isn't supported.

clang/docs/ReleaseNotes.rst

- Clang will now diagnose an overload set where a candidate has a constraint that
   subsuming constraint fails, which would result in a less-specific overload to
   be selected.
 - Add a fix-it hint for the ``-Wdefaulted-function-deleted`` warning to
   explicitly delete the function.
 - Fixed an accidental duplicate diagnostic involving the declaration of a
   function definition without a prototype which is preceded by a static
   declaration of the function with a prototype. Fixes
   `Issue 58181 <https://github.com/llvm/llvm-project/issues/58181>`_.
+- Copy-elided initialization of lock scopes is now handled differently in
+  ``-Wthread-safety-analysis``: annotations on the move constructor are no
+  longer taken into account, in favor of annotations on the function returning
+  the lock scope by value. This could result in new warnings if code depended
+  on the previous undocumented behavior. As a side effect of this change,
+  constructor calls outside of initializer expressions are no longer ignored,
+  which can result in new warnings (or make existing warnings disappear).
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 - It's now possible to set the crash diagnostics directory through
   the environment variable ``CLANG_CRASH_DIAGNOSTICS_DIR``.
   The ``-fcrash-diagnostics-dir`` flag takes precedence.
 - When using header modules, inclusion of a private header and violations of
   the `use-declaration rules

clang/docs/ThreadSafetyAnalysis.rst

 locking, in which a capability is acquired in the constructor, and released in
 the destructor.  Such classes require special handling because the constructor
 and destructor refer to the capability via different names; see the
 ``MutexLocker`` class in :ref:`mutexheader`, below.
 
 Scoped capabilities are treated as capabilities that are implicitly acquired
 on construction and released on destruction. They are associated with
 the set of (regular) capabilities named in thread safety attributes on the
-constructor. Acquire-type attributes on other member functions are treated as
+constructor or function returning them by value (using C++17 guaranteed copy
+elision). Acquire-type attributes on other member functions are treated as
 applying to that set of associated capabilities, while ``RELEASE`` implies that
 a function releases all associated capabilities in whatever mode they're held.
 
 
 TRY_ACQUIRE(<bool>, ...), TRY_ACQUIRE_SHARED(<bool>, ...)
 ---------------------------------------------------------
 
 *Previously:* ``EXCLUSIVE_TRYLOCK_FUNCTION``, ``SHARED_TRYLOCK_FUNCTION``
public:
 
     // Assume mu is held in shared mode, implicitly acquire *this and associate it with mu.
     MutexLocker(Mutex *mu, adopt_lock_t, shared_lock_t) REQUIRES_SHARED(mu)
       : mut(mu), locked(true) {}
 
     // Assume mu is not held, implicitly acquire *this and associate it with mu.
     MutexLocker(Mutex *mu, defer_lock_t) EXCLUDES(mu) : mut(mu), locked(false) {}
 
+    // Same as constructors, but without tag types. (Requires C++17 copy elision.)
+    static MutexLocker Lock(Mutex *mu) ACQUIRE(mu);
+    static MutexLocker Adopt(Mutex *mu) REQUIRES(mu);
+    static MutexLocker ReaderLock(Mutex *mu) ACQUIRE_SHARED(mu);
+    static MutexLocker AdoptReaderLock(Mutex *mu) REQUIRES_SHARED(mu);
+    static MutexLocker DeferLock(Mutex *mu) EXCLUDES(mu);
+
     // Release *this and all associated mutexes, if they are still held.
     // There is no warning if the scope was already unlocked before.
     ~MutexLocker() RELEASE() {
       if (locked)
         mut->GenericUnlock();
     }
 
     // Acquire all associated mutexes exclusively.

clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h

 #include "clang/Analysis/Analyses/ThreadSafetyTIL.h"
 #include "clang/Analysis/Analyses/ThreadSafetyTraverse.h"
 #include "clang/Analysis/Analyses/ThreadSafetyUtil.h"
 #include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/PointerIntPair.h"
+#include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
 #include <sstream>
 #include <string>
 #include <utility>
 #include <vector>
 
 namespace clang {
public:
   struct CallingContext {
     // The previous context; or 0 if none.
     CallingContext  *Prev;
 
     // The decl to which the attr is attached.
     const NamedDecl *AttrDecl;
 
     // Implicit object argument -- e.g. 'this'
-    const Expr *SelfArg = nullptr;
+    llvm::PointerUnion<const Expr *, til::SExpr *> SelfArg = nullptr;
 
     // Number of funArgs
     unsigned NumArgs = 0;
 
     // Function arguments
     const Expr *const *FunArgs = nullptr;
 
     // is Self referred to with -> or .?
     bool SelfArrow = false;
 
     CallingContext(CallingContext *P, const NamedDecl *D = nullptr)
         : Prev(P), AttrDecl(D) {}
   };
 
   SExprBuilder(til::MemRegionRef A) : Arena(A) {
     // FIXME: we don't always have a self-variable.
     SelfVar = new (Arena) til::Variable(nullptr);
     SelfVar->setKind(til::Variable::VK_SFun);
   }
 
   // Translate a clang expression in an attribute to a til::SExpr.
   // Constructs the context from D, DeclExp, and SelfDecl.
   CapabilityExpr translateAttrExpr(const Expr *AttrExp, const NamedDecl *D,
-                                   const Expr *DeclExp, VarDecl *SelfD=nullptr);
+                                   const Expr *DeclExp,
+                                   til::SExpr *Self = nullptr);
 
   CapabilityExpr translateAttrExpr(const Expr *AttrExp, CallingContext *Ctx);
 
+  // Translate a variable reference.
+  til::LiteralPtr *createVariable(const VarDecl *VD);
+
+  // Create placeholder for this: we don't know the VarDecl on construction yet.
+  std::pair<til::LiteralPtr *, StringRef>
+  createThisPlaceholder(const Expr *Exp);
+
   // Translate a clang statement or expression to a TIL expression.
   // Also performs substitution of variables; Ctx provides the context.
   // Dispatches on the type of S.
   til::SExpr *translate(const Stmt *S, CallingContext *Ctx);
   til::SCFG  *buildCFG(CFGWalker &Walker);
 
   til::SExpr *lookupStmt(const Stmt *S);
 

clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h

private:
   SExpr *Definition;
 
   // The clang declaration for this variable.
   const ValueDecl *Cvdecl = nullptr;
 };
 
 /// Placeholder for an expression that has not yet been created.
 /// Used to implement lazy copy and rewriting strategies.
 class Future : public SExpr {
 public:
   enum FutureStatus {
     FS_pending,
     FS_evaluating,
     FS_done
   };
 
   Future() : SExpr(COP_Future) {}
typename V::R_SExpr Literal::traverse(V &Vs, typename V::R_Ctx Ctx) {
   }
   return Vs.reduceLiteral(*this);
 }
 
 /// A Literal pointer to an object allocated in memory.
 /// At compile time, pointer literals are represented by symbolic names.
 class LiteralPtr : public SExpr {
 public:
-  LiteralPtr(const ValueDecl *D) : SExpr(COP_LiteralPtr), Cvdecl(D) {
-    assert(D && "ValueDecl must not be null");
-  }
+  LiteralPtr(const ValueDecl *D) : SExpr(COP_LiteralPtr), Cvdecl(D) {}
   LiteralPtr(const LiteralPtr &) = default;
 
   static bool classof(const SExpr *E) { return E->opcode() == COP_LiteralPtr; }
 
   // The clang declaration for the value that this pointer points to.
   const ValueDecl *clangDecl() const { return Cvdecl; }
+  void setClangDecl(const ValueDecl *VD) { Cvdecl = VD; }
 
   template <class V>
   typename V::R_SExpr traverse(V &Vs, typename V::R_Ctx Ctx) {
     return Vs.reduceLiteralPtr(*this);
   }
 
   template <class C>
   typename C::CType compare(const LiteralPtr* E, C& Cmp) const {
+    if (!Cvdecl || !E->Cvdecl)
+      return Cmp.comparePointers(this, E);
     return Cmp.comparePointers(Cvdecl, E->Cvdecl);
   }
 
 private:
   const ValueDecl *Cvdecl;
 };
 
 /// A function -- a.k.a. lambda abstraction.

clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h

else {
         SS << "#vref";
         return;
       }
     }
     SS << "#lit";
   }
 
   void printLiteralPtr(const LiteralPtr *E, StreamType &SS) {
-    SS << E->clangDecl()->getNameAsString();
+    if (const NamedDecl *D = E->clangDecl())
+      SS << D->getNameAsString();
+    else
+      SS << "<temporary>";
   }
 
   void printVariable(const Variable *V, StreamType &SS, bool IsVarDecl=false) {
     if (CStyle && V->kind() == Variable::VK_SFun)
       SS << "this";
     else
       SS << V->name() << V->id();
   }

clang/lib/Analysis/ThreadSafety.cpp

public:
 
   void addLock(FactSet &FSet, std::unique_ptr<FactEntry> Entry,
                bool ReqAttr = false);
   void removeLock(FactSet &FSet, const CapabilityExpr &CapE,
                   SourceLocation UnlockLoc, bool FullyRemove, LockKind Kind);
 
   template <typename AttrType>
   void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp,
-                   const NamedDecl *D, VarDecl *SelfDecl = nullptr);
+                   const NamedDecl *D, til::SExpr *Self = nullptr);
 
   template <class AttrType>
   void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp,
                    const NamedDecl *D,
                    const CFGBlock *PredBlock, const CFGBlock *CurrBlock,
                    Expr *BrE, bool Neg);
 
   const CallExpr* getTrylockCallExpr(const Stmt *Cond, LocalVarContext C,

 
 bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr &CapE) {
   const threadSafety::til::SExpr *SExp = CapE.sexpr();
   assert(SExp && "Null expressions should be ignored");
 
   if (const auto *LP = dyn_cast<til::LiteralPtr>(SExp)) {
     const ValueDecl *VD = LP->clangDecl();
     // Variables defined in a function are always inaccessible.
-    if (!VD->isDefinedOutsideFunctionOrMethod())
+    if (!VD || !VD->isDefinedOutsideFunctionOrMethod())
       return false;
     // For now we consider static class members to be inaccessible.
     if (isa<CXXRecordDecl>(VD->getDeclContext()))
       return false;
     // Global variables are always in scope.
     return true;
   }
 
void ThreadSafetyAnalyzer::removeLock(FactSet &FSet, const CapabilityExpr &Cp,
   LDat->handleUnlock(FSet, FactMan, Cp, UnlockLoc, FullyRemove, Handler);
 }
 
 /// Extract the list of mutexIDs from the attribute on an expression,
 /// and push them onto Mtxs, discarding any duplicates.
 template <typename AttrType>
 void ThreadSafetyAnalyzer::getMutexIDs(CapExprSet &Mtxs, AttrType *Attr,
                                        const Expr *Exp, const NamedDecl *D,
-                                       VarDecl *SelfDecl) {
+                                       til::SExpr *Self) {
   if (Attr->args_size() == 0) {
     // The mutex held is the "this" object.
-    CapabilityExpr Cp = SxBuilder.translateAttrExpr(nullptr, D, Exp, SelfDecl);
+    CapabilityExpr Cp = SxBuilder.translateAttrExpr(nullptr, D, Exp, Self);
     if (Cp.isInvalid()) {
       warnInvalidLock(Handler, nullptr, D, Exp, Cp.getKind());
       return;
     }
     //else
     if (!Cp.shouldIgnore())
       Mtxs.push_back_nodup(Cp);
     return;
   }
 
   for (const auto *Arg : Attr->args()) {
-    CapabilityExpr Cp = SxBuilder.translateAttrExpr(Arg, D, Exp, SelfDecl);
+    CapabilityExpr Cp = SxBuilder.translateAttrExpr(Arg, D, Exp, Self);
     if (Cp.isInvalid()) {
       warnInvalidLock(Handler, nullptr, D, Exp, Cp.getKind());
       continue;
     }
     //else
     if (!Cp.shouldIgnore())
       Mtxs.push_back_nodup(Cp);
   }

 /// 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> {
   friend class ThreadSafetyAnalyzer;
 
   ThreadSafetyAnalyzer *Analyzer;
   FactSet FSet;
+  /// Maps constructed objects to `this` placeholder prior to initialization.
+  llvm::SmallDenseMap<const Expr *, til::LiteralPtr *> ConstructedObjects;
   LocalVariableMap::Context LVarCtx;
   unsigned CtxIndex;
 
   // helper functions
   void warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp, AccessKind AK,
                           Expr *MutexExp, ProtectedOperationKind POK,
-                          SourceLocation Loc);
-  void warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, Expr *MutexExp);
+                          til::LiteralPtr *Self, SourceLocation Loc);
+  void warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, Expr *MutexExp,
+                       til::LiteralPtr *Self, SourceLocation Loc);
 
   void checkAccess(const Expr *Exp, AccessKind AK,
                    ProtectedOperationKind POK = POK_VarAccess);
   void checkPtAccess(const Expr *Exp, AccessKind AK,
                      ProtectedOperationKind POK = POK_VarAccess);
 
-  void handleCall(const Expr *Exp, const NamedDecl *D, VarDecl *VD = nullptr);
+  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)
       : ConstStmtVisitor<BuildLockset>(), Analyzer(Anlzr), FSet(Info.EntrySet),
         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);
 };
 
 } // namespace
 
 /// Warn if the LSet does not contain a lock sufficient to protect access
 /// of at least the passed in AccessKind.
 void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp,
                                       AccessKind AK, Expr *MutexExp,
                                       ProtectedOperationKind POK,
+                                      til::LiteralPtr *Self,
                                       SourceLocation Loc) {
   LockKind LK = getLockKindFromAccessKind(AK);
 
-  CapabilityExpr Cp = Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp);
+  CapabilityExpr Cp =
+      Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
   if (Cp.isInvalid()) {
     warnInvalidLock(Analyzer->Handler, MutexExp, D, Exp, Cp.getKind());
     return;
   } else if (Cp.shouldIgnore()) {
     return;
   }
 
   if (Cp.negative()) {
void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp,
   if (NoError && LDat && !LDat->isAtLeast(LK)) {
     Analyzer->Handler.handleMutexNotHeld(Cp.getKind(), D, POK, Cp.toString(),
                                          LK, Loc);
   }
 }
 
 /// Warn if the LSet contains the given lock.
 void BuildLockset::warnIfMutexHeld(const NamedDecl *D, const Expr *Exp,
-                                   Expr *MutexExp) {
-  CapabilityExpr Cp = Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp);
+                                   Expr *MutexExp, til::LiteralPtr *Self,
+                                   SourceLocation Loc) {
+  CapabilityExpr Cp =
+      Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
   if (Cp.isInvalid()) {
     warnInvalidLock(Analyzer->Handler, MutexExp, D, Exp, Cp.getKind());
     return;
   } else if (Cp.shouldIgnore()) {
     return;
   }
 
   const FactEntry *LDat = FSet.findLock(Analyzer->FactMan, Cp);
   if (LDat) {
     Analyzer->Handler.handleFunExcludesLock(Cp.getKind(), D->getNameAsString(),
-                                            Cp.toString(), Exp->getExprLoc());
+                                            Cp.toString(), Loc);
   }
 }
 
 /// Checks guarded_by and pt_guarded_by attributes.
 /// Whenever we identify an access (read or write) to a DeclRefExpr that is
 /// marked with guarded_by, we must ensure the appropriate mutexes are held.
 /// Similarly, we check if the access is to an expression that dereferences
 /// a pointer marked with pt_guarded_by.
void BuildLockset::checkAccess(const Expr *Exp, AccessKind AK,
   if (!D || !D->hasAttrs())
     return;
 
   if (D->hasAttr<GuardedVarAttr>() && FSet.isEmpty(Analyzer->FactMan)) {
     Analyzer->Handler.handleNoMutexHeld(D, POK, AK, Loc);
   }
 
   for (const auto *I : D->specific_attrs<GuardedByAttr>())
-    warnIfMutexNotHeld(D, Exp, AK, I->getArg(), POK, Loc);
+    warnIfMutexNotHeld(D, Exp, AK, I->getArg(), POK, nullptr, Loc);
 }
 
 /// Checks pt_guarded_by and pt_guarded_var attributes.
 /// POK is the same  operationKind that was passed to checkAccess.
 void BuildLockset::checkPtAccess(const Expr *Exp, AccessKind AK,
                                  ProtectedOperationKind POK) {
   while (true) {
     if (const auto *PE = dyn_cast<ParenExpr>(Exp)) {
void BuildLockset::checkPtAccess(const Expr *Exp, AccessKind AK,
   const ValueDecl *D = getValueDecl(Exp);
   if (!D || !D->hasAttrs())
     return;
 
   if (D->hasAttr<PtGuardedVarAttr>() && FSet.isEmpty(Analyzer->FactMan))
     Analyzer->Handler.handleNoMutexHeld(D, PtPOK, AK, Exp->getExprLoc());
 
   for (auto const *I : D->specific_attrs<PtGuardedByAttr>())
-    warnIfMutexNotHeld(D, Exp, AK, I->getArg(), PtPOK, Exp->getExprLoc());
+    warnIfMutexNotHeld(D, Exp, AK, I->getArg(), PtPOK, nullptr,
+                       Exp->getExprLoc());
 }
 
 /// Process a function call, method call, constructor call,
 /// or destructor call.  This involves looking at the attributes on the
 /// corresponding function/method/constructor/destructor, issuing warnings,
 /// and updating the locksets accordingly.
 ///
 /// FIXME: For classes annotated with one of the guarded annotations, we need
 /// to treat const method calls as reads and non-const method calls as writes,
 /// and check that the appropriate locks are held. Non-const method calls with
 /// the same signature as const method calls can be also treated as reads.
 ///
+/// \param Exp   The call expression.
+/// \param D     The callee declaration.
+/// \param Self  If \p Exp = nullptr, the implicit this argument.
+/// \param Loc   If \p Exp = nullptr, the location.
 void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
-                              VarDecl *VD) {
-  SourceLocation Loc = Exp->getExprLoc();
+                              til::LiteralPtr *Self, SourceLocation Loc) {
   CapExprSet ExclusiveLocksToAdd, SharedLocksToAdd;
   CapExprSet ExclusiveLocksToRemove, SharedLocksToRemove, GenericLocksToRemove;
   CapExprSet ScopedReqsAndExcludes;
 
   // Figure out if we're constructing an object of scoped lockable class
-  bool isScopedVar = false;
-  if (VD) {
-    if (const auto *CD = dyn_cast<const CXXConstructorDecl>(D)) {
-      const CXXRecordDecl* PD = CD->getParent();
-      if (PD && PD->hasAttr<ScopedLockableAttr>())
-        isScopedVar = true;
+  CapabilityExpr Scp;
+  if (Exp) {
+    assert(!Self);
+    const auto *TagT = Exp->getType()->getAs<TagType>();
+    if (TagT && Exp->isPRValue()) {
+      std::pair<til::LiteralPtr *, StringRef> Placeholder =
+          Analyzer->SxBuilder.createThisPlaceholder(Exp);
+      [[maybe_unused]] auto inserted =
+          ConstructedObjects.insert({Exp, Placeholder.first});
+      assert(inserted.second && "Are we visiting the same expression again?");
+      if (isa<CXXConstructExpr>(Exp))
+        Self = Placeholder.first;
+      if (TagT->getDecl()->hasAttr<ScopedLockableAttr>())
+        Scp = CapabilityExpr(Placeholder.first, Placeholder.second, false);
     }
+
+    assert(Loc.isInvalid());
+    Loc = Exp->getExprLoc();
   }
 
   for(const Attr *At : D->attrs()) {
     switch (At->getKind()) {
       // When we encounter a lock function, we need to add the lock to our
       // lockset.
       case attr::AcquireCapability: {
         const auto *A = cast<AcquireCapabilityAttr>(At);
         Analyzer->getMutexIDs(A->isShared() ? SharedLocksToAdd
                                             : ExclusiveLocksToAdd,
-                              A, Exp, D, VD);
+                              A, Exp, D, Self);
         break;
       }
 
       // An assert will add a lock to the lockset, but will not generate
       // a warning if it is already there, and will not generate a warning
       // if it is not removed.
       case attr::AssertExclusiveLock: {
         const auto *A = cast<AssertExclusiveLockAttr>(At);
 
         CapExprSet AssertLocks;
-        Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
+        Analyzer->getMutexIDs(AssertLocks, A, Exp, D, Self);
         for (const auto &AssertLock : AssertLocks)
           Analyzer->addLock(
               FSet, std::make_unique<LockableFactEntry>(
                         AssertLock, LK_Exclusive, Loc, FactEntry::Asserted));
         break;
       }
       case attr::AssertSharedLock: {
         const auto *A = cast<AssertSharedLockAttr>(At);
 
         CapExprSet AssertLocks;
-        Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
+        Analyzer->getMutexIDs(AssertLocks, A, Exp, D, Self);
         for (const auto &AssertLock : AssertLocks)
           Analyzer->addLock(
               FSet, std::make_unique<LockableFactEntry>(
                         AssertLock, LK_Shared, Loc, FactEntry::Asserted));
         break;
       }
 
       case attr::AssertCapability: {
         const auto *A = cast<AssertCapabilityAttr>(At);
         CapExprSet AssertLocks;
-        Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
+        Analyzer->getMutexIDs(AssertLocks, A, Exp, D, Self);
         for (const auto &AssertLock : AssertLocks)
           Analyzer->addLock(FSet, std::make_unique<LockableFactEntry>(
                                       AssertLock,
                                       A->isShared() ? LK_Shared : LK_Exclusive,
                                       Loc, FactEntry::Asserted));
         break;
       }
 
       // When we encounter an unlock function, we need to remove unlocked
       // mutexes from the lockset, and flag a warning if they are not there.
       case attr::ReleaseCapability: {
         const auto *A = cast<ReleaseCapabilityAttr>(At);
         if (A->isGeneric())
-          Analyzer->getMutexIDs(GenericLocksToRemove, A, Exp, D, VD);
+          Analyzer->getMutexIDs(GenericLocksToRemove, A, Exp, D, Self);
         else if (A->isShared())
-          Analyzer->getMutexIDs(SharedLocksToRemove, A, Exp, D, VD);
+          Analyzer->getMutexIDs(SharedLocksToRemove, A, Exp, D, Self);
         else
-          Analyzer->getMutexIDs(ExclusiveLocksToRemove, A, Exp, D, VD);
+          Analyzer->getMutexIDs(ExclusiveLocksToRemove, A, Exp, D, Self);
         break;
       }
 
       case attr::RequiresCapability: {
         const auto *A = cast<RequiresCapabilityAttr>(At);
         for (auto *Arg : A->args()) {
           warnIfMutexNotHeld(D, Exp, A->isShared() ? AK_Read : AK_Written, Arg,
-                             POK_FunctionCall, Exp->getExprLoc());
+                             POK_FunctionCall, Self, Loc);
           // use for adopting a lock
-          if (isScopedVar)
-            Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, VD);
+          if (!Scp.shouldIgnore())
+            Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, Self);
         }
         break;
       }
 
       case attr::LocksExcluded: {
         const auto *A = cast<LocksExcludedAttr>(At);
         for (auto *Arg : A->args()) {
-          warnIfMutexHeld(D, Exp, Arg);
+          warnIfMutexHeld(D, Exp, Arg, Self, Loc);
           // use for deferring a lock
-          if (isScopedVar)
-            Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, VD);
+          if (!Scp.shouldIgnore())
+            Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, Self);
         }
         break;
       }
 
       // Ignore attributes unrelated to thread-safety
       default:
         break;
     }
   }
 
   // Remove locks first to allow lock upgrading/downgrading.
   // FIXME -- should only fully remove if the attribute refers to 'this'.
   bool Dtor = isa<CXXDestructorDecl>(D);
   for (const auto &M : ExclusiveLocksToRemove)
     Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Exclusive);
   for (const auto &M : SharedLocksToRemove)
     Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Shared);
   for (const auto &M : GenericLocksToRemove)
     Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Generic);
 
   // Add locks.
   FactEntry::SourceKind Source =
-      isScopedVar ? FactEntry::Managed : FactEntry::Acquired;
+      !Scp.shouldIgnore() ? FactEntry::Managed : FactEntry::Acquired;
   for (const auto &M : ExclusiveLocksToAdd)
     Analyzer->addLock(FSet, std::make_unique<LockableFactEntry>(M, LK_Exclusive,
                                                                 Loc, Source));
   for (const auto &M : SharedLocksToAdd)
     Analyzer->addLock(
         FSet, std::make_unique<LockableFactEntry>(M, LK_Shared, Loc, Source));
 
-  if (isScopedVar) {
+  if (!Scp.shouldIgnore()) {
     // Add the managing object as a dummy mutex, mapped to the underlying mutex.
-    SourceLocation MLoc = VD->getLocation();
-    DeclRefExpr DRE(VD->getASTContext(), VD, false, VD->getType(), VK_LValue,
-                    VD->getLocation());
-    // FIXME: does this store a pointer to DRE?
-    CapabilityExpr Scp = Analyzer->SxBuilder.translateAttrExpr(&DRE, nullptr);
-
-    auto ScopedEntry = std::make_unique<ScopedLockableFactEntry>(Scp, MLoc);
+    auto ScopedEntry = std::make_unique<ScopedLockableFactEntry>(Scp, Loc);
     for (const auto &M : ExclusiveLocksToAdd)
       ScopedEntry->addLock(M);
     for (const auto &M : SharedLocksToAdd)
       ScopedEntry->addLock(M);
     for (const auto &M : ScopedReqsAndExcludes)
       ScopedEntry->addLock(M);
     for (const auto &M : ExclusiveLocksToRemove)
       ScopedEntry->addExclusiveUnlock(M);

 void BuildLockset::VisitCXXConstructExpr(const CXXConstructExpr *Exp) {
   const CXXConstructorDecl *D = Exp->getConstructor();
   if (D && D->isCopyConstructor()) {
     const Expr* Source = Exp->getArg(0);
     checkAccess(Source, AK_Read);
   } else {
     examineArguments(D, Exp->arg_begin(), Exp->arg_end());
   }
+  if (D && D->hasAttrs())
+    handleCall(Exp, D);
 }
 
-static CXXConstructorDecl *
-findConstructorForByValueReturn(const CXXRecordDecl *RD) {
-  // Prefer a move constructor over a copy constructor. If there's more than
-  // one copy constructor or more than one move constructor, we arbitrarily
-  // pick the first declared such constructor rather than trying to guess which
-  // one is more appropriate.
-  CXXConstructorDecl *CopyCtor = nullptr;
-  for (auto *Ctor : RD->ctors()) {
-    if (Ctor->isDeleted())
-      continue;
-    if (Ctor->isMoveConstructor())
-      return Ctor;
-    if (!CopyCtor && Ctor->isCopyConstructor())
-      CopyCtor = Ctor;
-  }
-  return CopyCtor;
-}
-
-static Expr *buildFakeCtorCall(CXXConstructorDecl *CD, ArrayRef<Expr *> Args,
-                               SourceLocation Loc) {
-  ASTContext &Ctx = CD->getASTContext();
-  return CXXConstructExpr::Create(Ctx, Ctx.getRecordType(CD->getParent()), Loc,
-                                  CD, true, Args, false, false, false, false,
-                                  CXXConstructExpr::CK_Complete,
-                                  SourceRange(Loc, Loc));
-}
-
-static Expr *UnpackConstruction(Expr *E) {
+static const Expr *UnpackConstruction(const Expr *E) {
   if (auto *CE = dyn_cast<CastExpr>(E))
     if (CE->getCastKind() == CK_NoOp)
       E = CE->getSubExpr()->IgnoreParens();
   if (auto *CE = dyn_cast<CastExpr>(E))
     if (CE->getCastKind() == CK_ConstructorConversion ||
         CE->getCastKind() == CK_UserDefinedConversion)
       E = CE->getSubExpr();
   if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(E))
     E = BTE->getSubExpr();
   return E;
 }
 
 void BuildLockset::VisitDeclStmt(const DeclStmt *S) {
   // adjust the context
   LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, S, LVarCtx);
 
   for (auto *D : S->getDeclGroup()) {
     if (auto *VD = dyn_cast_or_null<VarDecl>(D)) {
-      Expr *E = VD->getInit();
+      const Expr *E = VD->getInit();
       if (!E)
         continue;
       E = E->IgnoreParens();
 
       // handle constructors that involve temporaries
       if (auto *EWC = dyn_cast<ExprWithCleanups>(E))
         E = EWC->getSubExpr()->IgnoreParens();
       E = UnpackConstruction(E);
 
-      if (const auto *CE = dyn_cast<CXXConstructExpr>(E)) {
-        const auto *CtorD = dyn_cast_or_null<NamedDecl>(CE->getConstructor());
-        if (!CtorD || !CtorD->hasAttrs())
-          continue;
-        handleCall(E, CtorD, VD);
-      } else if (isa<CallExpr>(E) && E->isPRValue()) {
-        // If the object is initialized by a function call that returns a
-        // scoped lockable by value, use the attributes on the copy or move
-        // constructor to figure out what effect that should have on the
-        // lockset.
-        // FIXME: Is this really the best way to handle this situation?
-        auto *RD = E->getType()->getAsCXXRecordDecl();
-        if (!RD || !RD->hasAttr<ScopedLockableAttr>())
-          continue;
-        CXXConstructorDecl *CtorD = findConstructorForByValueReturn(RD);
-        if (!CtorD || !CtorD->hasAttrs())
-          continue;
-        handleCall(buildFakeCtorCall(CtorD, {E}, E->getBeginLoc()), CtorD, VD);
+      if (auto Object = ConstructedObjects.find(E);
+          Object != ConstructedObjects.end()) {
+        Object->second->setClangDecl(VD);
+        ConstructedObjects.erase(Object);
       }
     }
   }
 }
 
+void BuildLockset::VisitMaterializeTemporaryExpr(
+    const MaterializeTemporaryExpr *Exp) {
+  if (const ValueDecl *ExtD = Exp->getExtendingDecl()) {
+    if (auto Object =
+            ConstructedObjects.find(UnpackConstruction(Exp->getSubExpr()));
+        Object != ConstructedObjects.end()) {
+      Object->second->setClangDecl(ExtD);
+      ConstructedObjects.erase(Object);
+    }
+  }
+}
+
 /// 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()) {
     // For managed capabilities, the destructor should unlock in the right mode
for (const auto *CurrBlock : *SortedGraph) {
     // 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;
         }
-        // Ignore BaseDtor, MemberDtor, and TemporaryDtor for now.
+        // Ignore BaseDtor and MemberDtor for now.
         case CFGElement::AutomaticObjectDtor: {
           CFGAutomaticObjDtor AD = BI.castAs<CFGAutomaticObjDtor>();
           const auto *DD = AD.getDestructorDecl(AC.getASTContext());
           if (!DD->hasAttrs())
             break;
 
-          // Create a dummy expression,
-          auto *VD = const_cast<VarDecl *>(AD.getVarDecl());
-          DeclRefExpr DRE(VD->getASTContext(), VD, false,
-                          VD->getType().getNonReferenceType(), VK_LValue,
+          LocksetBuilder.handleCall(nullptr, DD,
+                                    SxBuilder.createVariable(AD.getVarDecl()),
                                     AD.getTriggerStmt()->getEndLoc());
-          LocksetBuilder.handleCall(&DRE, DD);
+          break;
+        }
+        case CFGElement::TemporaryDtor: {
+          auto TD = BI.castAs<CFGTemporaryDtor>();
+
+          // Clean up constructed object even if there are no attributes to
+          // keep the number of objects in limbo as small as possible.
+          if (auto Object = LocksetBuilder.ConstructedObjects.find(
+                  TD.getBindTemporaryExpr()->getSubExpr());
+              Object != LocksetBuilder.ConstructedObjects.end()) {
+            const auto *DD = TD.getDestructorDecl(AC.getASTContext());
+            if (DD->hasAttrs())
+              // TODO: the location here isn't quite correct.
+              LocksetBuilder.handleCall(nullptr, DD, Object->second,
+                                        TD.getBindTemporaryExpr()->getEndLoc());
+            LocksetBuilder.ConstructedObjects.erase(Object);
+          }
           break;
         }
         default:
           break;
       }
     }
     CurrBlockInfo->ExitSet = LocksetBuilder.FSet;
 
     // For every back edge from CurrBlock (the end of the loop) to another block
     // (FirstLoopBlock) we need to check that the Lockset of Block is equal to
     // the one held at the beginning of FirstLoopBlock. We can look up the
     // Lockset held at the beginning of FirstLoopBlock in the EntryLockSets map.

clang/lib/Analysis/ThreadSafetyCommon.cpp

 
 /// Translate a clang expression in an attribute to a til::SExpr.
 /// Constructs the context from D, DeclExp, and SelfDecl.
 ///
 /// \param AttrExp The expression to translate.
 /// \param D       The declaration to which the attribute is attached.
 /// \param DeclExp An expression involving the Decl to which the attribute
 ///                is attached.  E.g. the call to a function.
+/// \param Self    S-expression to substitute for a \ref CXXThisExpr.
 CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
                                                const NamedDecl *D,
                                                const Expr *DeclExp,
-                                               VarDecl *SelfDecl) {
+                                               til::SExpr *Self) {
   // If we are processing a raw attribute expression, with no substitutions.
-  if (!DeclExp)
+  if (!DeclExp && !Self)
     return translateAttrExpr(AttrExp, nullptr);
 
   CallingContext Ctx(nullptr, D);
 
   // Examine DeclExp to find SelfArg and FunArgs, which are used to substitute
   // for formal parameters when we call buildMutexID later.
-  if (const auto *ME = dyn_cast<MemberExpr>(DeclExp)) {
+  if (!DeclExp)
+    /* We'll use Self. */;
+  else if (const auto *ME = dyn_cast<MemberExpr>(DeclExp)) {
     Ctx.SelfArg   = ME->getBase();
     Ctx.SelfArrow = ME->isArrow();
   } else if (const auto *CE = dyn_cast<CXXMemberCallExpr>(DeclExp)) {
     Ctx.SelfArg   = CE->getImplicitObjectArgument();
     Ctx.SelfArrow = isCalleeArrow(CE->getCallee());
     Ctx.NumArgs   = CE->getNumArgs();
     Ctx.FunArgs   = CE->getArgs();
   } else if (const auto *CE = dyn_cast<CallExpr>(DeclExp)) {
     Ctx.NumArgs = CE->getNumArgs();
     Ctx.FunArgs = CE->getArgs();
   } else if (const auto *CE = dyn_cast<CXXConstructExpr>(DeclExp)) {
     Ctx.SelfArg = nullptr;  // Will be set below
     Ctx.NumArgs = CE->getNumArgs();
     Ctx.FunArgs = CE->getArgs();
-  } else if (D && isa<CXXDestructorDecl>(D)) {
-    // There's no such thing as a "destructor call" in the AST.
-    Ctx.SelfArg = DeclExp;
   }
 
-  // Hack to handle constructors, where self cannot be recovered from
-  // the expression.
-  if (SelfDecl && !Ctx.SelfArg) {
-    DeclRefExpr SelfDRE(SelfDecl->getASTContext(), SelfDecl, false,
-                        SelfDecl->getType(), VK_LValue,
-                        SelfDecl->getLocation());
-    Ctx.SelfArg = &SelfDRE;
+  if (Self) {
+    assert(!Ctx.SelfArg && "Ambiguous self argument");
+    Ctx.SelfArg = Self;
 
     // If the attribute has no arguments, then assume the argument is "this".
     if (!AttrExp)
-      return translateAttrExpr(Ctx.SelfArg, nullptr);
+      return CapabilityExpr(
+          Self, ClassifyDiagnostic(cast<CXXMethodDecl>(D)->getThisObjectType()),
+          false);
     else  // For most attributes.
       return translateAttrExpr(AttrExp, &Ctx);
   }
 
   // If the attribute has no arguments, then assume the argument is "this".
   if (!AttrExp)
-    return translateAttrExpr(Ctx.SelfArg, nullptr);
+    return translateAttrExpr(cast<const Expr *>(Ctx.SelfArg), nullptr);
   else  // For most attributes.
     return translateAttrExpr(AttrExp, &Ctx);
 }
 
 /// Translate a clang expression in an attribute to a til::SExpr.
 // This assumes a CallingContext has already been created.
 CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
                                                CallingContext *Ctx) {
CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
   // Hack to deal with smart pointers -- strip off top-level pointer casts.
   if (const auto *CE = dyn_cast<til::Cast>(E)) {
     if (CE->castOpcode() == til::CAST_objToPtr)
       return CapabilityExpr(CE->expr(), Kind, Neg);
   }
   return CapabilityExpr(E, Kind, Neg);
 }
 
+til::LiteralPtr *SExprBuilder::createVariable(const VarDecl *VD) {
+  return new (Arena) til::LiteralPtr(VD);
+}
+
+std::pair<til::LiteralPtr *, StringRef>
+SExprBuilder::createThisPlaceholder(const Expr *Exp) {
+  return {new (Arena) til::LiteralPtr(nullptr),
+          ClassifyDiagnostic(Exp->getType())};
+}
+
 // Translate a clang statement or expression to a TIL expression.
 // Also performs substitution of variables; Ctx provides the context.
 // Dispatches on the type of S.
 til::SExpr *SExprBuilder::translate(const Stmt *S, CallingContext *Ctx) {
   if (!S)
     return nullptr;
 
   // Check if S has already been translated and cached.
til::SExpr *SExprBuilder::translateDeclRefExpr(const DeclRefExpr *DRE,
 
   // For non-local variables, treat it as a reference to a named object.
   return new (Arena) til::LiteralPtr(VD);
 }
 
 til::SExpr *SExprBuilder::translateCXXThisExpr(const CXXThisExpr *TE,
                                                CallingContext *Ctx) {
   // Substitute for 'this'
-  if (Ctx && Ctx->SelfArg)
-    return translate(Ctx->SelfArg, Ctx->Prev);
+  if (Ctx && Ctx->SelfArg) {
+    if (const auto *SelfArg = dyn_cast<const Expr *>(Ctx->SelfArg))
+      return translate(SelfArg, Ctx->Prev);
+    else
+      return cast<til::SExpr *>(Ctx->SelfArg);
+  }
   assert(SelfVar && "We have no variable for 'this'!");
   return SelfVar;
 }
 
 static const ValueDecl *getValueDeclFromSExpr(const til::SExpr *E) {
   if (const auto *V = dyn_cast<til::Variable>(E))
     return V->clangDecl();
   if (const auto *Ph = dyn_cast<til::Phi>(E))

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

void bar4(MyData* d1, MyData* d2) {
       DataLocker dlr;
       dlr.lockData(d1);
       foo(d2); // \
         // expected-warning {{calling function 'foo' requires holding mutex 'd2->mu' exclusively}} \
         // expected-note {{found near match 'd1->mu'}}
       dlr.unlockData(d1);
     }
   };
+
+  // Automatic object destructor calls don't appear as expressions in the CFG,
+  // so we have to handle them separately whenever substitutions are required.
+  struct DestructorRequires {
+    Mutex mu;
+    ~DestructorRequires() EXCLUSIVE_LOCKS_REQUIRED(mu);
+  };
+
+  void destructorRequires() {
+    DestructorRequires rd;
+    rd.mu.AssertHeld();
+  }
+
+  struct DestructorExcludes {
+    Mutex mu;
+    ~DestructorExcludes() LOCKS_EXCLUDED(mu);
+  };
+
+  void destructorExcludes() {
+    DestructorExcludes ed;
+    ed.mu.Lock(); // expected-note {{mutex acquired here}}
+  } // expected-warning {{cannot call function '~DestructorExcludes' while mutex 'ed.mu' is held}}
+    // expected-warning@-1 {{mutex 'ed.mu' is still held at the end of function}}
+
 } // end namespace substituation_test
 
 
 
 namespace constructor_destructor_tests {
   Mutex fooMu;
   int myVar GUARDED_BY(fooMu);
 

 
 #ifdef __cpp_guaranteed_copy_elision
   void const_lock() {
     const MutexLock mulock = MutexLock(&mu1);
     a = 5;
   }
 #endif
 
+  void temporary() {
+    MutexLock{&mu1}, a = 5;
+  }
+
+  void lifetime_extension() {
+    const MutexLock &mulock = MutexLock(&mu1);
+    a = 5;
+  }
+
   void foo2() {
     ReaderMutexLock mulock1(&mu1);
     if (getBool()) {
       MutexLock mulock2a(&mu2);
       b = a + 1;
     }
     else {
       MutexLock mulock2b(&mu2);
       b = a + 2;
     }
   }
 
   void foo3() {
     MutexLock mulock_a(&mu1); // expected-note{{mutex acquired here}}
     MutexLock mulock_b(&mu1); // \
       // expected-warning {{acquiring mutex 'mu1' that is already held}}
   }
 
+  void temporary_double_lock() {
+    MutexLock mulock_a(&mu1); // expected-note{{mutex acquired here}}
+    MutexLock{&mu1};          // \
+      // expected-warning {{acquiring mutex 'mu1' that is already held}}
+  }
+
   void foo4() {
     MutexLock mulock1(&mu1), mulock2(&mu2);
     a = b+1;
     b = a+1;
   }
 
   void foo5() {
     DoubleMutexLock mulock(&mu1, &mu2);

 class LOCKABLE SelfLock2 {
 public:
   SelfLock2()  EXCLUSIVE_LOCK_FUNCTION();
   ~SelfLock2() UNLOCK_FUNCTION();
 
   void foo() EXCLUSIVE_LOCKS_REQUIRED(this);
 };
 
+class SelfLockDeferred {
+public:
+  SelfLockDeferred() LOCKS_EXCLUDED(mu_);
+  ~SelfLockDeferred() UNLOCK_FUNCTION(mu_);
+
+  Mutex mu_;
+};
+
+class LOCKABLE SelfLockDeferred2 {
+public:
+  SelfLockDeferred2() LOCKS_EXCLUDED(this);
+  ~SelfLockDeferred2() UNLOCK_FUNCTION();
+};
+
 
 void test() {
   SelfLock s;
   s.foo();
 }
 
 void test2() {
   SelfLock2 s2;
   s2.foo();
 }
 
+void testDeferredTemporary() {
+  SelfLockDeferred(); // expected-warning {{releasing mutex '<temporary>.mu_' that was not held}}
+}
+
+void testDeferredTemporary2() {
+  SelfLockDeferred2(); // expected-warning {{releasing mutex '<temporary>' that was not held}}
+}
+
 }  // end namespace SelfConstructorTest
 
 
 namespace MultipleAttributeTest {
 
 class Foo {
   Mutex mu1_;
   Mutex mu2_;

 };
 struct C {
   B *operator[](int);
 };
 C c;
 void f() { c[A()]->g(); }
 } // namespace PR34800
 
+#ifdef __cpp_guaranteed_copy_elision
+
 namespace ReturnScopedLockable {
-  template<typename Object> class SCOPED_LOCKABLE ReadLockedPtr {
-  public:
-    ReadLockedPtr(Object *ptr) SHARED_LOCK_FUNCTION((*this)->mutex);
-    ReadLockedPtr(ReadLockedPtr &&) SHARED_LOCK_FUNCTION((*this)->mutex);
-    ~ReadLockedPtr() UNLOCK_FUNCTION();
+
+class Object {
+  MutexLock lock() EXCLUSIVE_LOCK_FUNCTION(mutex) {
+    // TODO: False positive because scoped lock isn't destructed.
+    return MutexLock(&mutex); // expected-note {{mutex acquired here}}
+  }                           // expected-warning {{mutex 'mutex' is still held at the end of function}}
+
+  ReaderMutexLock lockShared() SHARED_LOCK_FUNCTION(mutex) {
+    // TODO: False positive because scoped lock isn't destructed.
+    return ReaderMutexLock(&mutex); // expected-note {{mutex acquired here}}
+  }                                 // expected-warning {{mutex 'mutex' is still held at the end of function}}
+
+  MutexLock adopt() EXCLUSIVE_LOCKS_REQUIRED(mutex) {
+    // TODO: False positive because scoped lock isn't destructed.
+    return MutexLock(&mutex, true); // expected-note {{mutex acquired here}}
+  }                                 // expected-warning {{mutex 'mutex' is still held at the end of function}}
+
+  ReaderMutexLock adoptShared() SHARED_LOCKS_REQUIRED(mutex) {
+    // TODO: False positive because scoped lock isn't destructed.
+    return ReaderMutexLock(&mutex, true); // expected-note {{mutex acquired here}}
+  }                                       // expected-warning {{mutex 'mutex' is still held at the end of function}}
 
-    Object *operator->() const { return object; }
+  int x GUARDED_BY(mutex);
+  void needsLock() EXCLUSIVE_LOCKS_REQUIRED(mutex);
 
-  private:
-    Object *object;
-  };
+  void testInside() {
+    MutexLock scope = lock();
+    x = 1;
+    needsLock();
+  }
 
-  struct Object {
-    int f() SHARED_LOCKS_REQUIRED(mutex);
-    Mutex mutex;
-  };
 
-  ReadLockedPtr<Object> get();
-  int use() {
-    auto ptr = get();
-    return ptr->f();
-  }
-  void use_constructor() {
-    auto ptr = ReadLockedPtr<Object>(nullptr);
-    ptr->f();
-    auto ptr2 = ReadLockedPtr<Object>{nullptr};
-    ptr2->f();
-    auto ptr3 = (ReadLockedPtr<Object>{nullptr});
-    ptr3->f();
-  }
-  struct Convertible {
-    Convertible();
-    operator ReadLockedPtr<Object>();
-  };
-  void use_conversion() {
-    ReadLockedPtr<Object> ptr = Convertible();
-    ptr->f();
-  }
+Object obj;
+
+void testLock() {
+  MutexLock scope = obj.lock();
+  obj.x = 1;
+  obj.needsLock();
+
+int testSharedLock() {
+  ReaderMutexLock scope = obj.lockShared();
+  obj.x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'obj.mutex' exclusively}}
+  return obj.x;
+}
+
+void testAdopt() {
+  obj.mutex.Lock();
+  MutexLock scope = obj.adopt();
+  obj.x = 1;
 }
 
+int testAdoptShared() {
+  obj.mutex.Lock();
+  ReaderMutexLock scope = obj.adoptShared();
+  obj.x = 1;
+  return obj.x;
+}
+
+} // namespace ReturnScopedLockable
+
+#endif
+
 namespace PR38640 {
 void f() {
   // Self-referencing assignment previously caused an infinite loop when thread
   // safety analysis was enabled.
   int &i = i; // expected-warning {{reference 'i' is not yet bound to a value when used within its own initialization}}
 }
 }