You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by zw...@apache.org on 2020/07/22 17:46:27 UTC
[trafficserver] 01/02: Extendible asan simple (#6650)
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 <ac...@verizonmedia.com>
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);