Thread safety analysis: Require exclusive lock for passing by non-const reference

[aaronpuchert]

Summary

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 functions, there are probably not many false
positives here: if a function takes a non-const reference, it will
in almost all cases modify the object that we passed it.

[aaron.ballman]

Unlike checking const qualifiers on member functions, there are probably not many false positives here: if a function takes a non-const reference, it will in almost all cases modify the object that we passed it.

I'm not certain I agree with the predicate here. We can make that inference when there *is* a const qualifier on the parameter, but I don't I think we can make any assumptions about whether it will or won't modify the object passed in in the absence of a const qualifier. This has come up in the past for things like C(C&) being a valid copy constructor despite the parameter being non-const. We might need some data to back this assertion up.

lib/Analysis/ThreadSafety.cpp

2023

This isn't specific to your changes, but this gives me the impression we don't distinguish between rvalue references and lvalue references, but that may be something of interest in here.

[aaronpuchert]

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 object can actually be modified although it looks like it's only passed as a parameter. That makes it really hard to decide whether a shared lock is sufficient just by reading the code, which was a major pain point in our project. We had to do very careful reviews to make sure we had the right kind of lock in every function. I'd like to offload some of that work to the compiler.

That's the main reason for this change: it's pretty easy to find out where a variable is used, but it's hard to say which uses are reads and which are writes in modern C++, if it weren't for const.

In D52395#1244021, @aaron.ballman wrote:

Unlike checking const qualifiers on member functions, there are probably not many false positives here: if a function takes a non-const reference, it will in almost all cases modify the object that we passed it.

I'm not certain I agree with the predicate here. We can make that inference when there *is* a const qualifier on the parameter, but I don't I think we can make any assumptions about whether it will or won't modify the object passed in in the absence of a const qualifier. This has come up in the past for things like C(C&) being a valid copy constructor despite the parameter being non-const.

For const qualifiers on member functions, the concern is that I might have a shared lock on a data structure like a std::vector and then I read stuff from that vector. But unless I'm reading the vector from a const member function, or through a const reference, I'm going to use the non-const operator[]. That operator doesn't actually modify anything, but it returns a reference that allows me to modify stuff. However, I'm not doing any of that, so why should there be a warning. I think that's a valid concern.

Effectively we have two overloads that are both not actually modifying anything, but the non-const variant returns a reference that does allow us to modify something. My thinking was that this pattern is not so common with function parameters. The function(s) will return a reference to a part of an object, and such references will typically come from member functions for which we don't enforce this. So even if I use a non-modifying <algorithm> like std::find, there'll be no warning because the iterators come from container members begin and end. (By the way, that would be easy to fix by using cbegin and cend.)

The way I see it, there are basically three cases of false positives:

• The function doesn't actually modify the object, but takes it as non-const reference nevertheless. That can be easily fixed though and I think it's a fix that doesn't hurt.
• The function modifies the object sometimes, but here the user is very certain that it won't. That might be indication of a poor design, in any event it's very fragile. As a developer, I would feel very anxious about this, especially in a larger code base.
• We have two overloads of a function, both don't modify the object, but the non-const variant returns a non-const reference to some part of the object. This isn't backed up by data, but I think this doesn't happen nearly as often as with member functions. There are several fixes available as well: one could make sure that we only have a const reference there, or make the function a const member, or if all else fails, by taking a const reference to the object we don't want to modify and then call the function with that as argument. That's an additional line of code like const auto &cref = obj; which I think will rarely be needed.

So basically my argument is that this can catch really nasty bugs, and false positives should be rare. If they occur, they can be fixed by making the code more const-correct. Such fixes should be easy to sell, because everybody loves const-correctness, and it doesn't require any annotations.

We might need some data to back this assertion up.

Makes sense to me, but I don't have access to the largest code base in terms of thread safety analysis. Maybe @delesley can help here? Or I need to apply for a job at Google. On our own code, we're still in the very early stages of annotating, so I can't provide reliable data from there. I've seen thread safety analysis in flutter and Chromium, but both don't seem to use read/write locks.

lib/Analysis/ThreadSafety.cpp

2023

You mean & vs &&? I don't that makes a difference here, the important property is whether it's const. So const& is a read, and & and && are writes, and const&& doesn't really make sense. (It's effectively the same as const& for all I know.)

But there is one error here: I definitely have to look at the canonical type, i.e. it should be &*Qt.getCanonicalType() instead of &*Qt.

[delesley]

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.

[delesley]

With respect to data, I really think these patches should be tested against Google's code base, because otherwise you're going to start seeing angry rollbacks. However, I don't work on the C++ team any more, and I don't have time to do it. When I was actively developing the analysis, I spent about 90% of my time just running tests and fixing the code base. Each incremental improvement in the analysis itself was a hard upward slog. If you're going to be adding lots of improvements, we need to have someone at Google running point.

