aaron.ballman added a comment.
In https://reviews.llvm.org/D52888#1255862, @aaronpuchert wrote:
> Additional changes (including some 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();
> }
>
>
> Worth the effort, or is that too exotic?
`foo16()` looks like code I could plausibly see in the wild. Consider the case
where the mutex is a pointer and you want to test it for null before calling
`TryLock()` on it (`M ? M->TryLock() : false`). However, I don't have a good
feeling for how much work it would be to support that case.
================
Comment at: test/SemaCXX/warn-thread-safety-analysis.cpp:1879-1880
+ void foo13() {
+ if (mu.TryLock() ? 1 : 0)
+ mu.Unlock();
+ }
----------------
aaronpuchert wrote:
> aaron.ballman wrote:
> > aaronpuchert wrote:
> > > aaron.ballman wrote:
> > > > Can you add a test that shows we get it right even if the user does
> > > > something less than practical, like:
> > > > ```
> > > > if (mu.TryLock() ? mu.TryLock() : false); // Warn about double lock
> > > > if (mu.TryLock() ? mu.Unlock(), 1 : 0)
> > > > mu.Unlock(); // Warn about unlocking unheld lock
> > > > if (mu.TryLock() ? 1 : mu.Unlock(), 0)
> > > > mu.Unlock(); // Warn about unlocking an unheld lock
> > > > if (mu.TryLock() ? (true ? mu.TryLock() : false) : false); // Warn
> > > > about double lock
> > > > ```
> > > I'm afraid these don't work as expected if we don't branch on `?:` as
> > > before. Basically we treat this as if both conditions where evaluated
> > > unconditionally.
> > >
> > > On calling a try-lock function, no lock is immediately acquired. Only
> > > when we branch on the result of that try-lock call is the mutex added to
> > > the capability set on the appropriate branch. Now if we branch on `?:`
> > > (the situation before this change), we are going to complain about
> > > conditionally held locks on the join point in @hokein's use case, and if
> > > we don't branch (the behavior after this change), we are not treating
> > > your examples correctly. I'm not sure we can treat both as intended.
> > >
> > > I'm slightly leaning towards not branching: the C++ standard says that
> > > only the [used expression is actually
> > > evaluated](https://en.cppreference.com/w/cpp/language/operator_other#Conditional_operator),
> > > but I think it's not considered good practice to have side effects in a
> > > ternary operator. (I can't actually find it anywhere besides a discussion
> > > on [Hacker News](https://news.ycombinator.com/item?id=14810994).
> > > Personally I would prefer to use an if-statement if one of the
> > > expressions has side-effects.)
> > >
> > > Tell me what you think.
> > I agree that we don't want to encourage complex code in ternary operators,
> > but that complexity does come up often as a result of macro expansion. It
> > feels like there isn't a good answer here because either approach will have
> > unexpected results.
> >
> > I'll let @delesley be the tie-breaker on whether we follow ternary branches
> > or not, but if we decide to accept this patch and not follow branches, I
> > think we should capture those test cases as expected false negatives with a
> > comment about why they happen and why it's expected behavior.
> >
> > Related, does this patch then impact the behavior of code like:
> > ```
> > void f(Mutex &M) {
> > M.Lock();
> > }
> >
> > void g() {
> > Mutex mu;
> > (void)(true ? f(mu) : (void)0);
> > mu.Unlock(); // Ok
> > }
> > ```
> >
> No, this is not affected. There will be a warning independently of this
> patch. We notice that the lock is held on only one branch after joining them.
>
> I think my wording was a bit unfortunate: we still follow the ordinary CFG.
> On try-lock the lock is not acquired immediately, only when we branch on the
> return value. So we weaken the usual requirement of "no conditional locks"
> for the period between the try-lock call and the branch. That was the
> situation before this patch already.
>
> With this patch `?:` branches on the return value of a try-lock are no longer
> considered as possible acquisition points. We postpone the acquisition even
> further to another (more obvious) branch like an `if` statement that uses the
> try-lock return result. To that end we inspect possible `ConditionalExpr`s so
> that we can derive which branch holds the lock even when these are used.
Ah, okay, thank you for the further explanation. That makes me a bit more
comfortable with the proposed approach.
Repository:
rC Clang
https://reviews.llvm.org/D52888
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
