[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D129755#3843144 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D49885>) and deferred locking (D81332 <https://reviews.llvm.org/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 <https://www.youtube.com/watch?v=5Xx0zktqSs4> 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 <https://discourse.llvm.org/t/rfc-add-new-discourse-channel-for-potentially-breaking-disruptive-changes-for-clang/65251>. 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.



================
Comment at: clang/test/SemaCXX/warn-thread-safety-analysis.cpp:1620
+    rd.mu.AssertHeld();
+  }
+
----------------
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.


================
Comment at: clang/test/SemaCXX/warn-thread-safety-analysis.cpp:1630-1631
+    ed.mu.Lock(); // expected-note {{mutex acquired here}}
+  } // expected-warning {{cannot call function '~DestructorExcludes' while mutex 'ed.mu' is held}}
+    // expected-warning at -1 {{mutex 'ed.mu' is still held at the end of function}}
+
----------------
Similar to the reported issue, here we were not reporting warnings. Underlying reason is the same missing substitution.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129755/new/

https://reviews.llvm.org/D129755