Thread safety analysis: Handle compound assignment and ->* overloads

[aaronpuchert]

Summary

Like regular assignment, compound assignment operators can be assumed to
write to their left-hand side operand. So we strengthen the requirements
there. (Previously only the default read access had been required.)

Just like operator->, operator->* can also be assumed to dereference the
left-hand side argument, so we require read access to the pointee. This
will generate new warnings if the left-hand side has a pt_guarded_by
attribute. This overload is rarely used, but it was trivial to add, so
why not. (Supporting the builtin operator requires changes to the TIL.)

[aaronpuchert]

@rupprecht, do you still do integration at Google? This patch might be interesting to try out, since it strengthens some warnings.

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

85–86

In case you're wondering, making this a member function prevents instantiating the class with non-class types:

error: member pointer refers into non-class type 'int'
  U& operator->*(U T::*p) const { return ptr_->*p; }
                      ^
note: in instantiation of template class 'SmartPtr<int>' requested here
SmartPtr<int> p;
              ^

[aaron.ballman]

clang/lib/Analysis/ThreadSafety.cpp

1994

If we're going to be handling these, should we also be handling overloads of ++ and --?

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

4344

We should probably add test coverage for all the newly supported operators.

[aaronpuchert]

clang/lib/Analysis/ThreadSafety.cpp

1994

Yes, in some sense these are also compound assignment operators despite their looks. I'll add them as well.

Sadly streams use << and >> instead of <<= and >>=, and we can't really assume the former to write.

[aaronpuchert]

Also consider ++ and -- as writing, test all operators.

[aaronpuchert]

Use 1 instead of 0 just to be sure. We don't want to trigger warnings about division by zero.

[aaron.ballman]

LGTM! You should probably add a release note for this change so users know about it. (Do we need to update any documentation from this?)

[aaronpuchert]

In D124966#3497305, @aaron.ballman wrote:

LGTM! You should probably add a release note for this change so users know about it.

Good point, I'll add a note about compound assignment and increment/decrement. I'll leave out operator->* though if you don't mind because it's rarely overloaded.

Do we need to update any documentation from this?

I don't think so, we don't really specify what we consider as write and what not. It's more of a heuristic anyway, unless at some point we feel comfortable enough to do D52395.

[aaronpuchert]

Add a release note.

[aaronpuchert]

clang/docs/ReleaseNotes.rst

173 ↗

