[PATCH] D124131: Thread safety analysis: Store capability kind in CapabilityExpr
This revision was automatically updated to reflect the committed changes.
aaronpuchert marked 2 inline comments as done.
Closed by commit rGf8afb8fdedae: Thread safety analysis: Store capability kind
in CapabilityExpr (authored by aaronpuchert).
Changed prior to commit:
https://reviews.llvm.org/D124131?vs=424038=426155#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124131/new/
https://reviews.llvm.org/D124131
Files:
clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
clang/lib/Analysis/ThreadSafety.cpp
clang/lib/Analysis/ThreadSafetyCommon.cpp
clang/test/SemaCXX/warn-thread-safety-analysis.cpp
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -3862,8 +3862,8 @@
}
a = 0; // \
// expected-warning {{writing variable 'a' requires holding mutex 'mu_' exclusively}}
-endNoWarnOnWrites(); // \
- // expected-warning {{releasing mutex '*' that was not held}}
+endNoWarnOnWrites(); // \
+ // expected-warning {{releasing wildcard '*' that was not held}}
}
Index: clang/lib/Analysis/ThreadSafetyCommon.cpp
===
--- clang/lib/Analysis/ThreadSafetyCommon.cpp
+++ clang/lib/Analysis/ThreadSafetyCommon.cpp
@@ -86,6 +86,28 @@
return ME ? ME->isArrow() : false;
}
+static StringRef ClassifyDiagnostic(const CapabilityAttr *A) {
+ return A->getName();
+}
+
+static StringRef ClassifyDiagnostic(QualType VDT) {
+ // We need to look at the declaration of the type of the value to determine
+ // which it is. The type should either be a record or a typedef, or a pointer
+ // or reference thereof.
+ if (const auto *RT = VDT->getAs()) {
+if (const auto *RD = RT->getDecl())
+ if (const auto *CA = RD->getAttr())
+return ClassifyDiagnostic(CA);
+ } else if (const auto *TT = VDT->getAs()) {
+if (const auto *TD = TT->getDecl())
+ if (const auto *CA = TD->getAttr())
+return ClassifyDiagnostic(CA);
+ } else if (VDT->isPointerType() || VDT->isReferenceType())
+return ClassifyDiagnostic(VDT->getPointeeType());
+
+ return "mutex";
+}
+
/// Translate a clang expression in an attribute to a til::SExpr.
/// Constructs the context from D, DeclExp, and SelfDecl.
///
@@ -152,16 +174,17 @@
CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
CallingContext *Ctx) {
if (!AttrExp)
-return CapabilityExpr(nullptr, false);
+return CapabilityExpr();
if (const auto* SLit = dyn_cast(AttrExp)) {
if (SLit->getString() == StringRef("*"))
// The "*" expr is a universal lock, which essentially turns off
// checks until it is removed from the lockset.
- return CapabilityExpr(new (Arena) til::Wildcard(), false);
+ return CapabilityExpr(new (Arena) til::Wildcard(), StringRef("wildcard"),
+false);
else
// Ignore other string literals for now.
- return CapabilityExpr(nullptr, false);
+ return CapabilityExpr();
}
bool Neg = false;
@@ -183,14 +206,16 @@
// Trap mutex expressions like nullptr, or 0.
// Any literal value is nonsense.
if (!E || isa(E))
-return CapabilityExpr(nullptr, false);
+return CapabilityExpr();
+
+ StringRef Kind = ClassifyDiagnostic(AttrExp->getType());
// Hack to deal with smart pointers -- strip off top-level pointer casts.
if (const auto *CE = dyn_cast(E)) {
if (CE->castOpcode() == til::CAST_objToPtr)
- return CapabilityExpr(CE->expr(), Neg);
+ return CapabilityExpr(CE->expr(), Kind, Neg);
}
- return CapabilityExpr(E, Neg);
+ return CapabilityExpr(E, Kind, Neg);
}
// Translate a clang statement or expression to a TIL expression.
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -139,12 +139,12 @@
SourceLocation JoinLoc, LockErrorKind LEK,
ThreadSafetyHandler ) const = 0;
virtual void handleLock(FactSet , FactManager ,
- const FactEntry , ThreadSafetyHandler ,
- StringRef DiagKind) const = 0;
+ const FactEntry ,
+ ThreadSafetyHandler ) const = 0;
virtual void handleUnlock(FactSet , FactManager ,
const CapabilityExpr , SourceLocation UnlockLoc,
-bool FullyRemove, ThreadSafetyHandler ,
-StringRef DiagKind) const = 0;
+bool FullyRemove,
+ThreadSafetyHandler ) const = 0;
//
[PATCH] D124131: Thread safety analysis: Store capability kind in CapabilityExpr
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:
> > Hr, 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
> 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D124131: Thread safety analysis: Store capability kind in CapabilityExpr
aaronpuchert added inline comments.
Comment at: clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h:277
+ /// The kind of capability as specified by @ref CapabilityAttr::getName.
+ StringRef CapKind;
+
aaron.ballman wrote:
> Hr, 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
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124131/new/
https://reviews.llvm.org/D124131
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D124131: Thread safety analysis: Store capability kind in CapabilityExpr
aaron.ballman added inline comments. Comment at: clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h:277 + /// The kind of capability as specified by @ref CapabilityAttr::getName. + StringRef CapKind; + Hr, 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124131/new/ https://reviews.llvm.org/D124131 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D124131: Thread safety analysis: Store capability kind in CapabilityExpr
aaronpuchert created this revision.
aaronpuchert added a reviewer: aaron.ballman.
Herald added a project: All.
aaronpuchert requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
This should make us print the right capability kind in many more cases,
especially when attributes name multiple capabilities of different kinds.
Previously we were trying to deduce the capability kind from the
original attribute, but most attributes can name multiple capabilities,
which could be of different kinds. So instead we derive the kind when
translating the attribute expression, and then store it in the returned
CapabilityExpr. Then we can extract the corresponding capability name
when we need it, which saves us lots of plumbing and almost guarantees
that the name is right.
I didn't bother adding any tests for this because it's just a usability
improvement and it's pretty much evident from the code that we don't
fall back to "mutex" anymore (save for a few cases that I'll address in
a separate change).
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D124131
Files:
clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
clang/lib/Analysis/ThreadSafety.cpp
clang/lib/Analysis/ThreadSafetyCommon.cpp
clang/test/SemaCXX/warn-thread-safety-analysis.cpp
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -3862,8 +3862,8 @@
}
a = 0; // \
// expected-warning {{writing variable 'a' requires holding mutex 'mu_' exclusively}}
-endNoWarnOnWrites(); // \
- // expected-warning {{releasing mutex '*' that was not held}}
+endNoWarnOnWrites(); // \
+ // expected-warning {{releasing wildcard '*' that was not held}}
}
Index: clang/lib/Analysis/ThreadSafetyCommon.cpp
===
--- clang/lib/Analysis/ThreadSafetyCommon.cpp
+++ clang/lib/Analysis/ThreadSafetyCommon.cpp
@@ -86,6 +86,28 @@
return ME ? ME->isArrow() : false;
}
+static StringRef ClassifyDiagnostic(const CapabilityAttr *A) {
+ return A->getName();
+}
+
+static StringRef ClassifyDiagnostic(QualType VDT) {
+ // We need to look at the declaration of the type of the value to determine
+ // which it is. The type should either be a record or a typedef, or a pointer
+ // or reference thereof.
+ if (const auto *RT = VDT->getAs()) {
+if (const auto *RD = RT->getDecl())
+ if (const auto *CA = RD->getAttr())
+return ClassifyDiagnostic(CA);
+ } else if (const auto *TT = VDT->getAs()) {
+if (const auto *TD = TT->getDecl())
+ if (const auto *CA = TD->getAttr())
+return ClassifyDiagnostic(CA);
+ } else if (VDT->isPointerType() || VDT->isReferenceType())
+return ClassifyDiagnostic(VDT->getPointeeType());
+
+ return "mutex";
+}
+
/// Translate a clang expression in an attribute to a til::SExpr.
/// Constructs the context from D, DeclExp, and SelfDecl.
///
@@ -152,16 +174,16 @@
CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
CallingContext *Ctx) {
if (!AttrExp)
-return CapabilityExpr(nullptr, false);
+return CapabilityExpr();
if (const auto* SLit = dyn_cast(AttrExp)) {
if (SLit->getString() == StringRef("*"))
// The "*" expr is a universal lock, which essentially turns off
// checks until it is removed from the lockset.
- return CapabilityExpr(new (Arena) til::Wildcard(), false);
+ return CapabilityExpr(new (Arena) til::Wildcard(), "wildcard", false);
else
// Ignore other string literals for now.
- return CapabilityExpr(nullptr, false);
+ return CapabilityExpr();
}
bool Neg = false;
@@ -183,14 +205,16 @@
// Trap mutex expressions like nullptr, or 0.
// Any literal value is nonsense.
if (!E || isa(E))
-return CapabilityExpr(nullptr, false);
+return CapabilityExpr();
+
+ StringRef Kind = ClassifyDiagnostic(AttrExp->getType());
// Hack to deal with smart pointers -- strip off top-level pointer casts.
if (const auto *CE = dyn_cast(E)) {
if (CE->castOpcode() == til::CAST_objToPtr)
- return CapabilityExpr(CE->expr(), Neg);
+ return CapabilityExpr(CE->expr(), Kind, Neg);
}
- return CapabilityExpr(E, Neg);
+ return CapabilityExpr(E, Kind, Neg);
}
// Translate a clang statement or expression to a TIL expression.
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -139,12 +139,12 @@
SourceLocation JoinLoc, LockErrorKind LEK,
ThreadSafetyHandler ) const = 0;
virtual void handleLock(FactSet ,
