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

[Delesley Hutchins via Phabricator via cfe-commits]

delesley added a comment.

I have mixed feelings about this patch.  When you pass an object to a function, either by pointer or by reference, no actual load from memory has yet occurred.  Thus, there is a real risk of false positives; what you are saying is that the function *might* read or write from protected memory, not that it actually will.  In fact, the false positive rate is high enough that this particular warning is sequestered under -Wthread-safety-reference, and is not used with warnings-as-errors at Google.

In theory, the correct way to deal with references is to make GUARDED_BY an attribute of the type, rather than an attribute of the data member; that way taking the address of a member, or passing it by reference, wouldn't cast away the annotation.  But that would require a properly parametric dependent type system for C++, which is pretty much impossible.  So you're left with two bad options: either issue a warning eagerly, and deal with false positives, or suppress the warning, and deal with false negatives.  At Google, false positives usually break the build, which has forced me to prefer false negatives in most cases; my strategy has been to gradually tighten the analysis bit by bit.  Thankfully, that's less of a concern here.

Adding to the difficulty is the fact that the correct use of "const" in real-world C++ code is spotty at best.  There is LOTS of code that arguably should be using const, but doesn't for various reasons.  E.g. if program A calls library B, and library B forgot to provide const-qualified versions of a few methods, than A has to make a choice: either drop the const qualifier, or insert ugly const-casts, neither of which is pleasant.  In other-words, non-constness has a tendency to propagate through the code base, and you can't depend on "const" being accurate.

I have two recommendations.  First think the default behavior should be to require only a read-only lock, as it currently does.  That's a compromise between the "false-positive" and "false-negative" camps.  It catches a lot of errors, because you have to hold the lock in some way, but doesn't require accurate const-ness.  For people who want more accuracy, there should be a different variant of the warning -- maybe -Wthread-safety-reference-precise?  Between beta/precise/reference etc. there are a now a lot of analysis options, and I would love to consolidate them in some fashion.  However, different code bases have different needs, and I think "one size fits all" is not really appropriate.

Second, there needs to be a thread-safety-variant of "const_cast" -- i.e. a way to pass a reference to a function without triggering the warning.  That may already be possible via clever use of our current annotations, or you may have to fiddle with something, but it needs to appear in the test cases.


Repository:
  rC Clang

https://reviews.llvm.org/D52395