[PATCH] D124131: Thread safety analysis: Store capability kind in CapabilityExpr

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM with the extra safety measure added. I'm happy to review again if you'd like, but don't require it.



================
Comment at: clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h:277
+  /// The kind of capability as specified by @ref CapabilityAttr::getName.
+  StringRef CapKind;
+
----------------
aaronpuchert wrote:
> aaron.ballman wrote:
> > Hrmmmm, I think this may actually be safe, but it does give me pause to store a `StringRef` as anything other than a local (or param/return type): https://llvm.org/docs/ProgrammersManual.html#the-stringref-class
> > 
> > Can you double-check my thinking? This is fine because it's pulling the `StringRef` from the `CapabilityAttr`, and that stores its name internally and isn't released until the AST is destroyed.
> Exactly, aside from two places where I'm storing a string literal ("mutex" and "wildcard"), the common case is taking the `StringRef` (as returned by `CapabilityAttr::getName`) from the attribute. So that would be lifetime-coupled to the AST and should be safe for our analysis.
> 
> Of course `StringRef` is implicitly constructible from other types, and we wouldn't want that. Especially `std::string` would be an issue. Perhaps we should prevent implicit conversions, and thus force the caller to pass a `StringRef` glvalue, by overloading with a deleted template?
> ```
> template<typename T>
> CapabilityExpr(const til::SExpr *, T, bool) = delete;
> ```
> Or maybe you've got a better idea.
> 
> As for a justification: we're building lots of `CapabilityExpr`s, essentially every time we see a capability expression in the code (in attributes or as capability type method call arguments). Given that the kind is only used for actual warnings I wouldn't want us to spend of lot of time or memory on storing it.
> 
> We could also store the original `Expr*` and extract on demand, but that seemed to me antithetical to translating into the TIL. Of course it would be slightly faster, but the current code (before this change) also extracts capability names eagerly without waiting for the need to arise.
> Of course StringRef is implicitly constructible from other types, and we wouldn't want that. Especially std::string would be an issue. Perhaps we should prevent implicit conversions, and thus force the caller to pass a StringRef glvalue, by overloading with a deleted template?

I think that's likely a good safety measure, good suggestion!

> As for a justification: 

Oh, I already thought it was well-motivated, I just wanted to make sure I wasn't wrong about the lack of lifetime issues. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124131