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

1993

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

1993

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

194–204

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

194–204

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

clang/docs/ReleaseNotes.rst

- Clang now appropriately issues an error in C when a definition of a function
   function with a prototype. e.g., ``void f(int); void f() {}`` is now properly
   diagnosed.
 - The ``-Wimplicit-function-declaration`` warning diagnostic now defaults to
   an error in C99 and later. Prior to C2x, it may be downgraded to a warning
   with ``-Wno-error=implicit-function-declaration``, or disabled entirely with
   ``-Wno-implicit-function-declaration``. As of C2x, support for implicit
   function declarations has been removed, and the warning options will have no
   effect.
 - The ``-Wimplicit-int`` warning diagnostic now defaults to an error in C99 and
   later. Prior to C2x, it may be downgraded to a warning with
   ``-Wno-error=implicit-int``, or disabled entirely with ``-Wno-implicit-int``.
   As of C2x, support for implicit int has been removed, and the warning options
   will have no effect. Specifying ``-Wimplicit-int`` in C89 mode will now issue
   warnings instead of being a noop.
 - No longer issue a "declaration specifiers missing, defaulting to int"
   diagnostic in C89 mode because it is not an extension in C89, it was valid
   code. The diagnostic has been removed entirely as it did not have a
   diagnostic group to disable it, but it can be covered wholly by
   ``-Wimplicit-int``.
 - ``-Wmisexpect`` warns when the branch weights collected during profiling
   conflict with those added by ``llvm.expect``.
+- ``-Wthread-safety-analysis`` now considers overloaded compound assignment and
+  increment/decrement operators as writing to their first argument, thus
+  requiring an exclusive lock if the argument is guarded.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 - Improve __builtin_dump_struct:
 
   - Support bitfields in struct and union.
   - Improve the dump format, dump both bitwidth(if its a bitfield) and field

clang/lib/Analysis/ThreadSafety.cpp

if (ME && MD) {
       } else {
         // Should perhaps be AK_Written if !MD->isConst().
         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();
+    OverloadedOperatorKind OEop = OE->getOperator();
     switch (OEop) {
-      case OO_Equal: {
-        const Expr *Target = OE->getArg(0);
-        const Expr *Source = OE->getArg(1);
-        checkAccess(Target, AK_Written);
-        checkAccess(Source, AK_Read);
+      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:
+        checkAccess(OE->getArg(1), AK_Read);
+        LLVM_FALLTHROUGH;
+      case OO_PlusPlus:
+      case OO_MinusMinus:
+        checkAccess(OE->getArg(0), AK_Written);
         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);
+  Data& operator-=(int);
+  Data& operator*=(int);
+  Data& operator/=(int);
+  Data& operator%=(int);
+  Data& operator^=(int);
+  Data& operator&=(int);
+  Data& operator|=(int);
+  Data& operator<<=(int);
+  Data& operator>>=(int);
+  Data& operator++();
+  Data& operator++(int);
+  Data& 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_ += 1;           // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
+    data_ -= 1;           // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
+    data_ *= 1;           // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
+    data_ /= 1;           // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
+    data_ %= 1;           // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
+    data_ ^= 1;           // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
+    data_ &= 1;           // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
+    data_ |= 1;           // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
+    data_ <<= 1;          // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
+    data_ >>= 1;          // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
+    ++data_;              // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
+    data_++;              // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
+    --data_;              // expected-warning {{writing variable 'data_' requires holding mutex 'mu_' exclusively}}
+    data_--;              // 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();
   }