From f0350f5dd17575ad961bb93c50b887c448d5c7b5 Mon Sep 17 00:00:00 2001 From: Martin Suda Date: Fri, 9 Oct 2026 19:13:52 +0200 Subject: [PATCH 1/2] Store Coproduct's payload in a union, not a reinterpreted byte buffer Coproduct kept the live alternative in `char _content[maxSize]` and read it back through template B& cast() { return *(B*)_content; } which every payload access in the class funnels through (unwrap, and so compare, operator==, apply, operator<<). That is undefined twice over: - the returned pointer is derived from the char array, not from the object that reused its storage, which is exactly the case std::launder exists for; - the same bytes are written at one type (placement-new of the alternative, or a memberwise copy of the Coproduct) and read at another, which is the assumption -fstrict-aliasing is allowed to rely on being false. Replaced by a recursive union, so the compiler is told the truth -- these types share this memory -- and reading the live alternative is an ordinary member access at the right type. No cast, nothing to launder, and the union supplies the size and alignment, so MaxSize/MaxAlign are gone. cast() becomes castAt(). All five call sites already had the index in hand, and going through it means the access path used to read an alternative is the same path the placement-new constructed it through. It also removes a latent conflation: with duplicate alternative types, cast() resolved to the first index of type B rather than to the index actually asked for. VariantStorage has two specialisations, as a standard-library variant does. Nothing is implicitly constructed or destroyed in the storage -- Coproduct does both explicitly via TrivialOperations -- but the destructor has to stay *trivial* when every alternative is trivially destructible, or Coproduct loses its own triviality. Layout is unchanged. Compiling the old and the new header side by side gives sizeof 16 for Coproduct and 24 for Coproduct either way, and the in-header is_trivially_copyable> assertion still holds, as do is_trivially_copyable and is_trivially_destructible for a two-POD Coproduct and !is_trivially_destructible for Coproduct,int>. Found via an -flto=auto build of this branch, where vampire -sa otter Problems/SET/SET978+1.p could not find a substitution tree child that was demonstrably present: Top::compare, which reads the Coproduct payload, disagreed with the structure. Putting __attribute__((optimize("no-strict-aliasing"))) on cast() fixed it, and so did __attribute__((noinline)) alone -- the two preconditions of one mechanism, inlining bringing the write and the read into a single body and TBAA then being allowed to conclude they are unrelated. Verified: ctest 103/103, and SET978+1, PUZ032-1, SYN084-1, SEU144+1, SWV019-1, ANA019-2, GRA013+1 all answered correctly by a debug build. Co-Authored-By: Claude Opus 5 --- Lib/Coproduct.hpp | 107 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 76 insertions(+), 31 deletions(-) diff --git a/Lib/Coproduct.hpp b/Lib/Coproduct.hpp index 8bb29c162..5197d43e1 100644 --- a/Lib/Coproduct.hpp +++ b/Lib/Coproduct.hpp @@ -201,7 +201,7 @@ namespace CoproductImpl { { this->switchN([&](auto N) { using A = TL::Get; - this->template cast().~A(); + this->template castAt().~A(); }); } }; @@ -228,8 +228,8 @@ namespace CoproductImpl { this->assignTag(other.tag()); \ this->switchN([&](auto N) { \ using A = TL::Get; \ - ::new(&this->template cast()) \ - A(MOVE(other.template cast())); \ + ::new(&this->template castAt()) \ + A(MOVE(other.template castAt())); \ }); \ } \ \ @@ -239,7 +239,7 @@ namespace CoproductImpl { if (this == &other) return *this; \ this->switchN([&](auto N) { \ using A = TL::Get; \ - this->template cast().~A(); \ + this->template castAt().~A(); \ }); \ ::new(this) DefaultImpl(MOVE(other)); \ return *this; \ @@ -255,27 +255,65 @@ namespace CoproductImpl { } - template - struct MaxSize; - - template<> - struct MaxSize<> - { static constexpr unsigned value = 0; }; + /** + * Storage holding exactly one of As..., as a union rather than a byte buffer. + * + * The point of using a union is that it tells the compiler the truth: these types share + * this memory. Reading the live alternative is then an ordinary member access at the + * right type -- no cast from char*, nothing to launder -- so type-based alias analysis + * stays correct, and the union also gets us the right size and alignment for free. + * + * The previous representation was `char _content[maxSize]` read back as `*(B*)_content`, + * which is undefined twice over: the pointer is derived from the char array rather than + * from the object that reused its storage, and the same bytes are written at one type and + * read at another. GCC exploited it as soon as -flto inlined enough for the write and the + * read to meet in one function body. + * + * Nothing is implicitly constructed or destroyed here. Coproduct placement-news the live + * alternative and destroys it explicitly (see TrivialOperations), so the destructor is a + * no-op -- but it has to stay *trivial* when every alternative is trivially destructible, + * or Coproduct would lose its own triviality. Hence the two specialisations, as in a + * standard-library variant. + */ + template + union VariantStorage; + + template<> union VariantStorage {}; + template<> union VariantStorage {}; + + template + union VariantStorage { + A head; + VariantStorage tail; + /** leaves every alternative dead; Coproduct constructs the live one */ + VariantStorage() {} + }; - template - struct MaxSize - { static constexpr unsigned value = std::max(sizeof(T), MaxSize::value); }; + template + union VariantStorage { + A head; + VariantStorage tail; + VariantStorage() {} + /** the live alternative is destroyed by Coproduct, not here */ + ~VariantStorage() {} + }; - template - struct MaxAlign; + /** + * Reference to the idx-th alternative of a VariantStorage. Constness follows the + * storage. Note that the path taken here is the same path the placement-new in + * Coproduct takes, so the alternative we read is the one that was constructed. + */ + template + struct StorageAt { + template + static auto& get(S& self) { return StorageAt::get(self.tail); } + }; template<> - struct MaxAlign<> - { static constexpr unsigned value = 0; }; - - template - struct MaxAlign - { static constexpr unsigned value = std::max(alignof(T), MaxAlign::value); }; + struct StorageAt<0> { + template + static auto& get(S& self) { return self.head; } + }; template class RawCoproduct { @@ -301,9 +339,13 @@ namespace CoproductImpl { static_assert(nTags == 0 || nTags - 1 == ((nTags - 1) & bitMask), "bug in function neededBits"); - using Bytes = char [MaxSize::value]; + /** trivial destructibility has to be inherited by the storage; see VariantStorage */ + static constexpr bool allTriviallyDestructible + = TL::All::val; + + using Storage = VariantStorage; unsigned _tag: neededBits(nTags); - alignas(MaxAlign::value) Bytes _content; + Storage _content; TrivialOperations::DisableIfNeeded _copyCons; @@ -317,8 +359,9 @@ namespace CoproductImpl { : _tag(size) { #if __COPRODUCT_CONTENT_INIT - for (unsigned i = 0; i < sizeof(Bytes); i++) { - _content[i] = 0xFF; + auto* bytes = reinterpret_cast(&_content); + for (unsigned i = 0; i < sizeof(Storage); i++) { + bytes[i] = 0xFF; } #endif // __COPRODUCT_CONTENT_INIT } @@ -333,11 +376,11 @@ namespace CoproductImpl { ) #define CONST_POLYMORPIHIC(CONST) \ - template \ - B CONST& cast() CONST \ + template \ + TL::Get CONST& castAt() CONST \ { \ - static_assert(TL::Contains>::val, "invalid cast"); \ - return *(B CONST*)_content; \ + static_assert(idx < size, "variant index out of bounds"); \ + return StorageAt::get(_content); \ } \ \ @@ -582,7 +625,7 @@ class Coproduct inline TL::Get REF unwrap() REF { \ static_assert(idx < size, "out of bounds"); \ ASS_EQ(idx, tag()); \ - return MOVE(_inner.template cast>()); \ + return MOVE(_inner.template castAt()); \ } \ \ /** \ @@ -633,7 +676,9 @@ class Coproduct static_assert(std::is_same>::value, "illegal index for variant"); _inner.template assignTag(); - ::new(&_inner._content) B(move_if_value(value._self)); + // construct through the same access path castAt() reads back through, so the + // alternative we later read is exactly the one whose lifetime started here + ::new(&_inner.template castAt()) B(move_if_value(value._self)); } /** From defe02cc8a9862a4a28035ee1fe7965e8328111f Mon Sep 17 00:00:00 2001 From: Martin Suda Date: Sat, 10 Oct 2026 21:47:47 +0200 Subject: [PATCH 2/2] shortened comments --- Lib/Coproduct.hpp | 43 +++++++++++++++---------------------------- 1 file changed, 15 insertions(+), 28 deletions(-) diff --git a/Lib/Coproduct.hpp b/Lib/Coproduct.hpp index 5197d43e1..e97c8db2d 100644 --- a/Lib/Coproduct.hpp +++ b/Lib/Coproduct.hpp @@ -256,24 +256,16 @@ namespace CoproductImpl { /** - * Storage holding exactly one of As..., as a union rather than a byte buffer. + * Storage holding exactly one of As..., as a union rather than a byte buffer: a union + * tells the compiler that these types share the memory, so reading the live alternative + * is an ordinary member access at the right type and alias analysis stays correct. + * Do not go back to a `char[]` read through a cast -- that is undefined, and GCC + * miscompiled it here under -flto. * - * The point of using a union is that it tells the compiler the truth: these types share - * this memory. Reading the live alternative is then an ordinary member access at the - * right type -- no cast from char*, nothing to launder -- so type-based alias analysis - * stays correct, and the union also gets us the right size and alignment for free. - * - * The previous representation was `char _content[maxSize]` read back as `*(B*)_content`, - * which is undefined twice over: the pointer is derived from the char array rather than - * from the object that reused its storage, and the same bytes are written at one type and - * read at another. GCC exploited it as soon as -flto inlined enough for the write and the - * read to meet in one function body. - * - * Nothing is implicitly constructed or destroyed here. Coproduct placement-news the live - * alternative and destroys it explicitly (see TrivialOperations), so the destructor is a - * no-op -- but it has to stay *trivial* when every alternative is trivially destructible, - * or Coproduct would lose its own triviality. Hence the two specialisations, as in a - * standard-library variant. + * Nothing is constructed or destroyed in the storage: Coproduct placement-news the live + * alternative and destroys it explicitly (see TrivialOperations). The destructor must + * still be *trivial* whenever every alternative is trivially destructible, or Coproduct + * loses its own triviality -- hence the two specialisations. */ template union VariantStorage; @@ -285,24 +277,20 @@ namespace CoproductImpl { union VariantStorage { A head; VariantStorage tail; - /** leaves every alternative dead; Coproduct constructs the live one */ - VariantStorage() {} + VariantStorage() {} // leaves every alternative dead }; template union VariantStorage { A head; VariantStorage tail; - VariantStorage() {} - /** the live alternative is destroyed by Coproduct, not here */ + VariantStorage() {} // leaves every alternative dead ~VariantStorage() {} }; - /** - * Reference to the idx-th alternative of a VariantStorage. Constness follows the - * storage. Note that the path taken here is the same path the placement-new in - * Coproduct takes, so the alternative we read is the one that was constructed. - */ + /** Reference to the idx-th alternative of a VariantStorage; constness follows the + * storage. Coproduct constructs through this same path, so the alternative read is + * the one that was constructed. */ template struct StorageAt { template @@ -676,8 +664,7 @@ class Coproduct static_assert(std::is_same>::value, "illegal index for variant"); _inner.template assignTag(); - // construct through the same access path castAt() reads back through, so the - // alternative we later read is exactly the one whose lifetime started here + // through castAt(), so the alternative constructed is the one read back ::new(&_inner.template castAt()) B(move_if_value(value._self)); }