On Wed, Aug 22, 2018 at 6:35 PM, Aaron Puchert via Phabricator
wrote:
> aaronpuchert added inline comments.
>
>
>
> Comment at: lib/Analysis/ThreadSafety.cpp:932
> + // We're relocking the underlying mutexes. Warn on double locking.
> + if (FSet.findLock(FactMan, UnderCp
aaronpuchert added inline comments.
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());
--
This revision was automatically updated to reflect the committed changes.
Closed by commit rL340459: Thread safety analysis: Allow relockable scopes
(authored by aaronpuchert, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D49885
Files:
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))
+
aaronpuchert updated this revision to Diff 161598.
aaronpuchert added a comment.
Reformat tests. I promise, this is the last one.
Repository:
rC Clang
https://reviews.llvm.org/D49885
Files:
lib/Analysis/ThreadSafety.cpp
test/SemaCXX/warn-thread-safety-analysis.cpp
Index: test/SemaCXX/wa
aaronpuchert updated this revision to Diff 161596.
aaronpuchert added a comment.
Formatting changes.
Repository:
rC Clang
https://reviews.llvm.org/D49885
Files:
lib/Analysis/ThreadSafety.cpp
test/SemaCXX/warn-thread-safety-analysis.cpp
Index: test/SemaCXX/warn-thread-safety-analysis.cpp
aaronpuchert updated this revision to Diff 161595.
aaronpuchert added a comment.
Proper capitalization of member function, reduction of test code.
Repository:
rC Clang
https://reviews.llvm.org/D49885
Files:
lib/Analysis/ThreadSafety.cpp
test/SemaCXX/warn-thread-safety-analysis.cpp
Index
aaronpuchert marked 4 inline comments as done.
aaronpuchert added a comment.
@aaron.ballman Maybe you can have a look again — this is much more elegant. I'm
not sure why I didn't see this in the first place.
Comment at: test/SemaCXX/warn-thread-safety-analysis.cpp:2765-2768
+
aaronpuchert updated this revision to Diff 160719.
aaronpuchert added a comment.
Found a much simpler solution. After introducing a new virtual function
HandleLock() in FactEntry, I just needed to change two lines in
ThreadSafetyAnalyzer::addLock. Changes in BuildLockset::handleCall are no
long
aaronpuchert added a comment.
The case distinction in `case attr::AcquireCapability` is not very beautiful,
but it's due to the fact that scoped capabilities are not "real" capabilities
and so we need to distinguish them.
What this still doesn't allow for is attributes on other classes than the
aaronpuchert updated this revision to Diff 160505.
aaronpuchert added a comment.
Fix crash. The problem was that ACQUIRES with arguments did not no longer have
(only) `this` as argument, hence our assumption that we would have only
ScopedLockableFactEntry's was false. So now we lock a bit closer
aaronpuchert reopened this revision.
aaronpuchert added a comment.
This revision is now accepted and ready to land.
This didn't cross my mind, because an `ACQUIRES` attribute with arguments on a
function other than the constructor does not add the argument locks to the set
of managed mutexes. So
hokein added a comment.
Hello, this patch seems introduce a new crash, and I have reverted it in
r339558.
Here is the minimal test case:
class SCOPED_LOCKABLE FileLock {
public:
explicit FileLock()
EXCLUSIVE_LOCK_FUNCTION(file_);
~FileLock() UNLOCK_FUNCTION(file_);
//vo
aaron.ballman closed this revision.
aaron.ballman added a comment.
Committed in r339456; if @delesley has concerns with the commit, they can be
addressed post-commit. Thank you for the patch!
Repository:
rC Clang
https://reviews.llvm.org/D49885
aaronpuchert marked an inline comment as done.
aaronpuchert added a comment.
@delesley Did you have a chance to look at this yet?
Repository:
rC Clang
https://reviews.llvm.org/D49885
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://
aaronpuchert marked 3 inline comments as done.
aaronpuchert added inline comments.
Comment at: lib/Analysis/ThreadSafety.cpp:960-961
+FSet.removeLock(FactMan, !UnderCp);
+FSet.addLock(FactMan, llvm::make_unique(UnderCp, LK,
+
aaronpuchert updated this revision to Diff 157872.
aaronpuchert added a comment.
Formatting changes suggested by Aaron Ballman.
Repository:
rC Clang
https://reviews.llvm.org/D49885
Files:
lib/Analysis/ThreadSafety.cpp
test/SemaCXX/warn-thread-safety-analysis.cpp
Index: test/SemaCXX/warn
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
The changes look reasonable to me (after fixing a few nits), but please give
@delesley a chance to weigh in before committing.
Comment at: lib/Analysis/ThreadS
aaronpuchert added a comment.
Imagine having a producer loop, where we check a loop condition while holding a
mutex, but release it in the loop body to let other producers/consumers do
their work. In that scenario it makes sense to allow "relocking" a scope.
RelockableScope Scope(mu);
while
aaronpuchert created this revision.
aaronpuchert added reviewers: delesley, aaron.ballman.
Herald added a subscriber: cfe-commits.
It's already allowed to prematurely release a scoped lock, now we also
allow relocking it again, possibly even in another mode.
Arguably the solution is not very eleg
20 matches
Mail list logo
