[PATCH] D50110: Handle shared release attributes correctly

aaronpuchert added inline comments. Comment at: test/SemaCXX/warn-thread-safety-analysis.cpp:4081-4085 + void unlockExclusive() EXCLUSIVE_UNLOCK_FUNCTION(mu_) { +mu_.Unlock(); + } + + void unlockShared() SHARED_UNLOCK_FUNCTION(mu_) { aaron.ballman wrote:

[PATCH] D50110: Handle shared release attributes correctly

aaronpuchert added a comment. Thanks for the review! Could you commit for me again? Repository: rC Clang https://reviews.llvm.org/D50110 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

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, +

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

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:

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

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);

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

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

[PATCH] D49355: Thread safety analysis: Allow lock upgrading and downgrading

aaronpuchert added a comment. No problem. Thanks for the review! Would be nice if you or @aaron.ballman could commit this, as I don't have commit access. Repository: rC Clang https://reviews.llvm.org/D49355 ___ cfe-commits mailing list

[PATCH] D50110: Handle shared release attributes correctly

aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley. Herald added a subscriber: cfe-commits. Repository: rC Clang https://reviews.llvm.org/D50110 Files: lib/Analysis/ThreadSafety.cpp test/SemaCXX/warn-thread-safety-analysis.cpp Index:

[PATCH] D49355: Thread safety analysis: Allow lock upgrading and downgrading

aaronpuchert added a comment. Could you explain what annotations like `LOCK_UNLOCK` are useful for? What do they check? The annotation should certainly not be necessary. Shouldn't you just use `REQUIRES(!...)` or `EXCLUDES(...)`? If a function locks and unlocks a mutex, I don't see a reason to

[PATCH] D49355: Thread safety analysis: Allow lock upgrading and downgrading

aaronpuchert added a comment. In https://reviews.llvm.org/D49355#1188603, @phosek wrote: > In https://reviews.llvm.org/D49355#1188520, @aaronpuchert wrote: > > > Could you explain what annotations like `LOCK_UNLOCK` are useful for? What > > do they check? The annotation should certainly not be

[PATCH] D50110: Handle shared release attributes correctly

aaronpuchert added a comment. For now I think I'm done fixing things in the thread safety analysis, but if I see myself contributing more in the longer term, I will definitely try to obtain commit access. Repository: rC Clang https://reviews.llvm.org/D50110

[PATCH] D49355: Thread safety analysis: Allow lock upgrading and downgrading

aaronpuchert added a comment. It seems @phosek was able to fix the issue in https://github.com/flutter/engine/pull/5944. By the way, a nice way to think about the attributes is that they encode state transitions as shown in the following table. This change fills the remaining two gaps. |

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

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

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

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

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

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

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

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.

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

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

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

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 +

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

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:

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

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:

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

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

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

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());

[PATCH] D51187: Thread safety analysis: Warn on double (un-)lock of scoped capability

aaronpuchert created this revision. aaronpuchert added reviewers: delesley, aaron.ballman. Herald added a subscriber: cfe-commits. We now warn when acquiring or releasing a scoped capability a second time, not just if the underlying mutexes have been acquired or released a second time. It's

[PATCH] D51187: Thread safety analysis: Warn on double (un-)lock of scoped capability

aaronpuchert added a comment. Sure. As I wrote in the commit message, I'm not sure about it myself, but I think it's worth a discussion. Maybe I should have tagged it as RFC. Releasable scopes need a way of knowing whether the lock is currently held to prevent double unlocking in the

[PATCH] D51187: Thread safety analysis: Warn on double (un-)lock of scoped capability

aaronpuchert updated this revision to Diff 162562. aaronpuchert added a comment. Use Locked flag to determine whether to unlock on destruction Instead of unlocking the mutexes that are still available while not complaining about those that aren't, we use the status of the scoped capability to

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

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:

[PATCH] D49355: Thread safety analysis: Allow lock upgrading and downgrading

aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley. Herald added a subscriber: cfe-commits. We can now have methods that release a locked in shared mode and acquire it in exclusive mode or the other way around. The fix was just to release the locks before

[PATCH] D49355: Thread safety analysis: Allow lock upgrading and downgrading

aaronpuchert added a comment. Ping? The functional changes should be minimal. Comment at: test/SemaCXX/warn-thread-safety-analysis.cpp:38-39 #endif +#define EXCLUSIVE_UNLOCK_FUNCTION(...) __attribute__((release_capability(__VA_ARGS__))) +#define SHARED_UNLOCK_FUNCTION(...)

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley, jmgao. Herald added a subscriber: cfe-commits. When thread safety annotations are used without capability arguments, they are assumed to apply to `this` instead. So we warn when either `this` doesn't exist,

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

aaronpuchert added a comment. The second warning message is a bit long, so if any of you have a better idea I'd welcome it. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3016-3017 +def warn_thread_attribute_not_on_capability_member : Warning< + "%0 attribute without

[PATCH] D49275: Thread safety: Run tests with both lock and capability attributes

aaronpuchert created this revision. aaronpuchert added reviewers: delesley, aaron.ballman. Herald added a subscriber: cfe-commits. Running all tests with both sets of attributes should make sure there is no regression in either variant. Repository: rC Clang https://reviews.llvm.org/D49275

[PATCH] D49275: Thread safety: Run tests with both lock and capability attributes