[aaron.ballman]

In D52395#1248608, @delesley wrote:

With respect to data, I really think these patches should be tested against Google's code base, because otherwise you're going to start seeing angry rollbacks. However, I don't work on the C++ team any more, and I don't have time to do it. When I was actively developing the analysis, I spent about 90% of my time just running tests and fixing the code base. Each incremental improvement in the analysis itself was a hard upward slog. If you're going to be adding lots of improvements, we need to have someone at Google running point.

Do you have a recommendation for who this point person could be? What do we do if we cannot find such a person? (I assume we won't halt progress on thread safety analysis and that propose->review->accept->commit->revert is not an acceptable workflow.)

[aaronpuchert]

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 the analysis on our own messy code base. Maybe I'm going to see the false positives right there.

However, as messy as our code may be, it might be messy in different ways than Google's code. So I might not see the same issues. Then on the other hand, there might be (probably smaller) code bases which don't have a problem with stricter rules. So I think additional flags are probably not the worst idea, but we need to keep their number small.

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.

There is such a mechanism in the test suite. It uses that we don't check the arguments for functions annotated with no_thread_safety_analysis, introduced by rC246806:

// Takes a reference to a guarded data member, and returns an unguarded
// reference.
template <class T>
inline const T& ts_unchecked_read(const T& v) NO_THREAD_SAFETY_ANALYSIS {
  return v;
}

template <class T>
inline T& ts_unchecked_read(T& v) NO_THREAD_SAFETY_ANALYSIS {
  return v;
}

[delesley]

For future patches, please add Richard Trieu ([e-mail]) as a subscriber. I will continue to try and do code reviews, but Richard is on the team that actually rolls out new compiler changes. Thanks!

BTW, the issue is not just that changes may introduce false positives. Even if it's a true positive, you risk breaking the build.

[aaronpuchert]

Rebase on top of 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.

[aaronpuchert]

I don't actually want to change anything, but remove this from your review queues until I have an idea how often the warning fires and how many false positives we have.

include/clang/Basic/DiagnosticSemaKinds.td

 
 // Thread safety warnings negative capabilities
 def warn_acquire_requires_negative_cap : Warning<
   "acquiring %0 '%1' requires negative capability '%2'">,
   InGroup<ThreadSafetyNegative>, DefaultIgnore;
 
 // Thread safety warnings on pass by reference
 def warn_guarded_pass_by_reference : Warning<
-  "passing variable %1 by reference requires holding %0 "
-  "%select{'%2'|'%2' exclusively}3">,
+  "passing variable %1 by %select{reference|non-const reference}3 requires "
+  "holding %0 %select{'%2'|'%2' exclusively}3">,
   InGroup<ThreadSafetyReference>, DefaultIgnore;
 def warn_pt_guarded_pass_by_reference : Warning<
-  "passing the value that %1 points to by reference requires holding %0 "
-  "%select{'%2'|'%2' exclusively}3">,
+  "passing the value that %1 points to by %select{reference|non-const "
+  "reference}3 requires holding %0 %select{'%2'|'%2' exclusively}3">,
   InGroup<ThreadSafetyReference>, DefaultIgnore;
 
 // Imprecise thread safety warnings
 def warn_variable_requires_lock : Warning<
   "%select{reading|writing}3 variable %1 requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
 def warn_var_deref_requires_lock : Warning<

lib/Analysis/ThreadSafety.cpp

if (const FunctionDecl *FD = Exp->getDirectCallee()) {
         }
         // Ignore default arguments
         unsigned n = (Fn < Cn) ? Fn : Cn;
 
         for (; i < n; ++i) {
           const ParmVarDecl *Pvd = FD->getParamDecl(i);
           const Expr *Arg = Exp->getArg(i + Skip);
           QualType Qt = Pvd->getType();
-          if (Qt->isReferenceType())
+          if (const auto* RT = dyn_cast<const ReferenceType>(&*Qt)) {
+            if (RT->getPointeeType().isConstQualified())
               checkAccess(Arg, AK_Read, POK_PassByRef);
+            else
+              checkAccess(Arg, AK_Written, POK_PassByRef);
+          }
         }
       }
     }
   }
 
   auto *D = dyn_cast_or_null<NamedDecl>(Exp->getCalleeDecl());
   if(!D || !D->hasAttrs())
     return;

test/SemaCXX/warn-thread-safety-analysis.cpp

