[llvm] r354200 - Revert r354199: Make Optional<T> Trivially Copyable when T is trivially copyable

[Serge Guelton via llvm-commits]

On Mon, Feb 18, 2019 at 12:06:07PM -0800, David Blaikie wrote:
> It can be helpful to mention the motivation for a revert in the commit message
> so others can follow along - I know this has had a few issues with buildbots
> and such - could you link to/quote the failure you were addressing by reverting
> this patch?

Sure,

http://lab.llvm.org:8011/builders/clang-atom-d525-fedora-rel/builds/21074

After reverting and digging a bit in the issue, it appears that the Optional of StringRef was miscompiled,
I tried to isolate the bug in the following godbolt:

    https://godbolt.org/z/rC70NK


It turns out existing implementation of Optional was very fragile (e.g. there was std::forward instead of an std::move),
encapsultation was not respected (internals of OptionalStorage were exposed and used in Optional), and probably UB
(inplace new on a char array followed by a reinterpret_cast on the storage).

Current trunk implementation is inspired from libcxx implementation and relies on union storage to avoid most of UB
(it may still be subject to unproper laundry though, if my understanding is correct). And encapsulation looks decent to me.


> On Sat, Feb 16, 2019 at 1:46 AM Serge Guelton via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> 
>     Author: serge_sans_paille
>     Date: Sat Feb 16 01:47:23 2019
>     New Revision: 354200
> 
>     URL: http://llvm.org/viewvc/llvm-project?rev=354200&view=rev
>     Log:
>     Revert r354199: Make Optional<T> Trivially Copyable when T is trivially
>     copyable
> 
>     Modified:
>         llvm/trunk/include/llvm/ADT/Optional.h
>         llvm/trunk/unittests/ADT/OptionalTest.cpp
> 
>     Modified: llvm/trunk/include/llvm/ADT/Optional.h
>     URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/
>     Optional.h?rev=354200&r1=354199&r2=354200&view=diff
>     ===========================================================================
>     ===
>     --- llvm/trunk/include/llvm/ADT/Optional.h (original)
>     +++ llvm/trunk/include/llvm/ADT/Optional.h Sat Feb 16 01:47:23 2019
>     @@ -108,59 +108,6 @@ template <typename T, bool = is_triviall
>          return reinterpret_cast<const T *>(storage.buffer);
>        }
>      };
>     -template <typename T> struct OptionalStorage<T, true> {
>     -  AlignedCharArrayUnion<T> storage;
>     -  bool hasVal = false;
>     -
>     -  OptionalStorage() = default;
>     -
>     -  OptionalStorage(const T &y) : hasVal(true) { new (storage.buffer) T(y);
>     }
>     -  OptionalStorage(const OptionalStorage &O)  = default;
>     -  OptionalStorage(T &&y) : hasVal(true) {
>     -    new (storage.buffer) T(std::forward<T>(y));
>     -  }
>     -  OptionalStorage(OptionalStorage &&O) = default;
>     -
>     -  OptionalStorage &operator=(T &&y) {
>     -    if (hasVal)
>     -      *getPointer() = std::move(y);
>     -    else {
>     -      new (storage.buffer) T(std::move(y));
>     -      hasVal = true;
>     -    }
>     -    return *this;
>     -  }
>     -  OptionalStorage &operator=(OptionalStorage &&O) = default;
>     -
>     -  OptionalStorage &operator=(const T &y) {
>     -    if (hasVal)
>     -      *getPointer() = y;
>     -    else {
>     -      new (storage.buffer) T(y);
>     -      hasVal = true;
>     -    }
>     -    return *this;
>     -  }
>     -  OptionalStorage &operator=(const OptionalStorage &O) = default;
>     -
>     -  ~OptionalStorage() = default;
>     -
>     -  void reset() {
>     -    if (hasVal) {
>     -      (*getPointer()).~T();
>     -      hasVal = false;
>     -    }
>     -  }
>     -
>     -  T *getPointer() {
>     -    assert(hasVal);
>     -    return reinterpret_cast<T *>(storage.buffer);
>     -  }
>     -  const T *getPointer() const {
>     -    assert(hasVal);
>     -    return reinterpret_cast<const T *>(storage.buffer);
>     -  }
>     -};
> 
>      } // namespace optional_detail
> 
> 
>     Modified: llvm/trunk/unittests/ADT/OptionalTest.cpp
>     URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/
>     OptionalTest.cpp?rev=354200&r1=354199&r2=354200&view=diff
>     ===========================================================================
>     ===
>     --- llvm/trunk/unittests/ADT/OptionalTest.cpp (original)
>     +++ llvm/trunk/unittests/ADT/OptionalTest.cpp Sat Feb 16 01:47:23 2019
>     @@ -14,15 +14,8 @@
> 
>      #include <array>
> 
>     -
>      using namespace llvm;
> 
>     -static_assert(llvm::is_trivially_copyable<Optional<int>>::value,
>     -              "trivially copyable");
>     -
>     -static_assert(llvm::is_trivially_copyable<Optional<std::array<int,
>     3>>>::value,
>     -              "trivially copyable");
>     -
>      namespace {
> 
>      struct NonDefaultConstructible {
> 
> 
>     _______________________________________________
>     llvm-commits mailing list
>     llvm-commits at lists.llvm.org
>     https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>