Use PointerIntPair in llvm::Error instead of hand-rolling it

[jordan_rose]

Summary

Also, tweak the application of LLVM_ENABLE_ABI_BREAKING_CHECKS so that a stray setChecked(false) call won't actually break ABI. And make sure the death tests don't run under NDEBUG.

[lhames]

include/llvm/Support/Error.h

290–292

Where does the stray 'setChecked(false)' happen?

I think I took the guards off this to make sure that Errors generated and checked in release mode code would not appear as "unchecked" to an app built in debug mode.

Sounds like you are hitting the opposite issue?

[jordan_rose]

include/llvm/Support/Error.h

290–292

Oh, this wasn't actually necessary; it was just in case someone actually did use setChecked(false). Since getPtr unconditionally does the masking, you'd never actually be able to tell anyway, so I figured it was better to have it optimize away. I'm happy to put it back.

include/llvm/Support/Error.h

 //===- llvm/Support/Error.h - Recoverable error handling --------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines an API used to report recoverable errors.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_ERROR_H
 #define LLVM_SUPPORT_ERROR_H
 
+#include "llvm/ADT/PointerIntPair.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Config/abi-breaking.h"
 #include "llvm/Support/AlignOf.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Debug.h"
#endif
   void assertIsChecked() {
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
     if (LLVM_UNLIKELY(!getChecked() || getPtr()))
       fatalUncheckedError();
 #endif
   }
 
   ErrorInfoBase *getPtr() const {
-    return reinterpret_cast<ErrorInfoBase*>(
-             reinterpret_cast<uintptr_t>(Payload) &
-             ~static_cast<uintptr_t>(0x1));
+    return Payload.getPointer();
   }
 
   void setPtr(ErrorInfoBase *EI) {
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
-    Payload = reinterpret_cast<ErrorInfoBase*>(
-                (reinterpret_cast<uintptr_t>(EI) &
-                 ~static_cast<uintptr_t>(0x1)) |
-                (reinterpret_cast<uintptr_t>(Payload) & 0x1));
+    Payload.setPointer(EI);
 #else
-    Payload = EI;
+    // This #else isn't strictly necessary, but should be more optimizable.
+    Payload.setPointerAndInt(EI, false);
 #endif
   }
 
   bool getChecked() const {
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
-    return (reinterpret_cast<uintptr_t>(Payload) & 0x1) == 0;
+    return !Payload.getInt();
 #else
+    // This #else isn't strictly necessary, but should be more optimizable.
     return true;
 #endif
   }
 
   void setChecked(bool V) {
-    Payload = reinterpret_cast<ErrorInfoBase*>(
-                (reinterpret_cast<uintptr_t>(Payload) &
-                  ~static_cast<uintptr_t>(0x1)) |
-                  (V ? 0 : 1));
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+    Payload.setInt(!V);
+#endif
   }
 
   std::unique_ptr<ErrorInfoBase> takePayload() {
     std::unique_ptr<ErrorInfoBase> Tmp(getPtr());
     setPtr(nullptr);
     setChecked(true);
     return Tmp;
   }
 
-  ErrorInfoBase *Payload = nullptr;
+  PointerIntPair<ErrorInfoBase *, 1, bool> Payload;
 };
 
 /// Subclass of Error for the sole purpose of identifying the success path in
 /// the type system. This allows to catch invalid conversion to Expected<T> at
 /// compile time.
 class ErrorSuccess : public Error {};
 
 inline ErrorSuccess Error::success() { return ErrorSuccess(); }

unittests/Support/ErrorTest.cpp

 
 TEST(Error, ConsumeError) {
   Error E = make_error<CustomError>(7);
   consumeError(std::move(E));
 }
 
 // Test that handleAllUnhandledErrors crashes if an error is not caught.
 // Test runs in debug mode only.
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS && !NDEBUG
 TEST(Error, FailureToHandle) {
   auto FailToHandle = []() {
     handleAllErrors(make_error<CustomError>(7), [&](const CustomSubError &SE) {
       errs() << "This should never be called";
       exit(1);
     });
   };
 
   EXPECT_DEATH(FailToHandle(),
                "Failure value returned from cantFail wrapped call")
       << "Unhandled Error in handleAllErrors call did not cause an "
          "abort()";
 }
 #endif
 
 // Test that handleAllUnhandledErrors crashes if an error is returned from a
 // handler.
 // Test runs in debug mode only.
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS && !NDEBUG
 TEST(Error, FailureFromHandler) {
   auto ReturnErrorFromHandler = []() {
     handleAllErrors(make_error<CustomError>(7),
                     [&](std::unique_ptr<CustomSubError> SE) {
                       return Error(std::move(SE));
                     });
   };