[PATCH] D51187: [RFC] Thread safety analysis: Track status of scoped capability
aaronpuchert added a comment.
> Any changes should always be done by adding or removing entries from the
> FactSet, not by mutating the underlying FactEntries.
To make that clearer in the code, I made `FactEntry`s immutable that are
managed by `FactManager` in https://reviews.llvm.org/rC342787.
In https://reviews.llvm.org/D51187#1242620, @delesley wrote:
> 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.
That's Ok for me. It's not something that I terribly need, but it seemed to
make things more consistent.
Does the migration of the before/after checks include changes to how it is
handled? Because I wondered why it doesn't work on S-expressions like the rest
of the analysis, but just `ValueDecl`s. That leads to some false positives with
more complex expressions:
class A {
public:
Mutex mu1;
Mutex mu2 ACQUIRED_AFTER(mu1);
};
class B {
A a1, a2;
Mutex lm ACQUIRED_AFTER(a1.mu2);
public:
void f() {
a2.mu2.Lock();
a1.mu1.Lock();// warns "mutex 'mu1' must be acquired before 'mu2'",
but they are on different objects
a1.mu1.Unlock();
a2.mu2.Unlock();
}
void g() {
lm.Lock();
a1.mu1.Lock();
a1.mu1.Unlock();
a2.mu1.Lock();// Warns "mutex 'mu1' must be acquired before 'lm'",
but lm talks only about a1.mu2.
a2.mu1.Unlock();
lm.Unlock();
}
};
But maybe that's not the right place to discuss this.
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 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
aaronpuchert added a comment. Thanks for pointing out my error! Ignoring the implementation for a moment, do you think this is a good idea or will it produce to 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. Comment at: lib/Analysis/ThreadSafety.cpp:929 + return Handler.handleDoubleLock(DiagKind, entry.toString(), entry.loc()); +Locked = true; + delesley wrote: > 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. > That explains why there are problems with back edges. Now that you say it I'm actually aware of how the `FactManager` and `FactSet` work, but apparently I didn't consider that here. Would you mind if I make a separate change that returns only `const FactEntry*` from the `FactManager`? Maybe that makes it harder to write wrong code in the future. 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] D51187: [RFC] Thread safety analysis: Track status of scoped capability
aaronpuchert planned changes to this revision.
aaronpuchert added a comment.
This doesn't work with loops yet:
void relockLoop() {
RelockableExclusiveMutexLock scope();
while (b) {
scope.Unlock(); // releasing mutex 'scope' that was not held
scope.Lock(); // acquiring mutex 'mu' that is already held
}
}
There should be no warnings here — this code is fine.
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
