From 4b48854949f8bf9afb871c293a9022331a0b77c7 Mon Sep 17 00:00:00 2001 From: Abseil Team Date: Wed, 16 Nov 2022 11:05:21 -0800 Subject: Update Condition to allocate 24 bytes for MSVC platform pointers to methods. PiperOrigin-RevId: 488986942 Change-Id: I2babb7ea30d60c544f55ca9ed02d9aed23051a12 --- absl/synchronization/BUILD.bazel | 12 ++ absl/synchronization/CMakeLists.txt | 14 +++ absl/synchronization/mutex.cc | 2 +- absl/synchronization/mutex.h | 20 ++-- absl/synchronization/mutex_method_pointer_test.cc | 138 ++++++++++++++++++++++ absl/synchronization/mutex_test.cc | 91 -------------- 6 files changed, 174 insertions(+), 103 deletions(-) create mode 100644 absl/synchronization/mutex_method_pointer_test.cc (limited to 'absl/synchronization') 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 @@ -161,6 +161,20 @@ absl_cc_test( GTest::gmock_main ) +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 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 +#include + +#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 -- cgit v1.2.3