Update Condition to allocate 24 bytes for MSVC platform pointers to methods.

PiperOrigin-RevId: 488986942
Change-Id: I2babb7ea30d60c544f55ca9ed02d9aed23051a12

6 files changed, 174 insertions, 103 deletions

diff --git a/absl/synchronization/BUILD.bazel b/absl/synchronization/BUILD.bazel
index 078e22a6..ccaee796 100644
--- a/absl/synchronization/BUILD.bazel
+++ b/absl/synchronization/BUILD.bazel
@@ -224,6 +224,18 @@ cc_test(
     ],
 )
 
+cc_test(
+    name = "mutex_method_pointer_test",
+    srcs = ["mutex_method_pointer_test.cc"],
+    copts = ABSL_TEST_COPTS,
+    linkopts = ABSL_DEFAULT_LINKOPTS,
+    deps = [
+        ":synchronization",
+        "//absl/base:config",
+        "@com_google_googletest//:gtest_main",
+    ],
+)
+
 cc_library(
     name = "mutex_benchmark_common",
     testonly = 1,
diff --git a/absl/synchronization/CMakeLists.txt b/absl/synchronization/CMakeLists.txt
index 975cb995..f64653bb 100644
--- a/absl/synchronization/CMakeLists.txt
+++ b/absl/synchronization/CMakeLists.txt
@@ -163,6 +163,20 @@ absl_cc_test(
 
 absl_cc_test(
   NAME
+    mutex_method_pointer_test
+  SRCS
+    "mutex_method_pointer_test.cc"
+  COPTS
+    ${ABSL_TEST_COPTS}
+  DEPS
+    absl::base
+    absl::config
+    absl::synchronization
+    GTest::gmock_main
+)
+
+absl_cc_test(
+  NAME
     notification_test
   SRCS
     "notification_test.cc"
diff --git a/absl/synchronization/mutex.cc b/absl/synchronization/mutex.cc
index ff18df5d..dd771421 100644
--- a/absl/synchronization/mutex.cc
+++ b/absl/synchronization/mutex.cc
@@ -2823,7 +2823,7 @@ bool Condition::GuaranteedEqual(const Condition *a, const Condition *b) {
   }
   // Check equality of the representative fields.
   return a->eval_ == b->eval_ && a->arg_ == b->arg_ &&
-         !memcmp(a->callback_, b->callback_, sizeof(ConservativeMethodPointer));
+         !memcmp(a->callback_, b->callback_, sizeof(a->callback_));
 }
 
 ABSL_NAMESPACE_END
diff --git a/absl/synchronization/mutex.h b/absl/synchronization/mutex.h
index 54ee703a..779aafa0 100644
--- a/absl/synchronization/mutex.h
+++ b/absl/synchronization/mutex.h
@@ -751,21 +751,19 @@ class Condition {
   // multiple inheritance are bigger than those of classes with single
   // inheritance. Other variations also exist.
 
-  // A good way to allocate enough space for *any* pointer in these ABIs is to
-  // employ a class declaration with no definition. Because the inheritance
-  // structure is not available for this declaration, the compiler must
-  // assume, conservatively, that its method pointers have the largest possible
-  // size.
-  class OpaqueClass;
-  using ConservativeMethodPointer = bool (OpaqueClass::*)();
-  static_assert(sizeof(bool(OpaqueClass::*)()) >= sizeof(bool (*)(void *)),
-                "Unsupported platform.");
-
+#ifndef _MSC_VER
   // Allocation for a function pointer or method pointer.
   // The {0} initializer ensures that all unused bytes of this buffer are
   // always zeroed out.  This is necessary, because GuaranteedEqual() compares
   // all of the bytes, unaware of which bytes are relevant to a given `eval_`.
-  char callback_[sizeof(ConservativeMethodPointer)] = {0};
+  using MethodPtr = bool (Condition::*)();
+  char callback_[sizeof(MethodPtr)] = {0};
+#else
+  // It is well known that the larget MSVC pointer-to-member is 24 bytes. This
+  // may be the largest known pointer-to-member of any platform. For this
+  // reason we will allocate 24 bytes for MSVC platform toolchains.
+  char callback_[24] = {0};
+#endif
 
   // Function with which to evaluate callbacks and/or arguments.
   bool (*eval_)(const Condition*);
diff --git a/absl/synchronization/mutex_method_pointer_test.cc b/absl/synchronization/mutex_method_pointer_test.cc
new file mode 100644
index 00000000..1ec801a0
--- /dev/null
+++ b/absl/synchronization/mutex_method_pointer_test.cc
@@ -0,0 +1,138 @@
+// Copyright 2017 The Abseil Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "absl/synchronization/mutex.h"
+
+#include <cstdlib>
+#include <string>
+
+#include "gtest/gtest.h"
+#include "absl/base/config.h"
+
+namespace {
+
+class IncompleteClass;
+
+#ifdef _MSC_VER
+// These tests verify expectations about sizes of MSVC pointers to methods.
+// Pointers to methods are distinguished by whether their class hierachies
+// contain single inheritance, multiple inheritance, or virtual inheritence.
+
+// Declare classes of the various MSVC inheritance types.
+class __single_inheritance SingleInheritance{};
+class __multiple_inheritance MultipleInheritance;
+class __virtual_inheritance VirtualInheritance;
+
+TEST(MutexMethodPointerTest, MicrosoftMethodPointerSize) {
+  void (SingleInheritance::*single_inheritance_method_pointer)();
+  void (MultipleInheritance::*multiple_inheritance_method_pointer)();
+  void (VirtualInheritance::*virtual_inheritance_method_pointer)();
+
+#if defined(_M_IX86) || defined(_M_ARM)
+  static_assert(sizeof(single_inheritance_method_pointer) == 4,
+                "Unexpected sizeof(single_inheritance_method_pointer).");
+  static_assert(sizeof(multiple_inheritance_method_pointer) == 8,
+                "Unexpected sizeof(multiple_inheritance_method_pointer).");
+  static_assert(sizeof(virtual_inheritance_method_pointer) == 12,
+                "Unexpected sizeof(virtual_inheritance_method_pointer).");
+#elif defined(_M_X64) || defined(__aarch64__)
+  static_assert(sizeof(single_inheritance_method_pointer) == 8,
+                "Unexpected sizeof(single_inheritance_method_pointer).");
+  static_assert(sizeof(multiple_inheritance_method_pointer) == 16,
+                "Unexpected sizeof(multiple_inheritance_method_pointer).");
+  static_assert(sizeof(virtual_inheritance_method_pointer) == 16,
+                "Unexpected sizeof(virtual_inheritance_method_pointer).");
+#endif
+  void (IncompleteClass::*incomplete_class_method_pointer)();
+  static_assert(sizeof(incomplete_class_method_pointer) >=
+                    sizeof(virtual_inheritance_method_pointer),
+                "Failed invariant: sizeof(incomplete_class_method_pointer) >= "
+                "sizeof(virtual_inheritance_method_pointer)!");
+}
+
+class Callback {
+  bool x = true;
+
+ public:
+  Callback() {}
+  bool method() {
+    x = !x;
+    return x;
+  }
+};
+
+class M2 {
+  bool x = true;
+
+ public:
+  M2() {}
+  bool method2() {
+    x = !x;
+    return x;
+  }
+};
+
+class MultipleInheritance : public Callback, public M2 {};
+
+TEST(MutexMethodPointerTest, ConditionWithMultipleInheritanceMethod) {
+  // This test ensures that Condition can deal with method pointers from classes
+  // with multiple inheritance.
+  MultipleInheritance object = MultipleInheritance();
+  absl::Condition condition(&object, &MultipleInheritance::method);
+  EXPECT_FALSE(condition.Eval());
+  EXPECT_TRUE(condition.Eval());
+}
+
+class __virtual_inheritance VirtualInheritance : virtual public Callback {
+  bool x = false;
+
+ public:
+  VirtualInheritance() {}
+  bool method() {
+    x = !x;
+    return x;
+  }
+};
+
+TEST(MutexMethodPointerTest, ConditionWithVirtualInheritanceMethod) {
+  // This test ensures that Condition can deal with method pointers from classes
+  // with virtual inheritance.
+  VirtualInheritance object = VirtualInheritance();
+  absl::Condition condition(&object, &VirtualInheritance::method);
+  EXPECT_TRUE(condition.Eval());
+  EXPECT_FALSE(condition.Eval());
+}
+#endif  // #ifdef _MSC_VER
+
+TEST(MutexMethodPointerTest, ConditionWithIncompleteClassMethod) {
+  using IncompleteClassMethodPointer = void (IncompleteClass::*)();
+
+  union CallbackSlot {
+    void (*anonymous_function_pointer)();
+    IncompleteClassMethodPointer incomplete_class_method_pointer;
+  };
+
+  static_assert(sizeof(CallbackSlot) >= sizeof(IncompleteClassMethodPointer),
+                "The callback slot is not big enough for method pointers.");
+  static_assert(
+      sizeof(CallbackSlot) == sizeof(IncompleteClassMethodPointer),
+      "The callback slot is not big enough for anonymous function pointers.");
+
+#if defined(_MSC_VER)
+  static_assert(sizeof(IncompleteClassMethodPointer) <= 24,
+                "The pointer to a method of an incomplete class is too big.");
+#endif
+}
+
+}  // namespace
diff --git a/absl/synchronization/mutex_test.cc b/absl/synchronization/mutex_test.cc
index f3d60852..34751cb1 100644
--- a/absl/synchronization/mutex_test.cc
+++ b/absl/synchronization/mutex_test.cc
@@ -1730,95 +1730,4 @@ TEST(Mutex, SignalExitedThread) {
   for (auto &th : top) th.join();
 }
 
-#ifdef _MSC_VER
-
-// Declare classes of the various MSVC inheritance types.
-class __single_inheritance SingleInheritance{};
-class __multiple_inheritance MultipleInheritance;
-class __virtual_inheritance VirtualInheritance;
-class UnknownInheritance;
-
-TEST(ConditionTest, MicrosoftMethodPointerSize) {
-  // This test verifies expectations about sizes of MSVC pointers to methods.
-  // Pointers to methods are distinguished by whether their class hierachies
-  // contain single inheritance, multiple inheritance, or virtual inheritence.
-  void (SingleInheritance::*single_inheritance)();
-  void (MultipleInheritance::*multiple_inheritance)();
-  void (VirtualInheritance::*virtual_inheritance)();
-  void (UnknownInheritance::*unknown_inheritance)();
-
-#if defined(_M_IX86) || defined(_M_ARM)
-  static_assert(sizeof(single_inheritance) == 4,
-                "Unexpected sizeof(single_inheritance).");
-  static_assert(sizeof(multiple_inheritance) == 8,
-                "Unexpected sizeof(multiple_inheritance).");
-  static_assert(sizeof(virtual_inheritance) == 12,
-                "Unexpected sizeof(virtual_inheritance).");
-#elif defined(_M_X64) || defined(__aarch64__)
-  static_assert(sizeof(single_inheritance) == 8,
-                "Unexpected sizeof(single_inheritance).");
-  static_assert(sizeof(multiple_inheritance) == 16,
-                "Unexpected sizeof(multiple_inheritance).");
-  static_assert(sizeof(virtual_inheritance) == 16,
-                "Unexpected sizeof(virtual_inheritance).");
-#endif
-  static_assert(sizeof(unknown_inheritance) >= sizeof(virtual_inheritance),
-                "Failed invariant: sizeof(unknown_inheritance) >= "
-                "sizeof(virtual_inheritance)!");
-}
-
-class Callback {
-  bool x = true;
-
- public:
-  Callback() {}
-  bool method() {
-    x = !x;
-    return x;
-  }
-};
-
-class M2 {
-  bool x = true;
-
- public:
-  M2() {}
-  bool method2() {
-    x = !x;
-    return x;
-  }
-};
-
-class MultipleInheritance : public Callback, public M2 {};
-
-TEST(ConditionTest, ConditionWithMultipleInheritanceMethod) {
-  // This test ensures that Condition can deal with method pointers from classes
-  // with multiple inheritance.
-  MultipleInheritance object = MultipleInheritance();
-  absl::Condition condition(&object, &MultipleInheritance::method);
-  EXPECT_FALSE(condition.Eval());
-  EXPECT_TRUE(condition.Eval());
-}
-
-class __virtual_inheritance VirtualInheritance : virtual public Callback {
-  bool x = false;
-
- public:
-  VirtualInheritance() {}
-  bool method() {
-    x = !x;
-    return x;
-  }
-};
-
-TEST(ConditionTest, ConditionWithVirtualInheritanceMethod) {
-  // This test ensures that Condition can deal with method pointers from classes
-  // with virtual inheritance.
-  VirtualInheritance object = VirtualInheritance();
-  absl::Condition condition(&object, &VirtualInheritance::method);
-  EXPECT_TRUE(condition.Eval());
-  EXPECT_FALSE(condition.Eval());
-}
-#endif
-
 }  // namespace