This is an automated email from the ASF dual-hosted git repository. zwoop pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit 82ac7cf3a6e5c0a8f50b2ac8fcd0f9cea94776fe Author: a-a-ron <[email protected]> AuthorDate: Fri May 1 15:59:47 2020 -0500 Extendible asan simple (#6650) * Extendible ASAN Fix v2 + Asserting that construction and init are happing as expected. Every class that inherits from an extendible will need to: 1. allocate with ext::create<T>() 2. override operator delete to call free(ptr) * Extendible create with arg forwarding (cherry picked from commit b5692b1bd8b7fa217eddfbb2234b05dc44795c33) --- .../internal-libraries/Extendible.en.rst | 32 +++++----- include/tscore/Extendible.h | 58 ++++++++++++----- src/tscore/Extendible.cc | 11 ++-- src/tscore/unit_tests/test_Extendible.cc | 72 ++++++++++++++++++---- 4 files changed, 124 insertions(+), 49 deletions(-) diff --git a/doc/developer-guide/internal-libraries/Extendible.en.rst b/doc/developer-guide/internal-libraries/Extendible.en.rst index 26e0392..8bb595e 100644 --- a/doc/developer-guide/internal-libraries/Extendible.en.rst +++ b/doc/developer-guide/internal-libraries/Extendible.en.rst @@ -80,18 +80,19 @@ the type's constructor, destructor, and serializer. And to avoid corruption, the } -When an derived class is instantiated, :func:`template<> alloc()` will allocate a block of memory for the derived class and all added -fields. The only memory overhead per instance is an uint16 used as a offset to the start of the extendible block. +When an derived class is instantiated, :func:`template<> create()` will allocate a block of memory for the derived class and all added +fields. The only memory overhead per instance is an uint16 used as a offset to the start of the extendible block. Then the constructor of the class +is called, followed by the constructors of each extendible field. .. code-block:: cpp ExtendibleExample* alloc_example() { - return ext::alloc<ExtendibleExample>(); + return ext::create<ExtendibleExample>(); } Memory Layout ------------- -One block of memory is allocated per |Extendible|, which included all member variables and extended fields. +One block of memory is allocated per |Extendible|, which include all member variables and extended fields. Within the block, memory is arranged in the following order: #. Derived members (+padding align next field) @@ -129,8 +130,8 @@ which simplifies the code using it. Also this provides compile errors for common } PluginFunc() { - Food *banana = ext::alloc<Food>(); - Car *camry = ext::alloc<Car>(); + Food *banana = ext::create<Food>(); + Car *camry = ext::create<Car>(); // Common user errors @@ -140,11 +141,11 @@ which simplifies the code using it. Also this provides compile errors for common float speed = ext::get(banana,fld_max_speed); // ^^^^^^^^^^^^^ - // Compile error: Cannot downcast banana to type Extendible<Car> + // Compile error: Cannot convert banana to type Extendible<Car> float weight = ext::get(camry,fld_food_weight); // ^^^^^^^^^^^^^^^ - // Compile error: Cannot downcast camry to type Extendible<Food>, even though Car and Food each have a 'weight' field, the FieldId is strongly typed. + // Compile error: Cannot convert camry to type Extendible<Food>, even though Car and Food each have a 'weight' field, the FieldId is strongly typed. } @@ -157,20 +158,20 @@ which simplifies the code using it. Also this provides compile errors for common Fruit.schema.addField(fld_has_seeds, "has_seeds"); - Fruit mango = ext::alloc<Fruit>(); + Fruit mango = ext::create<Fruit>(); - ext::set(mango, fld_has_seeds) = true; // downcasts mango to Extendible<Fruit> - ext::set(mango, fld_food_weight) = 2; // downcasts mango to Extendible<Food> + ext::set(mango, fld_has_seeds) = true; // converts mango to Extendible<Fruit> + ext::set(mango, fld_food_weight) = 2; // converts mango to Extendible<Food> ext::set(mango, fld_max_speed) = 9; // ^^^^^^^^^^^^^ - // Compile error: Cannot downcast mango to type Extendible<Car> + // Compile error: Cannot convert mango to type Extendible<Car> Inheritance ----------- Unfortunately it is non-trivial handle multiple |Extendible| super types in the same inheritance tree. - :func:`template<> alloc()` handles allocation and initialization of the entire `Derived` class, but it is dependent on each class defining :code:`using super_type = *some_super_class*;` so that it recurse through the classes. + :func:`template<> create()` handles allocation and initialization of the entire `Derived` class, but it is dependent on each class defining :code:`using super_type = *some_super_class*;` so that it recurse through the classes. .. code-block:: cpp @@ -191,7 +192,7 @@ Inheritance ext::FieldId<A, atomic<uint16_t>> ext_a_1; ext::FieldId<C, uint16_t> ext_c_1; - C &x = *(ext::alloc<C>()); + C &x = *(ext::create<C>()); ext::viewFormat(x); :func:`viewFormat` prints a diagram of the position and size of bytes used within the allocated @@ -235,8 +236,9 @@ Namespace `ext` one schema instance per |Extendible| to define contained |FieldDesc| -.. function:: template<typename Derived_t> Extendible* alloc() +.. function:: template<typename Derived_t> Extendible* create() + To be used in place of `new Derived_t()`. Allocate a block of memory. Construct the base data. Recursively construct and initialize `Derived_t::super_type` and its |Extendible| classes. diff --git a/include/tscore/Extendible.h b/include/tscore/Extendible.h index 16630ed..2ac826f 100644 --- a/include/tscore/Extendible.h +++ b/include/tscore/Extendible.h @@ -51,6 +51,13 @@ #include "tscore/ink_defs.h" ////////////////////////////////////////// +/// SUPPORT MACRO +#define DEF_EXT_NEW_DEL(cls) \ + void *operator new(size_t sz) { return ats_malloc(ext::sizeOf<cls>()); } \ + void *operator new(size_t sz, void *ptr) { return ptr; } \ + void operator delete(void *ptr) { free(ptr); } + +////////////////////////////////////////// /// HELPER CLASSES ///////////////////////// @@ -141,9 +148,11 @@ namespace details // internal stuff { public: std::unordered_map<std::string, FieldDesc> fields; ///< defined elements of the blob by name - size_t alloc_size = 0; ///< bytes to allocate for fields - uint8_t alloc_align = 1; ///< alignment of block - std::atomic_uint instance_count = {0}; ///< the number of Extendible<Derived> instances in use. + size_t alloc_size = 0; ///< bytes to allocate for fields + uint8_t alloc_align = 1; ///< alignment of block + std::atomic<uint> cnt_constructed = {0}; ///< the number of Extendible<Derived> created. + std::atomic<uint> cnt_fld_constructed = {0}; ///< the number of Extendible<Derived> that constructed fields. + std::atomic<uint> cnt_destructed = {0}; ///< the number of Extendible<Derived> destroyed. public: Schema() {} @@ -163,7 +172,7 @@ namespace details // internal stuff /// ext::Extendible allows code (and Plugins) to declare member-like variables during system init. /* - * This class uses a special allocator (ext::alloc) to extend the memory allocated to store run-time static + * This class uses a special allocator (ext::create) to extend the memory allocated to store run-time static * variables, which are registered by plugins during system init. The API is in a functional style to support * multiple inheritance of Extendible classes. This is templated so static variables are instanced per Derived * type, because we need to have different field schema per type. @@ -199,10 +208,10 @@ public: protected: Extendible(); - // use ext::alloc() exclusively for allocation and initialization + // use ext::create() exclusively for allocation and initialization /** destruct all fields */ - ~Extendible() { schema.callDestructor(uintptr_t(this) + ext_loc); } + ~Extendible(); private: /** construct all fields */ @@ -214,7 +223,7 @@ private: return uintptr_t(this) + ext_loc; } - template <typename T> friend T *alloc(); + template <typename T> friend T *create(); template <typename T> friend uintptr_t details::initRecurseSuper(T &, uintptr_t); template <typename T> friend FieldPtr details::FieldPtrGet(Extendible<T> const &, details::FieldDesc const &); template <typename T> friend std::string viewFormat(T const &, uintptr_t, int); @@ -575,6 +584,17 @@ sizeOf(size_t size = sizeof(Derived_t)) template <class Derived_t> Extendible<Derived_t>::Extendible() { ink_assert(ext::details::areFieldsFinalized()); + // don't call callConstructor until the derived class is fully constructed. + ++schema.cnt_constructed; +} + +template <class Derived_t> Extendible<Derived_t>::~Extendible() +{ + // assert callConstructors was called. + ink_assert(ext_loc); + schema.callDestructor(uintptr_t(this) + ext_loc); + ++schema.cnt_destructed; + ink_assert(schema.cnt_destructed <= schema.cnt_fld_constructed); } /// tell this extendible where it's memory offset start is. Added to support inheriting from extendible classes @@ -584,8 +604,9 @@ Extendible<Derived_t>::initFields(uintptr_t start_ptr) { ink_assert(ext_loc == 0); start_ptr = ROUNDUP(start_ptr, schema.alloc_align); // pad the previous struct, so that our fields are memaligned correctly - ext_loc = uint16_t(start_ptr - uintptr_t(this)); // store the offset to be used by ext::get and ext::set - ink_assert(ext_loc < 256); + ink_assert(start_ptr - uintptr_t(this) < UINT16_MAX); + ext_loc = uint16_t(start_ptr - uintptr_t(this)); // store the offset to be used by ext::get and ext::set + ink_assert(ext_loc > 0); schema.callConstructor(start_ptr); // construct all fields return start_ptr + schema.alloc_size; // return the end of the extendible data } @@ -608,23 +629,26 @@ namespace details } return tail_ptr; } + } // namespace details // allocate and initialize an extendible data structure -template <typename Derived_t> +template <typename Derived_t, typename... Args> Derived_t * -alloc() +create(Args &&... args) { - static_assert(std::is_base_of<Extendible<Derived_t>, Derived_t>::value); + // don't instantiate until all Fields are finalized. ink_assert(ext::details::areFieldsFinalized()); - // calculate the memory needed + // calculate the memory needed for the class and all Extendible blocks const size_t type_size = ext::sizeOf<Derived_t>(); - // alloc a block of memory - Derived_t *ptr = (Derived_t *)ats_memalign(alignof(Derived_t), type_size); - // Extendible_t *ptr = (Extendible_t *)::operator new(type_size); // alloc a block of memory + + // alloc one block of memory + Derived_t *ptr = static_cast<Derived_t *>(ats_memalign(alignof(Derived_t), type_size)); + // construct (recursively super-to-sub class) - new (ptr) Derived_t(); + new (ptr) Derived_t(std::forward(args)...); + // define extendible blocks start offsets (recursively super-to-sub class) details::initRecurseSuper(*ptr, uintptr_t(ptr) + sizeof(Derived_t)); return ptr; diff --git a/src/tscore/Extendible.cc b/src/tscore/Extendible.cc index 2ca6237..7d39f8d 100644 --- a/src/tscore/Extendible.cc +++ b/src/tscore/Extendible.cc @@ -44,7 +44,7 @@ namespace details void Schema::updateMemOffsets() { - ink_release_assert(instance_count == 0); + ink_release_assert(cnt_constructed == cnt_destructed); uint32_t acc_offset = 0; alloc_align = 1; @@ -86,7 +86,7 @@ namespace details bool Schema::reset() { - if (instance_count > 0) { + if (cnt_constructed > cnt_destructed) { // free instances before calling this so we don't leak memory return false; } @@ -99,7 +99,9 @@ namespace details Schema::callConstructor(uintptr_t ext_loc) { ink_assert(ext_loc); - ++instance_count; // don't allow schema modification + ++cnt_fld_constructed; // don't allow schema modification + ink_assert(cnt_fld_constructed <= cnt_constructed); + // init all extendible memory to 0, incase constructors don't memset(reinterpret_cast<void *>(ext_loc), 0, alloc_size); @@ -119,7 +121,6 @@ namespace details elm.second.destructor(FieldPtr(ext_loc + elm.second.field_offset)); } } - --instance_count; } size_t @@ -132,7 +133,7 @@ namespace details bool Schema::no_instances() const { - return instance_count == 0; + return cnt_constructed == cnt_destructed; } } // namespace details diff --git a/src/tscore/unit_tests/test_Extendible.cc b/src/tscore/unit_tests/test_Extendible.cc index 3219b7a..6f3d306 100644 --- a/src/tscore/unit_tests/test_Extendible.cc +++ b/src/tscore/unit_tests/test_Extendible.cc @@ -65,6 +65,8 @@ TEST_CASE("AtomicBit Atomic test") // Extendible Inheritance Tests struct A : public Extendible<A> { + using self_type = A; + DEF_EXT_NEW_DEL(self_type); uint16_t a = {1}; }; @@ -74,14 +76,18 @@ class B : public A { public: using super_type = A; - uint16_t b = {2}; + using self_type = B; + DEF_EXT_NEW_DEL(self_type); + uint16_t b = {2}; }; class C : public B, public Extendible<C> { public: using super_type = B; - uint16_t c = {3}; + using self_type = C; + DEF_EXT_NEW_DEL(self_type); + uint16_t c = {3}; // operator[] template <typename F> @@ -105,6 +111,46 @@ memDelta(void *p, void *q) { return uintptr_t(q) - uintptr_t(p); } +A *a_ptr = nullptr; +TEST_CASE("Create A", "") +{ + ext::details::areFieldsFinalized() = true; + a_ptr = ext::create<A>(); + CHECK(Extendible<A>::schema.no_instances() == false); +} +TEST_CASE("Delete A", "") +{ + delete a_ptr; + CHECK(Extendible<A>::schema.no_instances()); +} +TEST_CASE("Create B", "") +{ + a_ptr = ext::create<B>(); + CHECK(Extendible<A>::schema.no_instances() == false); +} +TEST_CASE("Delete B", "") +{ + delete a_ptr; + CHECK(Extendible<A>::schema.no_instances()); +} +TEST_CASE("Create C", "") +{ + a_ptr = ext::create<C>(); + CHECK(Extendible<A>::schema.no_instances() == false); + CHECK(Extendible<C>::schema.no_instances() == false); +} +TEST_CASE("Delete C", "") +{ + delete static_cast<C *>(a_ptr); + CHECK(Extendible<A>::schema.no_instances()); + CHECK(Extendible<C>::schema.no_instances()); + CHECK(Extendible<A>::schema.cnt_constructed == 3); + CHECK(Extendible<A>::schema.cnt_fld_constructed == 3); + CHECK(Extendible<A>::schema.cnt_destructed == 3); + CHECK(Extendible<C>::schema.cnt_constructed == 1); + CHECK(Extendible<C>::schema.cnt_fld_constructed == 1); + CHECK(Extendible<C>::schema.cnt_destructed == 1); +} TEST_CASE("Extendible Memory Allocations", "") { ext::details::areFieldsFinalized() = false; @@ -117,7 +163,7 @@ TEST_CASE("Extendible Memory Allocations", "") CHECK(ext::sizeOf<B>() == w * 4); CHECK(ext::sizeOf<C>() == w * 7); - C &x = *(ext::alloc<C>()); + C &x = *(ext::create<C>()); // 0 1 2 3 4 5 6 //[ EA*, a, b,EC*, c, EA, EC] // @@ -146,7 +192,7 @@ TEST_CASE("Extendible Memory Allocations", "") TEST_CASE("Extendible Pointer Math", "") { - C &x = *(ext::alloc<C>()); + C &x = *(ext::create<C>()); CHECK(x.a == 1); CHECK(x.b == 2); @@ -179,6 +225,8 @@ TEST_CASE("Extendible Pointer Math", "") // Extendible is abstract and must be derived in a CRTP struct Derived : Extendible<Derived> { + using self_type = Derived; + DEF_EXT_NEW_DEL(self_type); string m_str; // operator[] for shorthand @@ -213,7 +261,7 @@ struct Derived : Extendible<Derived> { void * DerivedExtalloc() { - return ext::alloc<Derived>(); + return ext::create<Derived>(); } void DerivedExtFree(void *ptr) @@ -285,7 +333,7 @@ TEST_CASE("Extendible", "") // I don't use SECTIONS because this modifies static variables many times, is not thread safe. INFO("Extendible()") { - ptr = ext::alloc<Derived>(); + ptr = ext::create<Derived>(); REQUIRE(ptr != nullptr); } @@ -297,7 +345,7 @@ TEST_CASE("Extendible", "") INFO("Schema Reset") { - ptr = ext::alloc<Derived>(); + ptr = ext::create<Derived>(); REQUIRE(Derived::schema.no_instances() == false); REQUIRE(Derived::schema.reset() == false); delete ptr; @@ -309,7 +357,7 @@ TEST_CASE("Extendible", "") INFO("shared_ptr") { - shared_ptr<Derived> sptr(ext::alloc<Derived>()); + shared_ptr<Derived> sptr(ext::create<Derived>()); REQUIRE(Derived::schema.no_instances() == false); REQUIRE(sptr); } @@ -326,7 +374,7 @@ TEST_CASE("Extendible", "") INFO("Extendible delete ptr"); { for (int i = 0; i < 10; i++) { - ptr = ext::alloc<Derived>(); + ptr = ext::create<Derived>(); REQUIRE(ptr != nullptr); INFO(__LINE__); REQUIRE(Derived::schema.no_instances() == false); @@ -339,7 +387,7 @@ TEST_CASE("Extendible", "") INFO("test bit field"); { - shared_ptr<Derived> sptr{ext::alloc<Derived>()}; + shared_ptr<Derived> sptr{ext::create<Derived>()}; Derived &ref = *sptr; CHECK(ext::viewFormat(ref) == Derived::testFormat()); @@ -376,7 +424,7 @@ TEST_CASE("Extendible", "") CHECK(ext::sizeOf<Derived>() == expected_size); ext::details::areFieldsFinalized() = true; - shared_ptr<Derived> sptr(ext::alloc<Derived>()); + shared_ptr<Derived> sptr(ext::create<Derived>()); Derived &ref = *sptr; CHECK(ext::viewFormat(ref) == Derived::testFormat()); using Catch::Matchers::Contains; @@ -405,7 +453,7 @@ TEST_CASE("Extendible", "") size_t expected_size = sizeof(Derived) + 1 + sizeof(std::atomic_int) * 2; CHECK(ext::sizeOf<Derived>() == expected_size); - shared_ptr<Derived> sptr(ext::alloc<Derived>()); + shared_ptr<Derived> sptr(ext::create<Derived>()); Derived &ref = *sptr; CHECK(ext::get(ref, int_a) == 0); CHECK(ext::get(ref, int_b) == 0);