aaronpuchert added a comment. Thanks for the review. Could you commit this as `Aaron Puchert https://reviews.llvm.org/D49275 ___ 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

aaronpuchert marked an inline comment as done. aaronpuchert added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3016-3017 +def warn_thread_attribute_not_on_capability_member : Warning< + "%0 attribute without capability arguments can only be applied in

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

aaronpuchert updated this revision to Diff 165004. aaronpuchert added a comment. Improved handling of type-dependent base classes and slightly reworded warning message. Repository: rC Clang https://reviews.llvm.org/D51901 Files: include/clang/Basic/DiagnosticSemaKinds.td

[PATCH] D52443: Thread safety analysis: Examine constructor arguments

aaronpuchert added inline comments. Comment at: lib/Analysis/ThreadSafety.cpp:1970 + // There can be default arguments, so we stop when one iterator is at end(). + for (auto Arg = ArgBegin; Param != Params.end() && Arg != ArgEnd; + ++Param, ++Arg) {

[PATCH] D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference

aaronpuchert added a comment. I've thought about your concerns and the important thing is: I didn't actually test this on a larger code base. The change makes sense to me, but maybe I'm missing something. So maybe instead of pushing through further restrictions, I should focus on rolling out

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

aaronpuchert added a comment. Maybe you should have a look at the tests first. I thought about the semantics that I think you are suggesting, but then we could have code like: class SCOPED_LOCKABLE MutexLockUnlock { public: MutexLockUnlock(Mutex *mu1, Mutex *mu2)

[PATCH] D52888: Thread safety analysis: Handle conditional expression in getTrylockCallExpr

aaronpuchert added a comment. In https://reviews.llvm.org/D52888#1256395, @aaron.ballman wrote: > In https://reviews.llvm.org/D52888#1255862, @aaronpuchert wrote: > > > Additional changes (including some non-tail recursion unfortunately) would > > allow the following to work: > > > > void

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

aaronpuchert planned changes to this revision. aaronpuchert added a comment. I think I'll try to simplify this and address @delesley's comments before we commit this. I'll admit that the semantics are somewhat counter-intuitive, but as I explained I think it's more consistent this way. Because

[PATCH] D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference

aaronpuchert updated this revision to Diff 168505. aaronpuchert added a comment. Rebase on top of https://reviews.llvm.org/D52443. We also check the move constructor argument for write access, as suggested in a review. This isn't intended to be merged (yet?), it should be seen as an RFC.

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

aaronpuchert updated this revision to Diff 170545. aaronpuchert marked 3 inline comments as done. aaronpuchert added a comment. This revision is now accepted and ready to land. Addressed some review comments and simplified the code. There is a lot less duplication and maybe it's even easier to

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

aaronpuchert added a comment. I hope the cleanup makes the code more easily digestible, and to some extent might also transport why I think this is the most elegant approach. I think we should document the semantics of scoped capabilities in more detail, and I will do so once this is either

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

aaronpuchert updated this revision to Diff 171026. aaronpuchert added a comment. Negative capabilities don't need a LockKind. Repository: rC Clang https://reviews.llvm.org/D52578 Files: lib/Analysis/ThreadSafety.cpp test/SemaCXX/warn-thread-safety-analysis.cpp Index:

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

aaronpuchert updated this revision to Diff 171019. aaronpuchert added a comment. Introduced helper functions to clarify lock handling. The previous version was too tightly coupled, and the introduction of AddCp and RemoveCp didn't help readability. Repository: rC Clang

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

aaronpuchert added a comment. @hokein Please have a look at https://reviews.llvm.org/D52888, maybe you can try it out already. The problem was that `?:` expressions are considered a branch point and when merging both branches the warning was emitted. Before this change, we couldn't look into

[PATCH] D52888: Thread safety analysis: Handle conditional expression in getTrylockCallExpr

aaronpuchert added a comment. By the way, the tests also work with `__builtin_expect`, but I didn't want to blow up the code too much. Repository: rC Clang https://reviews.llvm.org/D52888 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D52888: Thread safety analysis: Handle conditional expression in getTrylockCallExpr

aaronpuchert added inline comments. Comment at: lib/Analysis/ThreadSafety.cpp:1445 + if (!TCond && FCond) { +Negate = !Negate; +return getTrylockCallExpr(COP->getCond(), C, Negate); aaron.ballman wrote: > Rather than do an assignment here,

[PATCH] D52888: Thread safety analysis: Handle conditional expression in getTrylockCallExpr

aaronpuchert added inline comments. Comment at: test/SemaCXX/warn-thread-safety-analysis.cpp:1879-1880 + void foo13() { +if (mu.TryLock() ? 1 : 0) + mu.Unlock(); + } aaron.ballman wrote: > aaronpuchert wrote: > > aaron.ballman wrote: > > > Can you add

[PATCH] D52888: Thread safety analysis: Handle conditional expression in getTrylockCallExpr

aaronpuchert added a comment. Additional branches (with non-tail recursion unfortunately) would allow the following to work: void foo16() { if (cond ? mu.TryLock() : false) mu.Unlock(); } void foo17() { if (cond ? true : !mu.TryLock()) return; mu.Unlock(); }

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

This revision was automatically updated to reflect the committed changes. Closed by commit rL343681: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr (authored by aaronpuchert, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

aaronpuchert added inline comments. Comment at: lib/Analysis/ThreadSafety.cpp:893 private: - SmallVector UnderlyingMutexes; + enum UnderlyingCapabilityKind { +UCK_Acquired, ///< Any kind of acquired capability. delesley wrote: > aaronpuchert

[PATCH] D52443: Thread safety analysis: Examine constructor arguments

This revision was automatically updated to reflect the committed changes. Closed by commit rC343831: Thread safety analysis: Examine constructor arguments (authored by aaronpuchert, committed by ). Changed prior to commit: https://reviews.llvm.org/D52443?vs=167856=168414#toc Repository: rC

[PATCH] D52443: Thread safety analysis: Examine constructor arguments

aaronpuchert marked 2 inline comments as done. aaronpuchert added a comment. Since the (remaining) arguments are examined in a separate function, I thought I'd eliminate the boolean variables in `VisitCallExpr`. Apparently I prefer control flow over booleans, but if you disagree I can obviously

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

aaronpuchert added a comment. @delesley Any objections to this? It's certainly useful for our code base, because our `assert`-like macros use `__builtin_expect`, but I'm not sure if that applies to the general public. Repository: rC Clang https://reviews.llvm.org/D52398

[PATCH] D52443: Thread safety analysis: Examine constructor arguments

aaronpuchert updated this revision to Diff 167856. aaronpuchert marked 4 inline comments as done. aaronpuchert added a comment. Moved iterator shifting to VisitCallExpr, eliminated boolean variables, and minor corrections as suggested in the review. There remains no intended functional change

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

aaronpuchert added a comment. In https://reviews.llvm.org/D52398#1255148, @hokein wrote: > Hi, @aaronpuchert, the patch has caused many new warnings in our internal > codebase, below is a reduced one. Do you mind reverting the patch? > > // if the condition is not true, CHECK will terminate

[PATCH] D52888: Thread safety analysis: Handle conditional expression in getTrylockCallExpr

aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley, hokein. Herald added a subscriber: cfe-commits. We unwrap conditional expressions containing try-lock functions. Additionally we don't branch on conditionals, since that is usually not helpful. When

[PATCH] D52888: Thread safety analysis: Handle conditional expression in getTrylockCallExpr

This revision was automatically updated to reflect the committed changes. Closed by commit rL343902: Thread safety analysis: Handle conditional expression in getTrylockCallExpr (authored by aaronpuchert, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

aaronpuchert added a comment. In https://reviews.llvm.org/D52398#1257092, @rupprecht wrote: > I patched the proposed fix-forward and it seems to have things building > again. Is there an ETA on landing that? If it's going to take a bit, is there > any chance we could revert this patch until

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

aaronpuchert added a comment. It seems that `self` is an ordinary `DeclRefExpr` unlike `this`, which is a `CXXThisExpr`. Which means we'd have to make it dependent on the name whether we drop it, but `self` in C/C++ is just an ordinary variable. So I think I'll leave the `self->` part for now.

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

This revision was automatically updated to reflect the committed changes. Closed by commit rC342600: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate (authored by aaronpuchert, committed by ). Changed prior to commit:

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

aaronpuchert marked an inline comment as done. aaronpuchert added a comment. In https://reviews.llvm.org/D51901#1239759, @delesley wrote: > This looks okay to me, but I have not tested it on a large code base to see > if it breaks anything. On our code base (not as large as Google's) there

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

This revision was automatically updated to reflect the committed changes. Closed by commit rC342605: Thread Safety Analysis: warnings for attributes without arguments (authored by aaronpuchert, committed by ). Changed prior to commit: https://reviews.llvm.org/D51901?vs=165004=166205#toc

[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

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley. Herald added subscribers: cfe-commits, kristina. When people are really sure they'll get the lock they sometimes use __builtin_expect. It's also used by some assertion implementations. Asserting that

[PATCH] D52443: Thread safety analysis: Examine constructor arguments

aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley. Herald added a subscriber: cfe-commits. Instead of only examining call arguments, we also examine constructor arguments applying the same rules. That was an oppurtunity for refactoring the examination

[PATCH] D52443: Thread safety analysis: Examine constructor arguments

aaronpuchert added a comment. There is a (technical) merge conflict between this change and https://reviews.llvm.org/D52395, but that shouldn't be of any concern for the review. The issues are rather independent. (I think.) Repository: rC Clang https://reviews.llvm.org/D52443

[PATCH] D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference

aaronpuchert added a comment. While most people probably just use ordinary mutexes, hence won't be affected, those that use read/write locks need to know when to use a shared and when to use an exclusive lock. What makes things hard in C++ is that through passing by non-const reference, an

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

aaronpuchert added a comment. No problem. Thanks for reviewing! I'm terribly sorry to be bombarding the two of you with so many review requests lately, and I hope it'll soon be over. Repository: rC Clang https://reviews.llvm.org/D52398 ___

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley. Herald added a subscriber: cfe-commits. The pattern is problematic with C++ exceptions, and not as widespread as scoped locks, but it's still used by some, for example Chromium. We are a bit stricter here

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

aaronpuchert added a comment. See the bug for further discussion. I'm not sure if we want to have this, but if the pattern is used more widely it might make sense. It blows up the code a bit, although I hope that https://reviews.llvm.org/D51187 might reduce it again. Repository: rC Clang

[PATCH] D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference

aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley. Herald added a subscriber: cfe-commits. When passing by reference, we check if the reference is const-qualified and if it isn't, we demand an exclusive lock. Unlike checking const qualifiers on member

[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

[PATCH] D56967: Thread safety analysis: Improve diagnostics for double locking

aaronpuchert marked 2 inline comments as done. aaronpuchert added a comment. Thanks for the review! I'll commit this when I have commit access again, which is waiting for my employer's approval to the relicensing. Comment at: lib/Sema/AnalysisBasedWarnings.cpp:1693 +

[PATCH] D56967: Thread safety analysis: Improve diagnostics for double locking

aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley. Herald added a subscriber: cfe-commits. We use the existing diag::note_locked_here to tell the user where we saw the first locking. Repository: rC Clang https://reviews.llvm.org/D56967 Files:

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

This revision was automatically updated to reflect the committed changes. Closed by commit rL349300: Thread safety analysis: Allow scoped releasing of capabilities (authored by aaronpuchert, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM CHANGES SINCE LAST ACTION

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

aaronpuchert added a comment. > 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

[PATCH] D52141: Thread safety analysis: Run more tests with capability attributes [NFC]

aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley, grooverdan. Herald added a subscriber: cfe-commits. We run the tests for -Wthread-safety-{negative,verbose} with the new attributes as well as the old ones. Also put the macros in a header so that we don't

[PATCH] D52141: Thread safety analysis: Run more tests with capability attributes [NFC]

aaronpuchert marked an inline comment as done. aaronpuchert added a comment. Makes sense to me. Thanks for the review! Repository: rC Clang https://reviews.llvm.org/D52141 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D52141: Thread safety analysis: Run more tests with capability attributes [NFC]

This revision was automatically updated to reflect the committed changes. Closed by commit rC342418: Thread safety analysis: Run more tests with capability attributes [NFC] (authored by aaronpuchert, committed by ). Changed prior to commit: https://reviews.llvm.org/D52141?vs=165662=165835#toc

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley, lukasza. Herald added a subscriber: cfe-commits. This imitates the code for MemberExpr. I hope it is right, for I have absolutely no understanding of ObjC++. Fixes 38896. Repository: rC Clang

[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

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

aaronpuchert planned changes to this revision. aaronpuchert added a comment. Thanks to both of you for the reviews. I'll see what I can do about the arrows. My gut feeling is that using `{Member,ObjCIVarRef}Expr::isArrow` is the right way, but it's not yet obvious to me how.

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

aaronpuchert added a comment. I found something that would theoretically work: P->setArrow((isa(ME->getBase()) && Ctx && Ctx->SelfArg) ? Ctx->SelfArrow : ME->isArrow()); So if we have `this` and a context that tells us we have to replace `this` by something else, then we check

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

aaronpuchert updated this revision to Diff 166051. aaronpuchert added a comment. Detect ObjC pointer types as well as ordinary pointers. Repository: rC Clang https://reviews.llvm.org/D52200 Files: include/clang/Analysis/Analyses/ThreadSafetyCommon.h lib/Analysis/ThreadSafetyCommon.cpp

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

aaronpuchert marked 2 inline comments as done. aaronpuchert added a comment. I think it should be possible to get rid of `self->` in the warning message if we want to, after all `this->` is omitted in C++ as well. Comment at: test/SemaObjCXX/warn-thread-safety-analysis.mm:42

[PATCH] D59523: Thread Safety: also look at ObjC methods

aaronpuchert added a comment. > It's less about the regressions and more about the kind of regressions we're > testing against. But the test also verifies that no diagnostics are omitted (`// expected-no-diagnostics`), so it isn't just a "this doesn't crash" test. Which is why I think it's a

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

