[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)

[Marco Elver via cfe-commits]

================
@@ -271,26 +271,32 @@ class CFGWalker {
 // translateAttrExpr needs it, but that should be moved too.
 class CapabilityExpr {
 private:
-  /// The capability expression and whether it's negated.
-  llvm::PointerIntPair<const til::SExpr *, 1, bool> CapExpr;
+  /// The capability expression and flags.
+  llvm::PointerIntPair<const til::SExpr *, 2, unsigned> CapExpr;
 
   /// The kind of capability as specified by @ref CapabilityAttr::getName.
   StringRef CapKind;
 
 public:
-  CapabilityExpr() : CapExpr(nullptr, false) {}
-  CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
-      : CapExpr(E, Neg), CapKind(Kind) {}
+  static constexpr unsigned FlagNegative = 1u << 0;
+  static constexpr unsigned FlagReentrant = 1u << 1;
----------------
melver wrote:

There might be valid uses for that, even if it's not something we'd expect to be frequent. Developer creativity how to use a feature can sometimes lead to new uses we never expected, and I wouldn't want to prohibit them.

If it's *not wrong semantically*, I tend to not want to impose a certain pattern and imply to the user "you're holding it wrong" even though it's not strictly wrong and they might actually know what they're doing. I've been on the receiving end of such forced restrictions, and it's rather annoying - though quite often it's because the restriction allowed for a simpler implementation, I'd argue in this case that's a moot point.

I think if we were talking about synchronization only, I'd be more inclined to say "this never makes sense", but the feature is now about arbitrary capabilities.

For example, we might want to express that some capability is never held when entering a function for performance reasons, yet that capability itself can be reentrantly acquired on slow paths. I don't see a reason to prohibit uses like that.

https://github.com/llvm/llvm-project/pull/137133