(On Diff #427750)

Or maybe I should add it here? Not sure why there is a blank line.

[aaron.ballman]

In D124966#3497781, @aaronpuchert wrote:

In D124966#3497305, @aaron.ballman wrote:

Do we need to update any documentation from this?

I don't think so, we don't really specify what we consider as write and what not. It's more of a heuristic anyway, unless at some point we feel comfortable enough to do D52395.

SGTM, thanks for verifying! Continues to LGTM.

clang/docs/ReleaseNotes.rst

173 ↗

(On Diff #427750)

Either way is fine -- the blank line is an accident, but it'll go away at some point.

clang/lib/Analysis/ThreadSafety.cpp

if (ME && MD) {
         checkAccess(CE->getImplicitObjectArgument(), AK_Read);
       }
     }
 
     examineArguments(CE->getDirectCallee(), CE->arg_begin(), CE->arg_end());
   } else if (const auto *OE = dyn_cast<CXXOperatorCallExpr>(Exp)) {
     auto OEop = OE->getOperator();
     switch (OEop) {
-      case OO_Equal: {
+      case OO_Equal:
+      case OO_PlusEqual:
+      case OO_MinusEqual:
+      case OO_StarEqual:
+      case OO_SlashEqual:
+      case OO_PercentEqual:
+      case OO_CaretEqual:
+      case OO_AmpEqual:
+      case OO_PipeEqual:
+      case OO_LessLessEqual:
+      case OO_GreaterGreaterEqual: {
         const Expr *Target = OE->getArg(0);
         const Expr *Source = OE->getArg(1);
         checkAccess(Target, AK_Written);
         checkAccess(Source, AK_Read);
         break;
       }
       case OO_Star:
+      case OO_ArrowStar:
       case OO_Arrow:
       case OO_Subscript:
         if (!(OEop == OO_Star && OE->getNumArgs() > 1)) {
           // Grrr.  operator* can be multiplication...
           checkPtAccess(OE->getArg(0), AK_Read);
         }
         LLVM_FALLTHROUGH;
       default: {

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

public:
   T* operator->() const { return ptr_; }
   T& operator*()  const { return *ptr_; }
   T& operator[](int i) const { return ptr_[i]; }
 
 private:
   T* ptr_;
 };
 
+template<typename T, typename U>
+U& operator->*(const SmartPtr<T>& ptr, U T::*p) { return ptr->*p; }
+
 
 // For testing destructor calls and cleanup.
 class MyString {
 public:
   MyString(const char* s);
   ~MyString();
 };
 
public:
   int  getValue() const { return dat; }
   void setValue(int i)  { dat = i; }
 
   int  operator[](int i) const { return dat; }
   int& operator[](int i)       { return dat; }
 
   void operator()() { }
 
+  Data& operator+=(int);
+
 private:
   int dat;
 };
 
 
 class DataCell {
 public:
   DataCell(const Data& d) : dat(d) { }
void test2() {
     *datap1_ = data_;     // expected-warning {{reading variable 'datap1_' requires holding mutex 'mu_'}} \
                           // expected-warning {{reading variable 'data_' requires holding mutex 'mu_'}}
     *datap2_ = data_;     // expected-warning {{writing the value pointed to by 'datap2_' requires holding mutex 'mu_' exclusively}} \
                           // expected-warning {{reading variable 'data_' requires holding mutex 'mu_'}}
     data_ = *datap1_;     // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} \
                           // expected-warning {{reading variable 'datap1_' requires holding mutex 'mu_'}}
     data_ = *datap2_;     // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}} \
                           // expected-warning {{reading the value pointed to by 'datap2_' requires holding mutex 'mu_'}}
+    data_ += 0;           // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
 
     data_[0] = 0;         // expected-warning {{reading variable 'data_' requires holding mutex 'mu_'}}
     (*datap2_)[0] = 0;    // expected-warning {{reading the value pointed to by 'datap2_' requires holding mutex 'mu_'}}
 
     data_();              // expected-warning {{reading variable 'data_' requires holding mutex 'mu_'}}
   }
 
   // const operator tests

 
 
 class SmartPtr_PtGuardedBy_Test {
   Mutex mu1;
   Mutex mu2;
   SmartPtr<int>  sp GUARDED_BY(mu1) PT_GUARDED_BY(mu2);
   SmartPtr<Cell> sq GUARDED_BY(mu1) PT_GUARDED_BY(mu2);
 
+  static constexpr int Cell::*pa = &Cell::a;
+
   void test1() {
     mu1.ReaderLock();
     mu2.Lock();
 
     sp.get();
     if (*sp == 0) doSomething();
     *sp = 0;
     sq->a = 0;
+    sq->*pa = 0;
 
     if (sp[0] == 0) doSomething();
     sp[0] = 0;
 
     mu2.Unlock();
     mu1.Unlock();
   }
 
   void test2() {
     mu2.Lock();
 
     sp.get();                      // expected-warning {{reading variable 'sp' requires holding mutex 'mu1'}}
     if (*sp == 0) doSomething();   // expected-warning {{reading variable 'sp' requires holding mutex 'mu1'}}
     *sp = 0;                       // expected-warning {{reading variable 'sp' requires holding mutex 'mu1'}}
     sq->a = 0;                     // expected-warning {{reading variable 'sq' requires holding mutex 'mu1'}}
+    sq->*pa = 0;                   // expected-warning {{reading variable 'sq' requires holding mutex 'mu1'}}
 
     if (sp[0] == 0) doSomething();   // expected-warning {{reading variable 'sp' requires holding mutex 'mu1'}}
     sp[0] = 0;                       // expected-warning {{reading variable 'sp' requires holding mutex 'mu1'}}
     if (sq[0].a == 0) doSomething(); // expected-warning {{reading variable 'sq' requires holding mutex 'mu1'}}
     sq[0].a = 0;                     // expected-warning {{reading variable 'sq' requires holding mutex 'mu1'}}
 
     mu2.Unlock();
   }
 
   void test3() {
     mu1.Lock();
 
     sp.get();
     if (*sp == 0) doSomething();   // expected-warning {{reading the value pointed to by 'sp' requires holding mutex 'mu2'}}
     *sp = 0;                       // expected-warning {{reading the value pointed to by 'sp' requires holding mutex 'mu2'}}
     sq->a = 0;                     // expected-warning {{reading the value pointed to by 'sq' requires holding mutex 'mu2'}}
+    sq->*pa = 0;                   // expected-warning {{reading the value pointed to by 'sq' requires holding mutex 'mu2'}}
 
     if (sp[0] == 0) doSomething();   // expected-warning {{reading the value pointed to by 'sp' requires holding mutex 'mu2'}}
     sp[0] = 0;                       // expected-warning {{reading the value pointed to by 'sp' requires holding mutex 'mu2'}}
     if (sq[0].a == 0) doSomething(); // expected-warning {{reading the value pointed to by 'sq' requires holding mutex 'mu2'}}
     sq[0].a = 0;                     // expected-warning {{reading the value pointed to by 'sq' requires holding mutex 'mu2'}}
 
     mu1.Unlock();
   }