One problem that attempts to add the annotation to //base/synchronization/lock.h run into is a problematic interaction with base::Optional (where lock safety analysis incorrectly thinks that AutoLock's destructor will be run by base::OptionalBase<AutoLock>::FreeIfNeeded):

Summary of attempted changes:

    -class BASE_EXPORT Lock {
    +class BASE_EXPORT LOCKABLE Lock {

    ... avoiding more Lock annotations for now and focusing on AutoLock first ...

    -class AutoLock {
    +class SCOPED_LOCKABLE AutoLock {

    -  explicit AutoLock(Lock& lock) : lock_(lock) {
    +  explicit AutoLock(Lock& lock) EXCLUSIVE_LOCK_FUNCTION(lock) : lock_(lock) {

    -  AutoLock(Lock& lock, const AlreadyAcquired&) : lock_(lock) {
    +  AutoLock(Lock& lock, const AlreadyAcquired&) EXCLUSIVE_LOCKS_REQUIRED(lock) : lock_(lock) {
 
    -  ~AutoLock() {
    +  ~AutoLock() UNLOCK_FUNCTION() {

Example compile failure:

    ../../base/optional.h:283:22: error: releasing mutex 'storage_..value_' that was not held [-Werror,-Wthread-safety-analysis]
        storage_.value_.~T();
                         ^
    ../../base/optional.h:679:5: note: in instantiation of member function 'base::internal::OptionalBase<base::AutoLock>::FreeIfNeeded' requested here
        FreeIfNeeded();
        ^
    ../../services/ws/public/cpp/gpu/context_provider_command_buffer.cc:477:10: note: in instantiation of function template specialization 'base::Optional<base::AutoLock>::emplace<base::Lock &>' requested here
        hold.emplace(context_lock_);
             ^

Source code of base::OptionalBase<T>::FreeIfNeeded:

      void FreeIfNeeded() {
        if (!storage_.is_populated_)
          return;
        storage_.value_.~T();
        storage_.is_populated_ = false;
      }

Source code of base::Optional<T>::emplace:

      template <class... Args>
      T& emplace(Args&&... args) {
        FreeIfNeeded();
        storage_.Init(std::forward<Args>(args)...);
        return storage_.value_;
      }

Let's focus this bug on the minimal annotations that will help analyse/enforce base::AutoLock-protected fields.  I've opened a separate  issue 881903  for annotations needed for analysis/enforcement when base::Lock methods are called directly.

WIP CL @ https://crrev.com/c/1213272 (gn_all builds fine locally, haven't yet finished running tryjobs)

Upside of the CL:
- Enables meaningful (i.e. enforced by the compiler) GUARDED_BY annotation of fields protected via base::AutoLock usage.  Example in content/browser/child_process_security_policy_impl.h.

Downsides of the CL:
- Going forward all uses of base::AutoLock::AlreadyAcquired require NO_THREAD_SAFETY_ANALYSIS annotation, because base::Lock::Lock and base::Lock::Try are not annotated by the CL.  AFAICT there is currently only one usage of base::AutoLock::AlreadyAcquired in Chromium (in HistogramManager::GetDeltas).
- The NO_THREAD_SAFETY_ANALYSIS annotation in //base/optional.h is unfortunate.  Any suggestions on how to get rid of it would be appreciated.

The CL from #c3 fails to compile on


1. Win/Dbg (release build works fine)

../..\base/synchronization/lock.h(21,1):  error: declaration of anonymous class must be a definition
class BASE_EXPORT LOCKABLE Lock {
^
../..\base/synchronization/lock.h(21,1):  error: declaration does not declare anything [-Werror,-Wmissing-declarations]

I don't know how to debug further.  I've tried to add "-E" as follows:
    --- a/base/BUILD.gn
    +++ b/base/BUILD.gn
    @@ -70,6 +70,8 @@ if (is_fuchsia) {
     config("base_flags") {
       if (is_clang) {
         cflags = [
    +      "-E",
    +
but I can't find the pre-processed file anywhere / I don't know where to look...


2. Mac

../../media/capture/video/mac/video_capture_device_avfoundation_mac.mm:218:18: error: cannot resolve lock expression [-Werror,-Wthread-safety-analysis]
  base::AutoLock lock(lock_);
                 ^~~~

I guess I could solve #2 by slapping another NO_THREAD_SAFETY_ANALYSIS here?  Or maybe ObjC should be excluded from the analysis (that's probably undesirable if we ever need to analyse across C++ / ObjC boundary)?

And I guess I could also tweak THREAD_ANNOTATION_ATTRIBUTE__ definition so that it becomes a no-op on Win/Dbg + Mac builds?  This would solve both 1 and 2 (but then it seems undesirable to take away the current locks analysis from Blink [which seems to work fine on Windows and Mac]).


Any comments?  Help please? :-)

For 1, I suggest doing a local build with ninja, copying the failing command line, and then copying it and editing it. With clang-cl you want to add /FA /Fafoo.ii to write the preprocessed output to foo.ii.

For 2, is that a compiler bug? Maybe just file a reduced repro upstream?

( issue 875627  makes me believe that nobody uses these annotations in production for anything though...)

I've made some progress on the Windows problem - I've swapped the order of LOCKABLE and BASE_EXPORT and it helped in //base/synchronization/lock.h.  OTOH, the same error remains in 

../..\net/third_party/quic/platform/api/quic_mutex.h(14,1):  error: declaration of anonymous class must be a definition
class QUIC_EXPORT_PRIVATE LOCKABLE QuicMutex {

I am not sure what my options are here:
1. Turn off thread annotations on Windows?
2. Change the order of declarations in third_party/quic/platform/api/quic_mutex.h?
3. Somehow insulate third_party/quic/platform/api/quic_mutex.h from LOCKABLE definition coming from //base/thread_annotations.h
I think I'll try to pursue option #2 (for now in a very naive way of doing this in my CL, rather than landing something in an upstream QUIC repository).


When debugging the Mac problem, I've stumbled upon a NOTREACHED in clang and opened https://bugs.llvm.org/show_bug.cgi?id=38892, but I still need to repro the original problem. 

What's the root cause here? What does LOCKABLE expand to? I suppose that doesn't commutate __declspec(dllexport)? Maybe we could fix that in the compiler, but 2 sounds preferable.

RE: #c7: thakis@: LOCKABLE expands to __attribute__((lockable))

And the clang bug for the real issue (error: cannot resolve lock expression) is here: https://bugs.llvm.org/show_bug.cgi?id=38896

In the meantime I guess we have 2 options:
1. Annotate (and keep annotating going forward) ObjC methods using locks with NO_THREAD_SAFETY_ANALYSIS.
2. Turn off lock safety analysis on Mac.
I think I lean toward #2 (more sustainable than #1;  hopefully there isn't much non-ObjC, Mac-only code that uses locks).

3. Fix compiler bugs

FWIW, the problem in #c1 seems to be related to Chromium's implementation of base::Optional - the problem doesn't happen with std::optional<T> - see https://godbolt.org/z/2XG89f.  OTOH, we still get another error, because the analyser doesn't know that holding AutoLock inside std::optional counts as protecting g_value.  I've opened for the latter error https://bugs.llvm.org/show_bug.cgi?id=38904.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5e71bd454e60511c1293c0c686544aaa76094424

commit 5e71bd454e60511c1293c0c686544aaa76094424
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Mon Sep 17 19:28:57 2018

Minimal thread-safety annotations for base::AutoLock.

This CL adds minimal thread-safety annotations for base::AutoLock.
Note that direct usage of base::Lock is not covered by this CL
(i.e. it won't be automatically analysed after this CL).


Thanks to the added base::AutoLock annotations, an example
GUARDED_BY annotation could be added to ChildProcessSecurityPolicyImpl:

  base::flat_set<url::Origin> isolated_origins_ GUARDED_BY(lock_);

Because of the new GUARDED_BY annotation forgetting to take a lock:

  // ad-hoc test of lock annotations // base::AutoLock lock(lock_);
  isolated_origins_.insert(origins_to_add.begin(), origins_to_add.end());

would result in the following compile error:

    ../../content/browser/child_process_security_policy_impl.cc:1236:3:
    error: reading variable 'isolated_origins_' requires holding mutex 'lock_'
    [-Werror,-Wthread-safety-analysis]
      isolated_origins_.insert(origins_to_add.begin(), origins_to_add.end());
      ^


This CL requires a few workarounds to deal with known issues:

- Thread annotation macros have to become no-ops on Mac to deal
  with https://bugs.llvm.org/show_bug.cgi?id=38896

- base::OptionalBase::FreeIfNeeded has to be annotated with
  NO_THREAD_SAFETY_ANALYSIS because of https://crbug.com/881875#c1

- Uses of base::AutoLock::AlreadyAcquired have to be annotated with
  NO_THREAD_SAFETY_ANALYSIS until we can add more annotations
  to base::Lock (see  https://crbug.com/881903 ).

- Reversal of the order of QUIC_EXPORT_PRIVATE and LOCKABLE
  in net/third_party/quic/platform/api/quic_mutex.h to ensure
  that the build will succeed on Windows.  LOCKABLE expands
  to __attribute__((lockable)) and QUIC_EXPORT_PRIVATE expands to
  __declspec(dllexport) (or dllimport) and in a class declaration the
  former must come before the latter.


Bug: 881875
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;master.tryserver.chromium.android:android_cronet_tester
Change-Id: Id94e42fd730b7ba69e34651e6cf616420683a20c
Reviewed-on: https://chromium-review.googlesource.com/1213272
Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591776}
[modify] https://crrev.com/5e71bd454e60511c1293c0c686544aaa76094424/base/BUILD.gn
[modify] https://crrev.com/5e71bd454e60511c1293c0c686544aaa76094424/base/optional.h
[modify] https://crrev.com/5e71bd454e60511c1293c0c686544aaa76094424/base/synchronization/lock.h
[modify] https://crrev.com/5e71bd454e60511c1293c0c686544aaa76094424/base/thread_annotations.h
[modify] https://crrev.com/5e71bd454e60511c1293c0c686544aaa76094424/components/cronet/histogram_manager.cc
[modify] https://crrev.com/5e71bd454e60511c1293c0c686544aaa76094424/content/browser/child_process_security_policy_impl.h
[modify] https://crrev.com/5e71bd454e60511c1293c0c686544aaa76094424/content/browser/child_process_security_policy_unittest.cc
[modify] https://crrev.com/5e71bd454e60511c1293c0c686544aaa76094424/net/third_party/quic/platform/api/quic_mutex.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dca7cc817094010daa9121811e0cfc7c254abed2

commit dca7cc817094010daa9121811e0cfc7c254abed2
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Mon Oct 01 22:59:49 2018

Allow lock annotations on Mac.

Thanks to a clang fix (*) things work fine on Mac now.

(*) rC342600 fixes https://bugs.llvm.org/show_bug.cgi?id=38896

Bug: 881875
Change-Id: I8c65848bf09f1a7f80f640e8b2e09bb2ef1e3789
Reviewed-on: https://chromium-review.googlesource.com/1249837
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595619}
[modify] https://crrev.com/dca7cc817094010daa9121811e0cfc7c254abed2/base/BUILD.gn
[modify] https://crrev.com/dca7cc817094010daa9121811e0cfc7c254abed2/base/thread_annotations.h

I see that a pattern e.g.

  Bla input_buffers_ GUARDED_BY(lock_);

and 

  lock_.AssertAcquired();
  // ...
  input_buffers_.pop();

causes

... : error: reading variable 'input_buffers_' requires holding mutex 'lock_' [-Werror,-Wthread-safety-analysis]
  input_buffers_.pop();

Shouldn't this work?

LLVM will definitely not know anything about Lock::AssertAcquired() until its declaration is annotated with ASSERT_EXCLUSIVE_LOCK(). In general, base::Lock doesn't appear to have any annotations yet.

Separately, I'm not sure how well ASSERT_EXCLUSIVE_LOCK informs the analyzer. The "straightforward" way of doing what you want would be to annotate the function calling AssertAcquired() with EXCLUSIVE_LOCKS_REQUIRED(). That should silence the GUARDED_BY warning, and should let Clang statically determine if invariant is broken anywhere.

The EXCLUSIVE_LOCKS_REQUIRED() annotation on the function itself
seems to have done the trick, thanks !!