From 09c8c2a29f02a3bdb5b31f53eff09ca4654feef0 Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Tue, 18 Jun 2024 01:15:50 +0200 Subject: [PATCH] Library: better handle the alignment issues explicitly While there might be the possibility to use the magic of the standard library, it seems prudent rather to handle this insidious problem explicitly, to make clear what is going on here. To allow for such explicit alignment handling, I have now changed the scheme of the storage definition; the actual buffer now starts ''behind'' the `ArrayBucket` object, which thereby becomes a metadata managing header. __To summarise the problem__: since we are maintaining a dynamically sized buffer, and since we do not want to expose the actual element type through the front-end object, we're necessarily bound to perform a raw-memory allocation. This is denoted in bytes, and thus the allocator can no longer manage the proper alignment automatically. Rather, we get a storage buffer with just ''some accidental'' alignment, and we must care to request a sufficient overhead to be able to shift the actual storage area forward to the next proper alignment boundary. Obviously this also implies that we must store this individual padding adjustment somewhere in the metadata, in order to be able to report the correct size of the block later on de-allocation. --- src/lib/several-builder.hpp | 41 ++++-- src/lib/several.hpp | 38 ++--- tests/library/several-builder-test.cpp | 22 +-- wiki/thinkPad.ichthyo.mm | 192 ++++++++++++++++++------- 4 files changed, 189 insertions(+), 104 deletions(-) diff --git a/src/lib/several-builder.hpp b/src/lib/several-builder.hpp index 31f73e544..ded0b5437 100644 --- a/src/lib/several-builder.hpp +++ b/src/lib/several-builder.hpp @@ -41,7 +41,15 @@ ** Since the actual data elements can (optionally) be of a different type than ** the exposed interface type \a I, additional storage and spacing is required ** in the element array. The field ArrayBucket::spread defines this spacing - ** and thus the offset used for subscript access. + ** and thus the offset used for subscript access. The actual data storage starts + ** immediately behind the ArrayBucket, which thus acts as a metadata header. + ** This arrangement requires a sufficiently sized raw memory allocation to place + ** the ArrayBucket and the actual data into. Moreover, the allocation code in + ** ElementFactory::create() is responsible to ensure proper alignment of the + ** data storage, especially when the payload data type has alignment requirements + ** beyond `alignof(void*)`, which is typically used by the standard heap allocator; + ** additional headroom is added proactively in this case, to be able to shift the + ** storage buffer ahead to the next alignment boundrary. ** ** # Handling of data elements ** @@ -144,9 +152,6 @@ namespace lib { /** * Helper to determine the »spread« required to hold * elements of type \a TY in memory _with proper alignment._ - * @warning assumes that the start of the buffer is also suitably aligned, - * which _may not be the case_ for **over-aligned objects** with - * `alignof(TY) > alignof(void*)` */ template size_t inline constexpr @@ -187,16 +192,30 @@ namespace lib { : Allo{std::move (allo)} { } + Bucket* - create (size_t cnt, size_t spread) + create (size_t cnt, size_t spread, size_t alignment =alignof(I)) { - size_t storageBytes = Bucket::requiredStorage (cnt, spread); + REQUIRE (cnt); + REQUIRE (spread); + size_t storageBytes = Bucket::storageOffset + cnt*spread; + if (alignment > alignof(void*)) // over-aligned data => reserve for alignment padding + storageBytes += (alignment - alignof(void*)); + // Step-1 : acquire the raw storage buffer std::byte* loc = AlloT::allocate (baseAllocator(), storageBytes); + ENSURE (0 == size_t(loc) % alignof(void*)); + + size_t offset = (size_t(loc) + Bucket::storageOffset) % alignment; + if (offset) // padding needed to next aligned location + offset = alignment - offset; + offset += Bucket::storageOffset; + ASSERT (storageBytes - offset >= cnt*spread); Bucket* bucket = reinterpret_cast (loc); using BucketAlloT = typename AlloT::template rebind_traits; auto bucketAllo = adaptAllocator(); - try { BucketAlloT::construct (bucketAllo, bucket, cnt*spread, spread); } + // Step-2 : construct the Bucket metadata | ▽ ArrayBucket ctor arg ▽ + try { BucketAlloT::construct (bucketAllo, bucket, storageBytes, offset, spread); } catch(...) { AlloT::deallocate (baseAllocator(), loc, storageBytes); @@ -205,6 +224,7 @@ namespace lib { return bucket; }; + template E& createAt (Bucket* bucket, size_t idx, ARGS&& ...args) @@ -218,6 +238,7 @@ namespace lib { return *loc; }; + template void destroy (ArrayBucket* bucket) @@ -234,7 +255,7 @@ namespace lib { ElmAlloT::destroy (elmAllo, elm); } } - size_t storageBytes = Bucket::requiredStorage (bucket->buffSiz); + size_t storageBytes = bucket->buffOffset + bucket->buffSiz; std::byte* loc = reinterpret_cast (bucket); AlloT::deallocate (baseAllocator(), loc, storageBytes); }; @@ -255,7 +276,7 @@ namespace lib { Bucket* realloc (Bucket* data, size_t cnt, size_t spread) { - Bucket* newBucket = Fac::create (cnt, spread); + Bucket* newBucket = Fac::create (cnt, spread, alignof(E)); if (data) try { newBucket->deleter = data->deleter; @@ -558,7 +579,7 @@ namespace lib { REQUIRE (oldSpread); REQUIRE (newSpread); REQUIRE (Coll::data_); - byte* oldPos = Coll::data_->storage; + byte* oldPos = Coll::data_->storage(); byte* newPos = oldPos; oldPos += idx * oldSpread; newPos += idx * newSpread; diff --git a/src/lib/several.hpp b/src/lib/several.hpp index 240992ba0..bd54806af 100644 --- a/src/lib/several.hpp +++ b/src/lib/several.hpp @@ -71,15 +71,21 @@ namespace lib { - namespace {// Storage implementation details + namespace {// Storage header implementation details - template + /** + * Metadata record placed immediately before the data storage. + * @remark SeveralBuilder uses a custom allocation scheme to acquire + * a sufficiently sized allocation for ArrayBucket + the data. + */ + template struct ArrayBucket { - ArrayBucket (size_t bytes, size_t elmSize = sizeof(I)) + ArrayBucket (size_t storageSize, size_t buffStart, size_t elmSize = sizeof(I)) : cnt{0} , spread{elmSize} - , buffSiz{bytes} + , buffSiz{storageSize - buffStart} + , buffOffset{buffStart} , deleter{nullptr} { } @@ -88,30 +94,24 @@ namespace lib { size_t cnt; size_t spread; size_t buffSiz; + size_t buffOffset; Deleter deleter; - /** mark start of the storage area */ - alignas(E) - std::byte storage[space]; + static constexpr size_t storageOffset = sizeof(ArrayBucket); - - static size_t - requiredStorage (size_t cnt, size_t spread =1) + /** data storage area starts immediately behind the ArrayBucket */ + std::byte* + storage() { - return sizeof(ArrayBucket) - sizeof(storage) - + cnt * spread; + return reinterpret_cast(this) + buffOffset; } - /** perform unchecked access into the storage area - * @note typically reaching behind the nominal end of this object - */ + /** perform unchecked access into the storage area */ I& subscript (size_t idx) { - std::byte* elm = storage; - size_t off = idx * spread; - elm += off; - ENSURE (storage <= elm and elm < storage+buffSiz); + std::byte* elm = storage() + (idx * spread); + ENSURE (storage() <= elm and elm < storage()+buffSiz); return * std::launder (reinterpret_cast (elm)); } diff --git a/tests/library/several-builder-test.cpp b/tests/library/several-builder-test.cpp index 7359c91d5..e33abaace 100644 --- a/tests/library/several-builder-test.cpp +++ b/tests/library/several-builder-test.cpp @@ -506,29 +506,9 @@ namespace test{ { // Scenario-2 : alignment struct Ali { - alignas(32) + alignas(64) char charm = 'u'; }; -SHOW_EXPR(sizeof(Ali)) -SHOW_EXPR(alignof(Ali)) -SHOW_EXPR(sizeof(ArrayBucket)) -SHOW_EXPR(alignof(ArrayBucket)) -SHOW_EXPR(sizeof(ArrayBucket)) -SHOW_EXPR(alignof(ArrayBucket)) -SHOW_EXPR(sizeof(ArrayBucket)) -SHOW_EXPR(alignof(ArrayBucket)) - - std::allocator> aliAllo; - std::allocator byteAllo; - - ArrayBucket * locAli = aliAllo.allocate(1); - std::byte* locByte = byteAllo.allocate(96); -SHOW_EXPR(loc(locAli)) -SHOW_EXPR(loc(locAli) % alignof(Ali)) -SHOW_EXPR(loc(locByte)) -SHOW_EXPR(loc(locByte) % alignof(Ali)) - aliAllo.destroy(locAli); - byteAllo.destroy(locByte); auto elms = makeSeveral().fillElm(5).build(); CHECK (5 == elms.size()); diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index 6d33488f0..9bd6d38c5 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -81799,7 +81799,7 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
- + @@ -81884,8 +81884,8 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
 - - + + @@ -81895,76 +81895,72 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