public:
   static void smwrite2(int a, Foo& f);
   static void smread1(const Foo& f);
   static void smread2(int a, const Foo& f);
 
   void operator<<(const Foo& f);
 
   void test1() {
     copy(foo);             // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}}
-    write1(foo);           // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
-    write2(10, foo);       // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+    write1(foo);           // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+    write2(10, foo);       // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
     read1(foo);            // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
     read2(10, foo);        // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
-    destroy(mymove(foo));  // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+    destroy(mymove(foo));  // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
 
-    mwrite1(foo);           // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
-    mwrite2(10, foo);       // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+    mwrite1(foo);           // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+    mwrite2(10, foo);       // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
     mread1(foo);            // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
     mread2(10, foo);        // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 
-    smwrite1(foo);           // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
-    smwrite2(10, foo);       // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+    smwrite1(foo);           // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+    smwrite2(10, foo);       // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
     smread1(foo);            // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
     smread2(10, foo);        // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 
     foo + foo2;              // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}} \
                              // expected-warning {{passing variable 'foo2' by reference requires holding mutex 'mu'}}
     foo / foo2;              // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}} \
                              // expected-warning {{passing variable 'foo2' by reference requires holding mutex 'mu'}}
     foo * foo2;              // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}} \
                              // expected-warning {{passing variable 'foo2' by reference requires holding mutex 'mu'}}
     foo[foo2];               // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}} \
                              // expected-warning {{passing variable 'foo2' by reference requires holding mutex 'mu'}}
     (*this) << foo;          // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 
     copy(*foop);             // expected-warning {{reading the value pointed to by 'foop' requires holding mutex 'mu'}}
-    write1(*foop);           // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}}
-    write2(10, *foop);       // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}}
+    write1(*foop);           // expected-warning {{passing the value that 'foop' points to by non-const reference requires holding mutex 'mu' exclusively}}
+    write2(10, *foop);       // expected-warning {{passing the value that 'foop' points to by non-const reference requires holding mutex 'mu' exclusively}}
     read1(*foop);            // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}}
     read2(10, *foop);        // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}}
-    destroy(mymove(*foop));  // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}}
+    destroy(mymove(*foop));  // expected-warning {{passing the value that 'foop' points to by non-const reference requires holding mutex 'mu' exclusively}}
 
+    // TODO -- sometimes this should warn about writing.
     copy(*foosp);             // expected-warning {{reading the value pointed to by 'foosp' requires holding mutex 'mu'}}
     write1(*foosp);           // expected-warning {{reading the value pointed to by 'foosp' requires holding mutex 'mu'}}
     write2(10, *foosp);       // expected-warning {{reading the value pointed to by 'foosp' requires holding mutex 'mu'}}
     read1(*foosp);            // expected-warning {{reading the value pointed to by 'foosp' requires holding mutex 'mu'}}
     read2(10, *foosp);        // expected-warning {{reading the value pointed to by 'foosp' requires holding mutex 'mu'}}
     destroy(mymove(*foosp));  // expected-warning {{reading the value pointed to by 'foosp' requires holding mutex 'mu'}}
 
     // TODO -- these require better smart pointer handling.
     copy(*foosp.get());
     write1(*foosp.get());
     write2(10, *foosp.get());
     read1(*foosp.get());
     read2(10, *foosp.get());
     destroy(mymove(*foosp.get()));
   }
+
+  void test2() {
+    mu.ReaderLock();
+
+    copy(foo);
+    write1(foo);              // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+    write2(10, foo);          // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+    read1(foo);
+    read2(10, foo);
+    destroy(mymove(foo));     // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+
+    mwrite1(foo);             // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+    mwrite2(10, foo);         // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+    mread1(foo);
+    mread2(10, foo);
+
+    smwrite1(foo);            // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+    smwrite2(10, foo);        // expected-warning {{passing variable 'foo' by non-const reference requires holding mutex 'mu' exclusively}}
+    smread1(foo);
+    smread2(10, foo);
+
+    foo + foo2;
+    foo / foo2;
+    foo * foo2;
+    foo[foo2];
+    (*this) << foo;
+
+    copy(*foop);
+    write1(*foop);            // expected-warning {{passing the value that 'foop' points to by non-const reference requires holding mutex 'mu' exclusively}}
+    write2(10, *foop);        // expected-warning {{passing the value that 'foop' points to by non-const reference requires holding mutex 'mu' exclusively}}
+    read1(*foop);
+    read2(10, *foop);
+    destroy(mymove(*foop));   // expected-warning {{passing the value that 'foop' points to by non-const reference requires holding mutex 'mu' exclusively}}
+
+    // TODO -- these require better smart pointer handling.
+    copy(*foosp);
+    write1(*foosp);
+    write2(10, *foosp);
+    read1(*foosp);
+    read2(10, *foosp);
+    destroy(mymove(*foosp));
+
+    copy(*foosp.get());
+    write1(*foosp.get());
+    write2(10, *foosp.get());
+    read1(*foosp.get());
+    read2(10, *foosp.get());
+    destroy(mymove(*foosp.get()));
+
+    mu.ReaderUnlock();
+  }
 };
 
 
 }  // end namespace PassByRefTest
 
 
 namespace AcquiredBeforeAfterText {