[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 http://

[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. So

[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 closer

[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 the

[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 long

[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 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

[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: test/SemaCXX/warn-thread-safety-analysis.cpp

[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: test/SemaCXX/wa

[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] 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 debata

[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 destructor

[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 d

[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] 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 lib/Sema/SemaDe

[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] 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(&mu); while (b) { scope.Unlock(); // releasing mutex 'scope' that was not held scope.Lock(); // acquirin

[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] 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 cfe-commi

[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 eleg

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

[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: test/SemaCXX/warn

[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] 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: test/SemaCXX/wa

[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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-co

[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. 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 n

[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 un

[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 mer

[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 https://reviews.llvm.

[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: test/SemaCXX/warn-t

[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: include

[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

This revision was automatically updated to reflect the committed changes. Closed by commit rL352549: Thread safety analysis: Improve diagnostics for double locking (authored by aaronpuchert, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.o

[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 capabil

[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 and

[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 shoul

[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 locki

[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 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 lib/A

[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 yo

[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&id=191207#toc Repository:

[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] 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 fr

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

aaronpuchert added a reviewer: aaronpuchert. aaronpuchert added inline comments. Comment at: lib/Analysis/ThreadSafetyCommon.cpp:282-285 + const auto *Canonical = Ctx->AttrDecl->getCanonicalDecl(); + if (isa(D) + ? (cast(D)->getCanonicalDecl() == Canonical)

[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 so

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

aaronpuchert created this revision. aaronpuchert added reviewers: dblaikie, echristo. Herald added subscribers: cfe-commits, jdoerfert, aprantl. Herald added a project: clang. With Split DWARF the resulting object file (then called skeleton CU) contains the file name of another ("DWO") file with t

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

aaronpuchert added a comment. In D59673#1438716 , @dblaikie wrote: > Use llvm-dwarfdump rather than llvm-objdump to dump the contents of the > debug_info section and test the dwo_name there (rather than dumping hex) I didn't know about llvm-dwarfdump, I

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

aaronpuchert updated this revision to Diff 191813. aaronpuchert added a comment. Use llvm-dwarfdump to inspect debug info, remove unneeded flags. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59673/new/ https://reviews.llvm.org/D59673 Files: include/clang/Basi

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

aaronpuchert added inline comments. Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1 +// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class %s + aaron.ballman wrote: > jfb wrote: > > aaron.ballman wrote: > > > jfb wrote: >

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

aaronpuchert added a comment. Creating a new test makes sense to me if it tests things across components. We have such tests for modules, PCH, and templates. There are also separate tests for the attribute parsing, which doesn't work terribly well in ObjC either. I would agree to making a new t

[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 n

[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] 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 purpo

[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] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

aaronpuchert added a comment. Ok, here is an idea. We introduce `-split-dwarf-output` in Clang instead of `-fsplit-dwarf-dwo-name-attr`. If given, it overrides the output file name for the Split DWARF file, which we otherwise take from `-split-dwarf-file`. The option is obviously incompatible w

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

aaronpuchert planned changes to this revision. aaronpuchert added a comment. In D59673#1461983 , @dblaikie wrote: > Sure, I think the naming's a bit weird (but hard to come up with good names > for any of this) Agreed, `-split-dwarf-output` is pretty cl

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

aaronpuchert updated this revision to Diff 196149. aaronpuchert added a comment. Introduce -split-dwarf-output instead to mirror the behavior of llc, which already allows to set the attribute separately from the output file name. Repository: rC Clang CHANGES SINCE LAST ACTION https://revie

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

aaronpuchert added a subscriber: pcc. aaronpuchert added inline comments. Comment at: lib/CodeGen/BackendUtil.cpp:1345 Conf.RemarksPasses = CGOpts.OptRecordPasses; - Conf.DwoPath = CGOpts.SplitDwarfFile; + Conf.DwoPath = CGOpts.SplitDwarfOutput; switch (Action) {

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

aaronpuchert updated this revision to Diff 196156. aaronpuchert added a comment. Adapt one more test case. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59673/new/ https://reviews.llvm.org/D59673 Files: include/clang/Basic/CodeGenOptions.def include/clang/Ba

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

aaronpuchert edited reviewers, added: riccibruno; removed: efriedma. aaronpuchert removed a subscriber: riccibruno. aaronpuchert added a comment. Ping? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59402/new/ https://reviews.llvm.org/D59402 ___

[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 h

[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 http://lists.llvm.

[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&id=165835#to

[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 https://r

[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 `Ctx->SelfArro

[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] 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: https://reviews.llvm.org/D52200?vs=166051&id=166197#

[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 was

[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&id=166205#toc Rep

[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 mayb

[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 functi

[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.

[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 try-loc

[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 proce

[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 ___ cfe-c

[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 obje

[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 a

[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 ht

[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) { aaron

[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 th

[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) EXCLUSIVE_UNLOCK_FUNCTIO

[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 to

[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] 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 https://reviews.llv

[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 th

[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 joining

[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 h

[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, wh

  1   2   3   4   5   6   7   >