- - - - + + + + + + + + + + + + + + - - - +

auf Basis der buffSiz, d.h. der Größe des reinen Nutzdatenpuffers; auf diese konnte man noch den statisch ermittelbaren Overhead aufschlagen

- - + - - - +

Der Allocator macht nur eine raw-storage-Allocation, da is nix mit Alignment. Insofern kann der Beginn des gelieferten Speicherbereichs auch „daneben liegen“ — so daß wir einen Korrektur-Offset brauchen. Und der läßt sich nicht systematisch erschließen

- - + - + - - - - - - -

- Der Aufruf des Allokators erfolgt aus dem Builder, und an der Stelle ist ohne Weiteres der volle Typ konstelliert -

- - - + + + + - + + + - - - + + + + - - - +

...to allocate storage required for a single object whose alignment requirement exceeds __STDCPP_DEFAULT_NEW_ALIGNMENT__

- - + @@ -81972,27 +81968,106 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
 - - - +

...weshalb ich ja keinen wirklich passenden statischen Typ für ArrayBucket angeben kann, weil die Puffergröße erst zur Laufzeit bekannt wird.

- - + + + + + + + + + +

+ das ist eine gewisse Vereinfachung, die aber aller Wahrscheinlichkeit von allen Plattformen erfüllt wird, allein schon weil man sonst an jedem Objekt mit VTable herumfummeln müßte +

+ + + + + + + + + + + + + - + + + + + + + + + + + + + + + +

+ Der Aufruf des Allokators erfolgt aus dem Builder, und an der Stelle ist ohne Weiteres der volle Typ konstelliert +

+ + + + + + + +

+ Für den reinen Zugriff genügt ein vereinfachter Typ — insofern dadurch der Zugang zur dynamischen Layout-Information möglich wird +

+ + + + + + + + + + + + + + + + +
    +
  • + verwende sizeof(ArrayBucket<I>) als Offset +
    • +
  • + dimensioniere die eigentliche Allokation so, daß vorne das ArrayBucket reinpasst, und dahinter alle gewünschten Daten +
    • +
+ + + + + + + @@ -84299,7 +84374,7 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
- + @@ -84314,23 +84389,32 @@ Date:   Thu Apr 20 18:53:17 2023 +0200
 - - + + - + - - - +

Vorsicht Falle: der Allocator kann das gar nicht machen, wenn man ihm im Sinn von »placement new« explizit die Position vorgibt

- - + + + + + + + + + + + + +