From a9682caf96198508e3601e1e8160aa5505c3f266 Mon Sep 17 00:00:00 2001
From: Lang Hames <lhames@gmail.com>
Date: Fri, 28 Oct 2016 18:24:15 +0000
Subject: [PATCH] [Error] Unify +Asserts/-Asserts behavior for checked flags in
 Error/Expected<T>.

(1) Switches to raw pointer and bitmasking operations for Error payload.
(2) Always includes the 'unchecked' bitfield in Expected<T>, even in -Asserts.
(3) Always propagates checked bit status in move-ops for both classes, even in
    -Asserts.

This should allow debug programs to link against release libraries without
encountering spurious 'unchecked error' terminations.

Error checks still aren't verified in release mode so this doesn't introduce
any new control flow, but it does require new bit-masking ops in release mode
to preserve the flag values during move ops. I expect the overhead to be
minimal, but if we discover any corner cases where it matters we could fix
this by making flag propagation conditional on a new build option.

llvm-svn: 285426
---
 llvm/include/llvm/Support/Error.h | 70 +++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 33 deletions(-)

diff --git a/llvm/include/llvm/Support/Error.h b/llvm/include/llvm/Support/Error.h
index d228a0f..0f9b326 100644
--- a/llvm/include/llvm/Support/Error.h
+++ b/llvm/include/llvm/Support/Error.h
@@ -152,7 +152,7 @@ class LLVM_NODISCARD Error {
 public:
   /// Create a success value. Prefer using 'Error::success()' for readability
   /// where possible.
-  Error() {
+  Error() : Payload(nullptr) {
     setPtr(nullptr);
     setChecked(false);
   }
@@ -167,7 +167,7 @@ public:
   /// Move-construct an error value. The newly constructed error is considered
   /// unchecked, even if the source error had been checked. The original error
   /// becomes a checked Success value, regardless of its original state.
-  Error(Error &&Other) {
+  Error(Error &&Other) : Payload(nullptr) {
     setChecked(true);
     *this = std::move(Other);
   }
@@ -238,16 +238,17 @@ private:
   }
 
   ErrorInfoBase *getPtr() const {
-#ifndef NDEBUG
-    return PayloadAndCheckedBit.getPointer();
-#else
-    return Payload;
-#endif
+    return reinterpret_cast<ErrorInfoBase*>(
+             reinterpret_cast<uintptr_t>(Payload) &
+             ~static_cast<uintptr_t>(0x1));
   }
 
   void setPtr(ErrorInfoBase *EI) {
 #ifndef NDEBUG
-    PayloadAndCheckedBit.setPointer(EI);
+    Payload = reinterpret_cast<ErrorInfoBase*>(
+                (reinterpret_cast<uintptr_t>(EI) &
+                 ~static_cast<uintptr_t>(0x1)) |
+                (reinterpret_cast<uintptr_t>(Payload) & 0x1));
 #else
     Payload = EI;
 #endif
@@ -255,16 +256,17 @@ private:
 
   bool getChecked() const {
 #ifndef NDEBUG
-    return PayloadAndCheckedBit.getInt();
+    return (reinterpret_cast<uintptr_t>(Payload) & 0x1) == 0;
 #else
     return true;
 #endif
   }
 
   void setChecked(bool V) {
-#ifndef NDEBUG
-    PayloadAndCheckedBit.setInt(V);
-#endif
+    Payload = reinterpret_cast<ErrorInfoBase*>(
+                (reinterpret_cast<uintptr_t>(Payload) &
+                  ~static_cast<uintptr_t>(0x1)) |
+                  (V ? 0 : 1));
   }
 
   std::unique_ptr<ErrorInfoBase> takePayload() {
@@ -274,11 +276,7 @@ private:
     return Tmp;
   }
 
-#ifndef NDEBUG
-  PointerIntPair<ErrorInfoBase *, 1> PayloadAndCheckedBit;
-#else
   ErrorInfoBase *Payload;
-#endif
 };
 
 /// Make a Error instance representing failure using the given error info
@@ -631,11 +629,17 @@ private:
 public:
   /// Create an Expected<T> error value from the given Error.
   Expected(Error Err)
-      : HasError(true)
+      : HasError(true),
 #ifndef NDEBUG
-        ,
-        Checked(false)
+        // Expected is unchecked upon construction in Debug builds.
+        Unchecked(true)
+#else
+        // Expected's unchecked flag is set to false in Release builds. This
+        // allows Expected values constructed in a Release build library to be
+        // consumed by a Debug build application.
+        Unchecked(false)
 #endif
+
   {
     assert(Err && "Cannot create Expected<T> from Error success value.");
     new (getErrorStorage()) Error(std::move(Err));
@@ -647,10 +651,15 @@ public:
   Expected(OtherT &&Val,
            typename std::enable_if<std::is_convertible<OtherT, T>::value>::type
                * = nullptr)
-      : HasError(false)
+      : HasError(false),
 #ifndef NDEBUG
-        ,
-        Checked(false)
+        // Expected is unchecked upon construction in Debug builds.
+        Unchecked(true)
+#else
+        // Expected's 'unchecked' flag is set to false in Release builds. This
+        // allows Expected values constructed in a Release build library to be
+        // consumed by a Debug build application.
+        Unchecked(false)
 #endif
   {
     new (getStorage()) storage_type(std::forward<OtherT>(Val));
@@ -696,7 +705,7 @@ public:
   /// \brief Return false if there is an error.
   explicit operator bool() {
 #ifndef NDEBUG
-    Checked = !HasError;
+    Unchecked = HasError;
 #endif
     return !HasError;
   }
@@ -724,7 +733,7 @@ public:
   /// be made on the Expected<T> vaule.
   Error takeError() {
 #ifndef NDEBUG
-    Checked = true;
+    Unchecked = false;
 #endif
     return HasError ? Error(std::move(*getErrorStorage())) : Error::success();
   }
@@ -766,11 +775,8 @@ private:
 
   template <class OtherT> void moveConstruct(Expected<OtherT> &&Other) {
     HasError = Other.HasError;
-
-#ifndef NDEBUG
-    Checked = false;
-    Other.Checked = true;
-#endif
+    Unchecked = true;
+    Other.Unchecked = false;
 
     if (!HasError)
       new (getStorage()) storage_type(std::move(*Other.getStorage()));
@@ -813,7 +819,7 @@ private:
 
   void assertIsChecked() {
 #ifndef NDEBUG
-    if (!Checked) {
+    if (Unchecked) {
       dbgs() << "Expected<T> must be checked before access or destruction.\n";
       if (HasError) {
         dbgs() << "Unchecked Expected<T> contained error:\n";
@@ -832,9 +838,7 @@ private:
     AlignedCharArrayUnion<error_type> ErrorStorage;
   };
   bool HasError : 1;
-#ifndef NDEBUG
-  bool Checked : 1;
-#endif
+  bool Unchecked : 1;
 };
 
 /// This class wraps a std::error_code in a Error.
-- 
2.7.4