[PATCH] D104649: Thread safety analysis: Rename parameters of ThreadSafetyAnalyzer::intersectAndWarn (NFC)
delesley accepted this revision. delesley added a comment. Looks good. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104649/new/ https://reviews.llvm.org/D104649 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D104261: Thread safety analysis: Always warn when dropping locks on back edges
delesley accepted this revision. delesley added a comment. This revision is now accepted and ready to land. This looks good to me. Thanks for the patch! Since you're adding a new warning, this may break some code somewhere, but since it's restricted to relockable managed locks, I'm not too worried... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104261/new/ https://reviews.llvm.org/D104261 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities
delesley accepted this revision. delesley added a comment. This revision is now accepted and ready to land. Thanks for taking the time to discuss things with me. :-) Wrt. to the TEST_LOCKED_FUNCTION, I agree that you can simulate the behavior using Assert and Lock. But that pattern is too general/powerful, because it also allows you to write nonsensical and unsound code. Philosophically, static analysis is concerned with allowing things that are sound, but preventing things that are not, so I would prefer to allow the valid case, but warn on the nonsensical/unsound code. The goal is not to provide powerful back doors so that you can squeeze anything past the checker -- doing so kind of defeats the point. :-) That being said, I'm not certainly no asking you to implement TEST_LOCKED functionality in this particular patch, and I totally understand that it may simply not be worth the effort. Wrt. to unlocking an Assert, I see no problem. It makes perfect sense to dynamically assert that a mutex is held, then do something, and then unlock the mutex afterwards; after all, the caller asserted that it was held before unlocking. You shouldn't disallow that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102026/new/ https://reviews.llvm.org/D102026 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities
delesley added inline comments.
Comment at: clang/test/SemaCXX/warn-thread-safety-analysis.cpp:4570
mu_.AssertHeld();
-mu_.Unlock();
- } // should this be a warning?
+mu_.Unlock(); // should this be a warning?
+ }
This function should have a warning because is not marked with UNLOCK_FUNCTION.
The lock was held on entry, and not held on exit. So the original location of
the comment, on the closing curly brace, was correct.
Comment at: clang/test/SemaCXX/warn-thread-safety-analysis.cpp:4629
+ mu_.AssertHeld(); // expected-note {{the other acquisition of mutex
'mu_' is here}}
+// FIXME: should instead warn because it's unclear whether we need to
release or not.
+int b = a;
I'm not wild about having FIXMEs in test code. I would prefer to have the
patch actually issue a warning here (but see below).
HOWEVER, does the current code issue a warning in this situation? If not,
leave it as-is and keep the FIXME. You can't make the analysis more strict
without checking with the C++ compiler team at Google (which I am no longer
on), because it may break the build on our end.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102026/new/
https://reviews.llvm.org/D102026
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities
delesley added a comment. I have a few concerns. First, this patch introduces an inconsistency between managed and unmanaged locks. For unmanaged locks, we warn, and //then assume the lock is held exclusively//. For managed locks, we don't warn, but //assume it is held shared//. The warn/not-warn is consistent with more relaxed handling of managed locks, but exclusive/shared difference could lead to confusion. Both mechanisms are sound; they're just not consistent. Any thoughts? Second, the mixing of AssertHeld() and Lock() on different control flow branches looks very weird to me. I can see one obvious case where you might want to do that, e.g. if (mu_.is_locked()) mu_.AssertHeld(); else mu_.Lock(). However, if a function locks mu_ on one branch, and assert mu_ on the other, then that usually indicates a problem. It means that the lock-state going into that function was unknown; the programmer didn't know whether mu_ was locked or not, and that's never good. The //only// valid condition in that case is to check whether mu_ is locked -- any other condition would be unsound. The correct way to deal with this situation is to mark the is_locked() method with an attribute, e.g. TEST_LOCKED_FUNCTION, rather than relying on the AssertHeld mechanism. That's a lot more work, but I would prefer to at least have a TODO comment indicating that the AssertHeld is a workaround, and restrict the mixing of assert/lock to managed locks in the mean time. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102026/new/ https://reviews.llvm.org/D102026 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D98747: Thread safety analysis: Don't warn about managed locks on join points
delesley accepted this revision. delesley added a comment. This revision is now accepted and ready to land. I am convinced by your argument. I think it is reasonable to assume that if someone is using an RAII object, then the underlying object is responsible for managing double locks/unlocks, and we can restrict our efforts to detecting race conditions. Thanks for the detailed explanation! :-) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98747/new/ https://reviews.llvm.org/D98747 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D98747: Thread safety analysis: Don't warn about managed locks on join points
delesley added a comment. Thanks for roping me into the conversation, Aaron, and sorry about the delay. I have mixed feelings about this patch. With respect to the purpose of thread safety analysis, finding race conditions is obviously a major concern, because race conditions are hard to find in tests. However, preventing deadlocks is almost as important, and can be even more important in certain critical applications. The consequence of a double unlock is usually a crash or an exception with an error message. The consequence of a double lock is often a deadlock, depending on the underlying thread library, which is a more serious problem that is harder to fix. Thus, I am concerned about the change in the test case on line 2895, where a potential deadlock has gone from detected, to no warning. Deadlocks are often not found in tests, because test coverage is never perfect over all possible control-flow paths. The use cases here are also slightly different. A MutexLock object uses the RAII design pattern to ensure that the underlying mutex is unlocked on every control flow path, and presumably includes logic to prevent double releases/unlocks, as is usually the case with RAII. The RAII pattern is sufficiently robust that we can "trust the implementation", and relax static checks. A MutexUnlock object inverts the RAII pattern. Because a lock is being acquired on different control-flow paths, rather than being released, it may make sense to keep the stronger checks in order to guard against potential deadlocks. On the other hand, we are still using RAII, so the implementer of MutexUnlock could presumably insert logic to guard against deadlocks if that was a concern. Thus, I could be persuaded that deadlocks are less important in this particular case. Is there a compelling reason for this change, other than consistency/symmetry concerns? In other words, do you have real-world code where you are getting false positives when using MutexUnlock? If so, then I am in favor of the change. If not, then I tend to err on the side of "if it ain't broke don't fix it." :-) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98747/new/ https://reviews.llvm.org/D98747 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D59523: Thread Safety: also look at ObjC methods
delesley accepted this revision. delesley added a comment. This revision is now accepted and ready to land. The if logic does not enhance readability, but I suppose it can't be helped. Looks good to me. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59523/new/ https://reviews.llvm.org/D59523 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities
delesley accepted this revision. delesley added a comment. Just to be clear, I'm approving the change, but I'd still like the methods to be renamed. :-) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52578/new/ https://reviews.llvm.org/D52578 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities
delesley added inline comments.
Comment at: lib/Analysis/ThreadSafety.cpp:951
+}
} else {
+// We're removing the underlying mutex. Warn on double unlocking.
aaronpuchert wrote:
> aaronpuchert wrote:
> > delesley wrote:
> > > aaronpuchert wrote:
> > > > delesley wrote:
> > > > > I find this very confusing. A lock here unlocks the underlying
> > > > > mutex, and vice versa. At the very least, some methods need to be
> > > > > renamed, or maybe we can have separate classes for ScopedLockable and
> > > > > ScopedUnlockable.
> > > > I agree that it's confusing, but it follows what I think was the idea
> > > > behind scoped capabilities: that they are also capabilities that can be
> > > > acquired/released. Now since the scoped capability releases a mutex on
> > > > construction (i.e. when it is acquired), it has to acquire the mutex
> > > > when released. So the way it handles the released mutexes mirrors what
> > > > happens on the scoped capability itself.
> > > >
> > > > It's definitely not very intuitive, but I feel it's the most consistent
> > > > variant with what we have already.
> > > >
> > > > The nice thing about this is that it works pretty well with the
> > > > existing semantics and allows constructs such as `MutexLockUnlock` (see
> > > > the tests) that unlocks one mutex and locks another. Not sure if anyone
> > > > needs this, but why not?
> > > A scoped_lockable object is not a capability, it is an object that
> > > manages capabilities. IMHO, conflating those two concepts is a bad idea,
> > > and recipe for confusion. You would not, for example, use a
> > > scoped_lockable object in a guarded_by attribute.
> > >
> > > Unfortunately, the existing code tends to conflate "capabilities" and
> > > "facts", which is my fault. A scoped_lockable object is a valid fact --
> > > the fact records whether the object is in scope -- it's just not a valid
> > > capability.
> > >
> > > Simply renaming the methods from handleLock/Unlock to addFact/removeFact
> > > would be a nice first step in making the code clearer, and would
> > > distinguish between facts that refer to capabilities, and facts that
> > > refer to scoped objects.
> > >
> > >
> > >
> > >
> > Makes sense to me. I'll see if I can refactor some of the code to clarify
> > this.
> >
> > I think that we should also separate between unlocking the underlying
> > mutexes and destructing the scoped_lockable, which is currently handled via
> > the extra parameter `FullyRemove` of `handleUnlock`.
> Scoped capabilities are not the same as capabilities, but they are also more
> than just facts. (Except if you follow
> [Wittgenstein](https://en.wikipedia.org/wiki/Tractatus_Logico-Philosophicus#Proposition_1),
> perhaps.) They can be released, and reacquired, which makes them more than
> mere immutable facts. I think we can consider them as proxy capabilities.
> They cannot directly protect any resource, but we can acquire and release
> (actual) capabilities through them. That's why it doesn't sound crazy to me
> to say that releasing a scoped capability might acquire an underlying mutex,
> if that was released on construction of the scoped capability.
The notion of "proxy" is how scoped objects have always been treated in the
past, but that's mostly a historical accident, rather than a well-reasoned
design choice. As a bit of history, the first version of thread safety
analysis only tracked mutexes. Thus, the original implementation pretended
that a scoped object was a mutex, because that's the only thing that the
analysis could deal with.
As the analysis grew more complex, I switched to the current system based on
"facts". There are a number of facts that are potentially useful in static
analysis, such as whether one expression aliases another, and most of them
don't look at all like capabilities. IMHO, knowing whether an object is within
scope falls into this class. The basic feature of facts is that they are
control flow dependent -- they can be added to sets, removed from sets, and
removed from intersections at joint points in the CFG. Renaming the methods to
reflect this design will make future extensions easier to understand.
The only real argument for treating scoped lockable objects as proxies, which
can be "locked" and "unlocked", is that you can (in a future patch) reuse the
existing acquire_capability and release_capability attributes to support
releasing and then re-acquiring the proxy. It's a bit counter-intuitive, but
the alternative is adding new attributes, which is also bad.
However, even if you reuse the existing attributes, when it comes time to
define a ScopedUnlockable class, you *still* shouldn't provide methods named
"lock" and "unlock", where "lock" releases the underlying mutex, and "unlock"
reacquires it. That's bad API design which will be endlessly confusing to
users.
Thus, I would still
[PATCH] D52443: Thread safety analysis: Examine constructor arguments
delesley accepted this revision.
delesley added inline comments.
This revision is now accepted and ready to land.
Comment at: lib/Analysis/ThreadSafety.cpp:2046
const CXXConstructorDecl *D = Exp->getConstructor();
if (D && D->isCopyConstructor()) {
const Expr* Source = Exp->getArg(0);
aaronpuchert wrote:
> delesley wrote:
> > As a side note, we should probably special-case the move constructor too,
> > with AK_Written. That should be in a separate patch, though, and needs to
> > be sequestered under -Wthread-safety-beta, which is complicated.
> I think your arguments from D52395 apply here as well: the move constructor
> does not necessarily write. Many simple types are trivially move
> constructible, and then the move constructor is effectively the same as the
> copy constructor.
Good point.
Repository:
rC Clang
https://reviews.llvm.org/D52443
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities
delesley added inline comments.
Comment at: lib/Analysis/ThreadSafety.cpp:893
private:
- SmallVector UnderlyingMutexes;
+ enum UnderlyingCapabilityKind {
+UCK_Acquired, ///< Any kind of acquired capability.
aaronpuchert wrote:
> delesley wrote:
> > IMHO, it would make more sense to break this into two properties (or bits):
> > acquired/released and shared/exclusive.
> We don't use the information (shared/exclusive) for acquired locks, but we
> need two bits anyway, so why not.
The normal unlock doesn't distinguish between shared/exclusive, but there is a
release_shared_capability which does...
Repository:
rC Clang
https://reviews.llvm.org/D52578
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities
delesley added inline comments.
Comment at: lib/Analysis/ThreadSafety.cpp:951
+}
} else {
+// We're removing the underlying mutex. Warn on double unlocking.
aaronpuchert wrote:
> delesley wrote:
> > I find this very confusing. A lock here unlocks the underlying mutex, and
> > vice versa. At the very least, some methods need to be renamed, or maybe
> > we can have separate classes for ScopedLockable and ScopedUnlockable.
> I agree that it's confusing, but it follows what I think was the idea behind
> scoped capabilities: that they are also capabilities that can be
> acquired/released. Now since the scoped capability releases a mutex on
> construction (i.e. when it is acquired), it has to acquire the mutex when
> released. So the way it handles the released mutexes mirrors what happens on
> the scoped capability itself.
>
> It's definitely not very intuitive, but I feel it's the most consistent
> variant with what we have already.
>
> The nice thing about this is that it works pretty well with the existing
> semantics and allows constructs such as `MutexLockUnlock` (see the tests)
> that unlocks one mutex and locks another. Not sure if anyone needs this, but
> why not?
A scoped_lockable object is not a capability, it is an object that manages
capabilities. IMHO, conflating those two concepts is a bad idea, and recipe
for confusion. You would not, for example, use a scoped_lockable object in a
guarded_by attribute.
Unfortunately, the existing code tends to conflate "capabilities" and "facts",
which is my fault. A scoped_lockable object is a valid fact -- the fact
records whether the object is in scope -- it's just not a valid capability.
Simply renaming the methods from handleLock/Unlock to addFact/removeFact would
be a nice first step in making the code clearer, and would distinguish between
facts that refer to capabilities, and facts that refer to scoped objects.
Comment at: lib/Analysis/ThreadSafety.cpp:992
+FSet.removeLock(FactMan, UnderCp);
+FSet.addLock(FactMan, std::move(UnderEntry));
+ }
aaronpuchert wrote:
> delesley wrote:
> > Shouldn't this be outside of the else?
> If we don't find `UnderCp` anymore, the negation should be there already. But
> I can also keep it outside if you like.
That's not necessarily true. Not knowing whether a mutex is locked is
different from knowing that it is unlocked.
You're probably right in this case, because capabilities that are managed by
scoped objects obey a more restricted set of rules. But then, you're also
extending the ways in which scoped objects can be used. :-)
Repository:
rC Clang
https://reviews.llvm.org/D52578
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference
delesley added a comment. For future patches, please add Richard Trieu (rtr...@google.com) as a subscriber. I will continue to try and do code reviews, but Richard is on the team that actually rolls out new compiler changes. Thanks! BTW, the issue is not just that changes may introduce false positives. Even if it's a true positive, you risk breaking the build. Repository: rC Clang https://reviews.llvm.org/D52395 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities
delesley added inline comments.
Comment at: lib/Analysis/ThreadSafety.cpp:893
private:
- SmallVector UnderlyingMutexes;
+ enum UnderlyingCapabilityKind {
+UCK_Acquired, ///< Any kind of acquired capability.
IMHO, it would make more sense to break this into two properties (or bits):
acquired/released and shared/exclusive.
Comment at: lib/Analysis/ThreadSafety.cpp:916
+for (const auto : ShrdRel)
+ UnderlyingMutexes.emplace_back(M.sexpr(), UCK_ReleasedExclusive);
}
UCK_ReleasedShared? (Shouldn't this have been caught by a test case?)
Comment at: lib/Analysis/ThreadSafety.cpp:926
+ FactMan, CapabilityExpr(UnderlyingMutex.getPointer(), false));
+ if ((UnderlyingMutex.getInt() == UCK_Acquired) == (Entry != nullptr)) {
// If this scoped lock manages another mutex, and if the underlying
This if statement makes my head hurt. Can you find a different way of
expressing it?
Comment at: lib/Analysis/ThreadSafety.cpp:951
+}
} else {
+// We're removing the underlying mutex. Warn on double unlocking.
I find this very confusing. A lock here unlocks the underlying mutex, and vice
versa. At the very least, some methods need to be renamed, or maybe we can
have separate classes for ScopedLockable and ScopedUnlockable.
Comment at: lib/Analysis/ThreadSafety.cpp:992
+FSet.removeLock(FactMan, UnderCp);
+FSet.addLock(FactMan, std::move(UnderEntry));
+ }
Shouldn't this be outside of the else?
Repository:
rC Clang
https://reviews.llvm.org/D52578
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52443: Thread safety analysis: Examine constructor arguments
delesley added inline comments.
Comment at: lib/Analysis/ThreadSafety.cpp:2046
const CXXConstructorDecl *D = Exp->getConstructor();
if (D && D->isCopyConstructor()) {
const Expr* Source = Exp->getArg(0);
As a side note, we should probably special-case the move constructor too, with
AK_Written. That should be in a separate patch, though, and needs to be
sequestered under -Wthread-safety-beta, which is complicated.
Repository:
rC Clang
https://reviews.llvm.org/D52443
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52443: Thread safety analysis: Examine constructor arguments
delesley added a comment. This looks good, and resolves an outstanding bug that was on my list. Thanks for the patch! Comment at: lib/Analysis/ThreadSafety.cpp:1537 void handleCall(const Expr *Exp, const NamedDecl *D, VarDecl *VD = nullptr); + void ExamineCallArguments(const FunctionDecl *FD, +CallExpr::const_arg_iterator ArgBegin, Nit: capitalization does not match other methods. Repository: rC Clang https://reviews.llvm.org/D52443 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference
delesley added a comment. With respect to data, I really think these patches should be tested against Google's code base, because otherwise you're going to start seeing angry rollbacks. However, I don't work on the C++ team any more, and I don't have time to do it. When I was actively developing the analysis, I spent about 90% of my time just running tests and fixing the code base. Each incremental improvement in the analysis itself was a hard upward slog. If you're going to be adding lots of improvements, we need to have someone at Google running point. Repository: rC Clang https://reviews.llvm.org/D52395 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference
delesley added a comment. I have mixed feelings about this patch. When you pass an object to a function, either by pointer or by reference, no actual load from memory has yet occurred. Thus, there is a real risk of false positives; what you are saying is that the function *might* read or write from protected memory, not that it actually will. In fact, the false positive rate is high enough that this particular warning is sequestered under -Wthread-safety-reference, and is not used with warnings-as-errors at Google. In theory, the correct way to deal with references is to make GUARDED_BY an attribute of the type, rather than an attribute of the data member; that way taking the address of a member, or passing it by reference, wouldn't cast away the annotation. But that would require a properly parametric dependent type system for C++, which is pretty much impossible. So you're left with two bad options: either issue a warning eagerly, and deal with false positives, or suppress the warning, and deal with false negatives. At Google, false positives usually break the build, which has forced me to prefer false negatives in most cases; my strategy has been to gradually tighten the analysis bit by bit. Thankfully, that's less of a concern here. Adding to the difficulty is the fact that the correct use of "const" in real-world C++ code is spotty at best. There is LOTS of code that arguably should be using const, but doesn't for various reasons. E.g. if program A calls library B, and library B forgot to provide const-qualified versions of a few methods, than A has to make a choice: either drop the const qualifier, or insert ugly const-casts, neither of which is pleasant. In other-words, non-constness has a tendency to propagate through the code base, and you can't depend on "const" being accurate. I have two recommendations. First think the default behavior should be to require only a read-only lock, as it currently does. That's a compromise between the "false-positive" and "false-negative" camps. It catches a lot of errors, because you have to hold the lock in some way, but doesn't require accurate const-ness. For people who want more accuracy, there should be a different variant of the warning -- maybe -Wthread-safety-reference-precise? Between beta/precise/reference etc. there are a now a lot of analysis options, and I would love to consolidate them in some fashion. However, different code bases have different needs, and I think "one size fits all" is not really appropriate. Second, there needs to be a thread-safety-variant of "const_cast" -- i.e. a way to pass a reference to a function without triggering the warning. That may already be possible via clever use of our current annotations, or you may have to fiddle with something, but it needs to appear in the test cases. Repository: rC Clang https://reviews.llvm.org/D52395 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51187: [RFC] Thread safety analysis: Track status of scoped capability
delesley added a comment. In https://reviews.llvm.org/D51187#1241354, @aaronpuchert wrote: > Thanks for pointing out my error! Ignoring the implementation for a moment, > do you think this is a good idea or will it produce too many false positives? > Releasable/relockable scoped capabilities that I have seen keep track of the > status, so it makes sense, but maybe there are others out there. I think this is probably a good idea. Code which manipulates the underlying mutex while it is being managed by another object is a recipe for trouble. It's hard for me to guess how many false positives it will generate without actually running it over our code base, which I personally don't have time to do right now. It should definitely go in -Wthread-safety-beta so it won't break the build. Unfortunately, the before/after checks are still in thread-safety-beta, and they should really be moved out of beta before something else is moved in. The good news is that Matthew O'Niel just volunteered to do that. That is, unfortunately, not a trivial operation, so it may be some weeks before he's done. Repository: rC Clang https://reviews.llvm.org/D51187 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51187: [RFC] Thread safety analysis: Track status of scoped capability
delesley added inline comments. Comment at: lib/Analysis/ThreadSafety.cpp:929 + return Handler.handleDoubleLock(DiagKind, entry.toString(), entry.loc()); +Locked = true; + It's been a while since I last looked at this code, but I don't think you can use mutable fields in a FactEntry. The analysis creates a FactSet for each program point, but each FactSet simply has pointers (FactIDs) for the underlying FactEntries. If you change the definition of a FactEntry, it will change that definition for every program point. So if you do an unlock on one side of a branch, and change the underlying FactEntry to reflect that, then analysis will then think you have also done the unlock on the other side of the branch. Any changes should always be done by adding or removing entries from the FactSet, not by mutating the underlying FactEntries. Repository: rC Clang https://reviews.llvm.org/D51187 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate
delesley added inline comments.
Comment at: lib/Analysis/ThreadSafetyCommon.cpp:362
+ til::Project *P = new (Arena) til::Project(E, D);
+ if (hasCppPointerType(BE))
+P->setArrow(true);
aaronpuchert wrote:
> rjmccall wrote:
> > aaron.ballman wrote:
> > > I feel like this will always return false. However, I wonder if
> > > `IVRE->getBase()->isArrow()` is more appropriate here?
> > The base of an `ObjCIvarRefExpr` will never directly be a C pointer type.
> > I don't know whether this data structure looks through casts, but it's
> > certainly *possible* to cast arbitrarily weird C stuff to ObjC pointer
> > type. On the other hand, by and large nobody actually ever does that, so I
> > wouldn't make a special case for it here.
> >
> > `ObjCIvarRefExpr` just has an `isArrow()` method directly if this is just
> > supposed to be capturing the difference between `.` and `->`. But then, so
> > does `MemberExpr`, and in that case you're doing this odd analysis of the
> > base instead of trusting `isArrow()`, so I don't know what to tell you to
> > do.
> >
> > Overall it is appropriate to treat an ObjC ivar reference as a perfectly
> > ordinary projection. The only thing that's special about it is that the
> > actual offset isn't necessarily statically known, but ivars still behave as
> > if they're all independently declared, so I wouldn't worry about it.
> I had wondered about this as well, but when I changed `hasCppPointerType(BE)`
> to `ME->isArrow()` in the `MemberExpr` case, I got some test failures. For
> example:
>
> ```
> struct Foo {
> int a __attribute__((guarded_by(mu)));
> Mutex mu;
> };
>
> void f() {
> Foo foo;
> foo.a = 5; // \
> // expected-warning{{writing variable 'a' requires holding mutex 'foo.mu'
> exclusively}}
> }
> ```
>
> Instead we warn `writing variable 'a' requires holding mutex 'foo->mu'
> exclusively`. That's not right.
>
> The expression (`ME`) we are processing is `mu` from the attribute on `a`,
> which the compiler sees as `this->mu`. So we get `ME->isArrow() == true` and
> `ME->getBase()` is a `CXXThisExpr`.
>
> Basically the `translate*` functions do a substitution. They try to derive
> `foo.mu` from `this->mu` on the `a` member and the expression `foo.a`. The
> annotation `this->mu` is the expression we get as parameter, and `foo.a` is
> encoded in the `CallingContext`.
>
> The information whether `foo.a` is an arrow (it isn't) seems to be in the
> `CallingContext`'s `SelfArrow` member.
This is a recurring problem. The fundamental issue is the analysis must
compare expressions for equality, but many C++ expressions are syntactically
different but semantically the same, like ()->mu and foo.mu. It's
particularly problematic because (as Aaron notes) we frequently substitute
arguments when constructing expressions, which makes it easy to get things like
()->mu instead of foo.mu, when substituting for a pointer argument.
The purpose of the TIL is to translate C++ expressions into a simpler,
lower-level IR, where syntactic equality in the IR corresponds to semantic
equality between expressions. The TIL doesn't distinguish between pointers and
references, doesn't distinguish between -> and ., and ignores * and & as
no-ops. But then we have to recover the distinction between -> and . when we
generate an error message.
In the presence of substitution, you can't go by whether the source syntax has
an ->. You have to look at whether the type of the underlying argument (after
stripping away * and &) requires an arrow.
I know nothing about objective C, so I don't know what the right answer is here.
Repository:
rC Clang
https://reviews.llvm.org/D52200
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments
delesley accepted this revision. delesley added a comment. This revision is now accepted and ready to land. This looks okay to me, but I have not tested it on a large code base to see if it breaks anything. Comment at: lib/Sema/SemaDeclAttr.cpp:589 +<< AL << MD->getParent(); +} else + S.Diag(AL.getLoc(), diag::warn_thread_attribute_not_on_non_static_member) Curly braces around the else section. Repository: rC Clang https://reviews.llvm.org/D51901 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49885: Thread safety analysis: Allow relockable scopes
delesley accepted this revision. delesley added a comment. This looks good to me. Thanks for the patch. Comment at: lib/Analysis/ThreadSafety.cpp:932 + // We're relocking the underlying mutexes. Warn on double locking. + if (FSet.findLock(FactMan, UnderCp)) +Handler.handleDoubleLock(DiagKind, UnderCp.toString(), entry.loc()); Minor nit. Use curly braces on the if, to match the else. Repository: rC Clang https://reviews.llvm.org/D49885 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49355: Thread safety analysis: Allow lock upgrading and downgrading
delesley accepted this revision. delesley added a comment. This revision is now accepted and ready to land. Looks good to me. Thanks for the patch, and my apologies for the slow response. (I'm on a different project now, so I'm afraid thread safety doesn't always get the attention from me that it deserves.) -DeLesley Repository: rC Clang https://reviews.llvm.org/D49355 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41933: handle scoped_lockable objects being returned by value in C++17
delesley accepted this revision. delesley added a comment. LGTM. Thanks, Richard! Repository: rC Clang https://reviews.llvm.org/D41933 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36237: Reland "Thread Safety Analysis: fix assert_capability.", add warnings
delesley added a comment. Overall looks good. However, I would change the wording on the warning to the following. The terms "free function" and "instance method" may be confusing to some people. Also, warn_thread_attribute_noargs_static_method should not mention the capability attribute, which is checked separately in warn_thread_attribute_noargs_not_lockable. def warn_thread_attribute_noargs_not_method: ... "%0 attribute without arguments can only be applied to a method of a class." ... def warn_thread_attribute_noargs_static_method: ... "%0 attribute without arguments cannot be applied a static method." ... https://reviews.llvm.org/D36237 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36122: Thread Safety Analysis: fix assert_capability.
delesley accepted this revision. delesley added a comment. This revision is now accepted and ready to land. Thanks! Repository: rL LLVM https://reviews.llvm.org/D36122 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29685: Lit C++11 Compatibility - Function Attributes
delesley added a comment. I agree with Aaron here. Those thread safety errors are supposed to fire; simply disabling the unit tests because they no longer fire is not acceptable. I also don't understand how this could be a bug with the thread safety analysis, since these particular errors are issued when the attributes are parsed, not when the analysis is run. I haven't looked at the relevant code in a long time; have there been significant changes to the attribute-parsing mechanism? There's no member initialization going on here, so why would changes to member initialization have this side effect? I'm confused. DeLesley https://reviews.llvm.org/D29685 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28520: Disable -Wthread-safety-analysis for some functions in __thread_support
delesley added a comment. This looks good to me. https://reviews.llvm.org/D28520 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28520: Disable -Wthread-safety-analysis for some functions in __thread_support
delesley added a comment. The big question for me is whether these functions are exposed as part of the public libcxx API, or whether they are only used internally. (Again, I'm not a libcxx expert.) If they are part of the public API, then users may want to switch them on and off in their own code. In that case, I'm happy with the current variant. If they are only used internally, then no_thread_safety_analysis is probably the correct option. (Unless, of course, the libcxx developers want to start using the analysis themselves, but I suspect they don't.) https://reviews.llvm.org/D28520 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28520: Disable -Wthread-safety-analysis for some functions in __thread_support
delesley added a comment. Sorry about the slow response. My main concern here is that the thread safety analysis was designed for use with a library that wraps the system mutex in a separate Mutex class. We did that specifically to avoid breaking anything; code has to opt-in to the static checking by defining and using a Mutex class, and the API of that class is restricted to calls that can be easily checked via annotations. Including attributes directly in the standard library has the potential to cause lots of breakage and false positives. Is there some way to control the #ifdefs so that the annotations are turned off by default for everyone except possibly freebsd, but there's still a way to turn them on for users who want to see the warnings? I'm not a libcxx expert. https://reviews.llvm.org/D28520 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