aaronpuchert added a comment. In D59673#1452269 , @dblaikie wrote: > Ah, fair. You could actually test the dwo_name is accurate in the .dwo file > (I added the dwo_name to the .dwo file so that multi-level dwp error messages > could be more informative)

[PATCH] D59523: Thread Safety: also look at ObjC methods

aaronpuchert added a comment. In D59523#1440263 , @aaron.ballman wrote: > In D59523#1440238 , @jfb wrote: > > > This is very concrete: this specific code used to cause a crash. This test > > has exactly this

[PATCH] D59523: Thread Safety: also look at ObjC methods

aaronpuchert accepted this revision. aaronpuchert added a comment. Alright, go ahead. I don't want this to be held up by such a minor detail. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59523/new/ https://reviews.llvm.org/D59523

[PATCH] D59455: Thread safety analysis: Add note for unlock kind mismatch

aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley. Herald added a project: clang. Herald added a subscriber: cfe-commits. Similar to D56967 , we add the existing diag::note_locked_here to tell the user where we saw the

[PATCH] D59455: Thread safety analysis: Add note for unlock kind mismatch

aaronpuchert updated this revision to Diff 190968. aaronpuchert added a comment. Factor out some common code. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59455/new/ https://reviews.llvm.org/D59455 Files: include/clang/Analysis/Analyses/ThreadSafety.h

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert updated this revision to Diff 190966. aaronpuchert added a comment. Don't suggest adding `static` if there is a non-prototype declaration. This required a minor refactoring: we let `ShouldWarnAboutMissingPrototype` return any kind of declaration it finds and check for the number of

