Loosen/improve AtomicFlag semantics.

base/synchronization/atomic_flag.h

Index: base/synchronization/atomic_flag.h
diff --git a/base/synchronization/atomic_flag.h b/base/synchronization/atomic_flag.h
index c754e8729c87769098ba1c54ef6447a340555a05..7b361d720cfb1152323da9082d7cd81212147228 100644
--- a/base/synchronization/atomic_flag.h
+++ b/base/synchronization/atomic_flag.h
@@ -8,7 +8,7 @@
 #include "base/atomicops.h"
 #include "base/base_export.h"
 #include "base/macros.h"
-#include "base/threading/thread_checker.h"
+#include "base/sequence_checker.h"
 
 namespace base {
 
@@ -20,19 +20,24 @@ class BASE_EXPORT AtomicFlag {
   AtomicFlag();
   ~AtomicFlag() = default;
 
-  // Set the flag. May only be called on the thread which created the object.
+  // Set the flag. Must always be called from the same sequence.
   void Set();
 
-  // Returns true iff the flag was set.
+  // Returns true iff the flag was set. If this returns true, the current thread
+  // is guaranteed to be synchronized with all memory operations on the sequence
+  // which invoked Set() up until at least the first call to Set() on it.
   bool IsSet() const;
 
   // Resets the flag. Be careful when using this: callers might not expect
-  // IsSet() to return false after returning true once.
-  void UnsafeResetForTesting();
+  // IsSet() to return false after returning true once. After a call to
+  // UnsafeReset(), the AtomicFlag is safe and regains it's initial properties

[danakj, 2016/08/01 17:14:56]

I dont like the comment saying it becomes safe by calling an unsafe method.

[gab, 2016/08/01 17:46:45]

Well it's "unsafe" unless external measures (as described) are taken to make it
safe.

But okay to keep it as ForTesting for now.

+  // (Set() may even bind to a new sequence) if and only if all threads using it
+  // are synchronized past the call to UnsafeReset().
+  void UnsafeReset();

[fdoray, 2016/08/01 16:15:51]

Keep ForTesting() if you no longer need to call this outside of tests.

[danakj, 2016/08/01 17:14:56]

I agree.

 
  private:
   base::subtle::Atomic32 flag_ = 0;
-  ThreadChecker thread_checker_;
+  SequenceChecker set_sequence_checker_;

[fdoray, 2016/07/29 20:41:37]

Given that there are no guarantees about the synchronization of memory
operations made after the first call to Set(), I'm not convinced that
SequenceChecker is useful (Why should we disallow calls to Set() from different
sequences if IsSet() callers cannot really assume anything about memory
synchronization when multiple calls to Set() are possible?)

I'm not strongly against keeping the SequenceChecker if you think it's useful.

[gab, 2016/08/01 15:08:29]

I think it's useful because the IsSet() callers can at least know which sequence
they're synchronizing with. If there are multiple callers to Set() than there
might as well not be barriers as the IsSet() callers can't assume anything about
the state of memory anyways (they're synced with "a" thread/sequence but don't
know which one...).

[fdoray, 2016/08/01 16:15:51]

If there are multiple callers to Set(), IsSet() callers can't assume anything
about the state of memory even if there is a SequenceChecker. Because of that, I
don't think that making sure that all calls to Set() occur on the same sequence
is useful.

However, SequenceChecker prevents Set() from being called from a PARALLEL task,
which is nice because it forces users to think about memory synchronization.

 
   DISALLOW_COPY_AND_ASSIGN(AtomicFlag);
 };