[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D87629#2275639 <https://reviews.llvm.org/D87629#2275639>, @ryanofsky wrote:

> The mistakes about exceptions came from me taking "(no return)" in the previous documentation too literally thinking it was referring to https://en.cppreference.com/w/cpp/language/attributes/noreturn.

The key here is the word "assumed". We treat the function as if it looks like this:

  [[noreturn]] void error();
  
  void assertHeld(Mutex &mu) ASSERT_CAPABILITY(mu) {
    if (!mu.isLocked())
      error();
  }

So we assume that the function doesn't return if the mutex isn't held. (For the analysis, not for generating code.)

But the idea—as usual with assertions—is that they just verify assumptions that should hold anyway, so they are not integral part of the code and can be dropped <https://www.youtube.com/watch?v=1QhtXRMp3Hg>, like the standard assert <https://en.cppreference.com/w/cpp/error/assert> when `NDEBUG` is defined. And that's what we might clarify here. The right model is that this function converts dynamically held capabilities into statically held capabilities. It doesn't have to actually do anything, instead it's a marker for a promise: the programmer can't prove that the capability is held but promises it. And in some build profiles we might check that the promise is satisfied, but we don't guarantee that we do.

Which obviously means that an `ASSERT_CAPABILITY` function should only be used as a last resort, and it makes some sense to me that some bitcoin contributors want to have the standard assertion annotated with `EXCLUSIVE_LOCKS_REQUIRED` so that people don't accidentally defeat the Analysis by adding assertions. I can't really speak as to whether that's a valid concern, but there is certainly no misunderstanding.

> Re: "I got the impression that you want both runtime checks and compile-time checks in parallel". Personally I only want the compile checks, and would like to drop all the runtime checks except in handful of places where compile time checks don't work.

Indeed, now that I've re-read the discussion I see that the argument about guarding against compiler bugs <https://github.com/bitcoin/bitcoin/pull/19865#issuecomment-687604066> wasn't yours. I guess you have to discuss in the bitcoin community how much to trust the compiler, although you might just end up with Ken Thompson's “Reflections on Trusting Trust” <http://users.ece.cmu.edu/~ganger/712.fall02/papers/p761-thompson.pdf>. In my view you're probably better off testing with ThreadSanitizer <https://clang.llvm.org/docs/ThreadSanitizer.html> than adding runtime checks where the Analysis says the lock is held, but I do see the point <https://github.com/bitcoin/bitcoin/pull/19865#issuecomment-686986462> of nudging people to add annotations when they have an assertion. No misunderstanding there as well.

> ASSERT_CAPABILITY does a great job in these places so I don't want people to be scared off from using it unduly because of documentation!

If you're referring to @vasild's comment <https://github.com/bitcoin/bitcoin/pull/19865#issuecomment-688469149> I hope that we cleared this up. The attribute is meant to mirror the behavior of the standard `assert` macro.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87629