[PATCH] D59455: Thread safety analysis: Add note for unlock kind mismatch

aaronpuchert added inline comments. Comment at: include/clang/Analysis/Analyses/ThreadSafety.h:127 + SourceLocation LocLocked, SourceLocation Loc) {} aaron.ballman wrote: > Can

[PATCH] D59455: Thread safety analysis: Add note for unlock kind mismatch

This revision was automatically updated to reflect the committed changes. Closed by commit rC356427: Thread safety analysis: Add note for unlock kind mismatch (authored by aaronpuchert, committed by ). Changed prior to commit: https://reviews.llvm.org/D59455?vs=190968=191207#toc Repository:

[PATCH] D59523: Thread Safety: also look at ObjC methods

aaronpuchert added a comment. > It should consider both because the attributes can be used on Objective-C as > well. Well, it's not really supported that well though. There are known bugs like https://bugs.llvm.org/show_bug.cgi?id=38892, and I don't really have the time to fix that. (You're

[PATCH] D59455: Thread safety analysis: Add note for unlock kind mismatch

aaronpuchert marked 4 inline comments as done. aaronpuchert added inline comments. Comment at: include/clang/Analysis/Analyses/ThreadSafety.h:127 + SourceLocation LocLocked, SourceLocation Loc) {}

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert created this revision. aaronpuchert added reviewers: bkramer, efriedma, rsmith. Herald added subscribers: cfe-commits, jdoerfert. Herald added a project: clang. I've found that most often the proper way to fix this warning is to add `static`, because if the code otherwise compiles

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert added inline comments. Comment at: test/Sema/warn-missing-prototypes.c:7 int f(int x) { return x; } // expected-warning{{no previous prototype for function 'f'}} +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static " Maybe there

[PATCH] D59523: Thread Safety: also look at ObjC methods

aaronpuchert added inline comments. Comment at: lib/Analysis/ThreadSafetyCommon.cpp:283-285 + if (isa(D) + ? (cast(D)->getCanonicalDecl() == Canonical) + : (cast(D)->getCanonicalDecl() == Canonical)) { aaron.ballman wrote: > Also

  1   2   3   4   5   6   7   >