1 files changed, 66 insertions, 38 deletions

diff --git a/absl/base/internal/atomic_hook.h b/absl/base/internal/atomic_hook.h
index 1f9a8102f7ec..47d4013928a0 100644
--- a/absl/base/internal/atomic_hook.h
+++ b/absl/base/internal/atomic_hook.h
@@ -18,28 +18,12 @@
 
 #include <atomic>
 #include <cassert>
+#include <cstdint>
 #include <utility>
 
 namespace absl {
 namespace base_internal {
 
-// In current versions of MSVC (as of July 2017), a std::atomic<T> where T is a
-// pointer to function cannot be constant-initialized with an address constant
-// expression.  That is, the following code does not compile:
-//   void NoOp() {}
-//   constexpr std::atomic<void(*)()> ptr(NoOp);
-//
-// This is the only compiler we support that seems to have this issue.  We
-// conditionalize on MSVC here to use a fallback implementation.  But we
-// should revisit this occasionally.  If MSVC fixes this compiler bug, we
-// can then change this to be conditionalized on the value on _MSC_FULL_VER
-// instead.
-#ifdef _MSC_FULL_VER
-#define ABSL_HAVE_FUNCTION_ADDRESS_CONSTANT_EXPRESSION 0
-#else
-#define ABSL_HAVE_FUNCTION_ADDRESS_CONSTANT_EXPRESSION 1
-#endif
-
 template <typename T>
 class AtomicHook;
 
@@ -55,7 +39,7 @@ class AtomicHook<ReturnType (*)(Args...)> {
  public:
   using FnPtr = ReturnType (*)(Args...);
 
-  constexpr AtomicHook() : hook_(DummyFunction) {}
+  constexpr AtomicHook() : hook_(kInitialValue) {}
 
   // Stores the provided function pointer as the value for this hook.
   //
@@ -64,28 +48,16 @@ class AtomicHook<ReturnType (*)(Args...)> {
   // as a memory_order_release operation, and read accesses are implemented as
   // memory_order_acquire.
   void Store(FnPtr fn) {
-    assert(fn);
-    FnPtr expected = DummyFunction;
-    hook_.compare_exchange_strong(expected, fn, std::memory_order_acq_rel,
-                                  std::memory_order_acquire);
-    // If the compare and exchange failed, make sure that's because hook_ was
-    // already set to `fn` by an earlier call.  Any other state reflects an API
-    // violation (calling Store() multiple times with different values).
-    //
-    // Avoid ABSL_RAW_CHECK, since raw logging depends on AtomicHook.
-    assert(expected == DummyFunction || expected == fn);
+    bool success = DoStore(fn);
+    static_cast<void>(success);
+    assert(success);
   }
 
   // Invokes the registered callback.  If no callback has yet been registered, a
   // default-constructed object of the appropriate type is returned instead.
   template <typename... CallArgs>
   ReturnType operator()(CallArgs&&... args) const {
-    FnPtr hook = hook_.load(std::memory_order_acquire);
-    if (ABSL_HAVE_FUNCTION_ADDRESS_CONSTANT_EXPRESSION || hook) {
-      return hook(std::forward<CallArgs>(args)...);
-    } else {
-      return ReturnType();
-    }
+    return DoLoad()(std::forward<CallArgs>(args)...);
   }
 
   // Returns the registered callback, or nullptr if none has been registered.
@@ -98,23 +70,79 @@ class AtomicHook<ReturnType (*)(Args...)> {
   // Load()() unless you must conditionalize behavior on whether a hook was
   // registered.
   FnPtr Load() const {
-    FnPtr ptr = hook_.load(std::memory_order_acquire);
+    FnPtr ptr = DoLoad();
     return (ptr == DummyFunction) ? nullptr : ptr;
   }
 
  private:
-#if ABSL_HAVE_FUNCTION_ADDRESS_CONSTANT_EXPRESSION
   static ReturnType DummyFunction(Args...) {
     return ReturnType();
   }
+
+  // Current versions of MSVC (as of September 2017) have a broken
+  // implementation of std::atomic<T*>:  Its constructor attempts to do the
+  // equivalent of a reinterpret_cast in a constexpr context, which is not
+  // allowed.
+  //
+  // This causes an issue when building with LLVM under Windows.  To avoid this,
+  // we use a less-efficient, intptr_t-based implementation on Windows.
+
+#ifdef _MSC_FULL_VER
+#define ABSL_HAVE_WORKING_ATOMIC_POINTER 0
 #else
-  static constexpr FnPtr DummyFunction = nullptr;
+#define ABSL_HAVE_WORKING_ATOMIC_POINTER 1
 #endif
 
+#if ABSL_HAVE_WORKING_ATOMIC_POINTER
+  static constexpr FnPtr kInitialValue = &DummyFunction;
+
+  // Return the stored value, or DummyFunction if no value has been stored.
+  FnPtr DoLoad() const { return hook_.load(std::memory_order_acquire); }
+
+  // Store the given value.  Returns false if a different value was already
+  // stored to this object.
+  bool DoStore(FnPtr fn) {
+    assert(fn);
+    FnPtr expected = DummyFunction;
+    hook_.compare_exchange_strong(expected, fn, std::memory_order_acq_rel,
+                                  std::memory_order_acquire);
+    const bool store_succeeded = (expected == DummyFunction);
+    const bool same_value_already_stored = (expected == fn);
+    return store_succeeded || same_value_already_stored;
+  }
+
   std::atomic<FnPtr> hook_;
+#else  // !ABSL_HAVE_WORKING_ATOMIC_POINTER
+  // Use a sentinel value unlikely to be the address of an actual function.
+  static constexpr intptr_t kInitialValue = 0;
+
+  static_assert(sizeof(intptr_t) >= sizeof(FnPtr),
+                "intptr_t can't contain a function pointer");
+
+  FnPtr DoLoad() const {
+    const intptr_t value = hook_.load(std::memory_order_acquire);
+    if (value == 0) {
+      return DummyFunction;
+    }
+    return reinterpret_cast<FnPtr>(value);
+  }
+
+  bool DoStore(FnPtr fn) {
+    assert(fn);
+    const auto value = reinterpret_cast<intptr_t>(fn);
+    intptr_t expected = 0;
+    hook_.compare_exchange_strong(expected, value, std::memory_order_acq_rel,
+                                  std::memory_order_acquire);
+    const bool store_succeeded = (expected == 0);
+    const bool same_value_already_stored = (expected == value);
+    return store_succeeded || same_value_already_stored;
+  }
+
+  std::atomic<intptr_t> hook_;
+#endif
 };
 
-#undef ABSL_HAVE_FUNCTION_ADDRESS_CONSTANT_EXPRESSION
+#undef ABSL_HAVE_WORKING_ATOMIC_POINTER
 
 }  // namespace base_internal
 }  // namespace absl