Fix SegFault in Expected

[yuyichao]

Summary

The placement new on getErrorStorage() is of the wrong type.

AFAICT this is the intent in https://reviews.llvm.org/rL265446 which somehow starts to trigger a segfault when the destructor of the unique_ptr is called after https://reviews.llvm.org/rL285426. Ref https://github.com/JuliaLang/julia/issues/19154

[yuyichao]

And I think it didn't cause any problem because Error and unique_ptr has the same layout except for the lowest bit. Before https://reviews.llvm.org/rL285426, the lowest bit should never be set when the Expected is checked/assertion is disabled but this is not true anymore for an unchecked Expected on a release build after the change.

[yuyichao]

@lhames ping.

[vsk]

(I'm just trying to wrap my head around this.)

It looks like getErrorStorage() provides a pointer to an error_type (std::unique_ptr<ErrorInfoBase>), but we're storing an Error (funky pointer-int pair) into it. Since this started causing problems after r285426, I'm wondering what the reproducer looks like / why we haven't seen this issue already. Would you need to look into an unchecked Error?, e.g:

auto E = Expected<int>(make_error<MyErrorInfo>(...));
ASSERT_DEATH(*E);

If there's a way to add a unit test to unittests/Support/ErrorTest without introducing a death test, that would be great.

[yuyichao]

Since this started causing problems after r285426, I'm wondering what the reproducer looks like / why we haven't seen this issue already.

Any code that doesn't check for the error when assertion is disabled should trigger this error and the reason was that r285426 introduces the tag bit in this case. The code path we had to trigger it is https://github.com/JuliaLang/julia/blob/10ffbaeaa809a5a9cdc7866aa659ad2a4e525a6a/src/debuginfo.cpp#L1255, which clears the error before assigning the next attempt to it if assertion is enabled.

[lhames]

This is great. Thanks Yichao!

I've committed it in r285970.

[yuyichao]

I assume the patch is actually merged even though somehow it doesn't trigger the hook that closes this?

Hopefully this is the right way to close a PR...

include/llvm/Support/Error.h

class LLVM_NODISCARD Error {
   friend class ErrorList;
 
   // handleErrors needs to be able to set the Checked flag.
   template <typename... HandlerTs>
   friend Error handleErrors(Error E, HandlerTs &&... Handlers);
 
   // Expected<T> needs to be able to steal the payload when constructed from an
   // error.
-  template <typename T> class Expected;
+  template <typename T> friend class Expected;
 
 public:
   /// Create a success value. Prefer using 'Error::success()' for readability
   /// where possible.
   Error() : Payload(nullptr) {
     setPtr(nullptr);
     setChecked(false);
   }
#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));
+    new (getErrorStorage()) error_type(Err.takePayload());
   }
 
   /// Create an Expected<T> success value from the given OtherT value, which
   /// must be convertible to T.
   template <typename OtherT>
   Expected(OtherT &&Val,
            typename std::enable_if<std::is_convertible<OtherT, T>::value>::type
                * = nullptr)