[PATCH] D50710: [ADT] Fix Optional ABI mismatch between clang & gcc

[Kenneth Benzie via Phabricator via llvm-commits]

kbenzie created this revision.
kbenzie added a reviewer: chandlerc.
Herald added subscribers: llvm-commits, dexonsmith.

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D50710

Files:
  include/llvm/ADT/Optional.h


Index: include/llvm/ADT/Optional.h
===================================================================
--- include/llvm/ADT/Optional.h
+++ include/llvm/ADT/Optional.h
@@ -108,7 +108,6 @@
   }
 };
 
-#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;
@@ -118,14 +117,20 @@
 
   OptionalStorage(const T &y) : hasVal(true) { new (storage.buffer) T(y); }
   OptionalStorage &operator=(const T &y) {
-    *reinterpret_cast<T *>(storage.buffer) = y;
-    hasVal = true;
+    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 {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D50710.160596.patch
Type: text/x-patch
Size: 1165 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180814/d8686c27/attachment.bin>