[ADT] Fix Optional ABI mismatch between clang & gcc

[kbenzie]

Summary

This patch addresses discussion in
https://bugs.llvm.org/show_bug.cgi?id=35978
and the email thread
https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180122/519188.html

Compiling LLVM with either clang or gcc and a dependant project which
uses the opposite compiler results in an ABI incompatibility in Optional
due to the trivially copyable optimisation in the OptionalStorage type
being enabled when compiling with clang and disabled when GCC.

Additionally, this patch removes undefined behavior in the copy assignment
operator of OptionalStorage. When OptionalStorage does not hold a
valid object and the copy assignment operator is invoked, the object
being copied must be copy constructed in the uninitialized storage.buffer.

In the scenario of installing LLVM from the apt packages and a
downstream project choosing to use a different compiler the resulting
application will exhibit runtime failures such as segmentation faults
when calling LLVM functions with an Optional parameter crossing which
cross the ABI boundary.

I believe this is a serious bug and if this patch is accepted should
also be backported to the 7.0 release branch as this has been a latent
issue since late January 2018.

I did not have enough information to be able to reproduce the
"miscompiles" mentioned on the bug tracker so am unable to verify if
those have been fixed this with patch, it is my hope that fixing the
undefined behavior should resolve these issues.

[timshen]

include/llvm/ADT/Optional.h

120

Will this suffice?
if (hasVal) { memcpy(&storage.buffer, &y.storage.buffer, sizeof(T)); }

[timshen]

include/llvm/ADT/Optional.h

120

Sorry, I meant

hasVal = y.hasVal;
if (hasVal) { memcpy(...); }

[kbenzie]

include/llvm/ADT/Optional.h

120

This assignment operator does not take a const OptionalStorage& so y.hasVal is not a valid expression since T is the type of the object to be copied into storage.buffer e.g. for OptionalStorage<int>
then T is int.

A previous attempt to fix the GCC miscompiles used memmove which was only reported to fix GCC 5.4 release builds.

The expression *reinterpret_cast<T *>(storage.buffer) = y is undefined behaviour in the case where a valid object is not held in storage.buffer according to C++ rules which lead to this implementation.

[rsmith]

include/llvm/ADT/Optional.h

113

When the type is *actually* trivially-copyable (and not just isPodLike, which lies), we could just store it directly: union { T storage; };

122

This would be more correct as:

#ifdef __cpp_lib_launder
*std::launder(reinterpret_cast<T *>(&storage.buffer)) = y;
#else
*reinterpret_cast<T *>(std::addressof(storage.buffer)) = y;
#endif

(I'd expect the std::launder to also shut up the TBAA warnings.)

include/llvm/ADT/Optional.h

T *getPointer() {
     return reinterpret_cast<T *>(storage.buffer);
   }
   const T *getPointer() const {
     assert(hasVal);
     return reinterpret_cast<const T *>(storage.buffer);
   }
 };
 
-#if !defined(__GNUC__) || defined(__clang__) // GCC up to GCC7 miscompiles this.
 /// Storage for trivially copyable types only.
 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 &operator=(const T &y) {
-    *reinterpret_cast<T *>(storage.buffer) = y;
+    if (hasVal) {
+      // Use std::addressof to silence strict-aliasing warnings from gcc.
+      *reinterpret_cast<T *>(std::addressof(storage.buffer)) = y;
+    } else {
+      // Copy assignment to uninitialized storage is undefined behavior so copy
+      // construct with placement new when no value is held.
+      new (storage.buffer) T(y);
       hasVal = true;
+    }
     return *this;
   }
 
   void reset() { hasVal = false; }
 };
-#endif
 } // namespace optional_detail
 
 template <typename T> class Optional {
   optional_detail::OptionalStorage<T, isPodLike<T>::value> Storage;
 
 public:
   using value_type = T;