Add Status -- llvm::Error glue

[labath]

Summary

I tried convert a function to llvm::Error/Expected but I quickly ran into the
issue of interfacing with code that still expects the Status objects.

This is my proposal for conversion functions between the two. I use
constructors and conversion operators on the Status class for the job,
but I make them explicit to make sure they're not converted
accidentally, as they have different semantics. I made sure conversion
preserves the errno error codes (by converting them to std::error_code,
which llvm::Error understands) as we have bits of code which switch
based on the error value. As for the other errors, only the error
message is preserved during the conversion.

[zturner]

include/lldb/Utility/Status.h

108

I think we should remove the conversion operator. Instead, why don't we make a class called LLDBError and then just have people say:

Status S = getSomeStatus();
return make_error<LLDBError>(S);

source/Utility/Status.cpp

81–88

Delete in favor of an LLDBError class as mentioned before.

[zturner]

source/Utility/Status.cpp

81–88

Actually this doesn't really work, because you don't want to call make_error<LLDBError>(S) if S is a success condition.

So perhaps instead, I would at least call it something more explicit. llvm::Error toError() const perhaps.

[jingham]

This seems fine to me.

Zachary didn't give reasons why he didn't like the conversion form so I can't really assess that point. The use in the ErrorConversion test case seems pretty natural to me, but it's possible I'm missing whatever was bugging him about this. So I'll okay this for my part, but if you guys want to continue discussing, have at it!

[zturner]

Mostly just that implicit conversion operators are a very subtle source of bugs, and you can't even find where they're being used because it's impossible to grep for it.

If you're signing up to get an object that has strict usage semantics like llvm::Error, you had better be explicit about it.

[labath]

In D33241#756921, @zturner wrote:

Mostly just that implicit conversion operators are a very subtle source of bugs, and you can't even find where they're being used because it's impossible to grep for it.

I agree, and that's why I made the conversions explicit. For example Error foo() { return Status(); } (or vice-versa) will fail to compile without an explicit cast. So you should always see that a conversion is happening as the other type in the general vicinity of the conversion. However you will not be able to easily grep for the occurences of the conversion, as you would be in the toError case.

I did also have my doubts about whether this would be explicit enough though, so I would not be opposed to going to the separate function you propose. Do you want to beef up the explicitness on both directions of the conversion (e.g. Status s = Status::ConsumeError(std::move(error))) or just the one?

source/Utility/Status.cpp

81–88

Besides the success case issue, I believe having a member function (be it a conversion operator or not) allows us to have better interoperability between the two. I think of Status as being at the same level as llvm::Error, as it also tries to multiplex multiple error kinds into a single type, whereas having LLDBError introduces a subordinate relationship. Having a member function allows us to represent errno errors the same way as llvm does it.

Instead of an LLDBError I'd propose we create more specialized error classes (UnwindError? NetworkError?) once we actually identify use cases for them. Although we can have LLDBError as a common superclass of these errors if you think it will be useful.

[labath]

use a separate function instead of a conversion operator

[zturner]

source/Utility/Status.cpp

81–88

Yes, eventually we will definitely want to have different error implementations. CommandArgumentError, RemoteCommunicationError, DebugInfoError, etc all come to mind as potential candidates.

include/lldb/Utility/Status.h

-//===-- Status.h -------------------------------------------------*- C++
-//-*-===//
+//===-- Status.h ------------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef __DCError_h__
-#define __DCError_h__
-#if defined(__cplusplus)
+#ifndef LLDB_UTILITY_STATUS_H
+#define LLDB_UTILITY_STATUS_H
 
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h" // for ErrorType, ErrorType...
 #include "llvm/ADT/StringRef.h"     // for StringRef
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
-
 #include <cstdarg>
+#include <stdint.h> // for uint32_t
 #include <string>
 #include <system_error> // for error_code
 #include <type_traits>  // for forward
 
-#include <stdint.h> // for uint32_t
-
 namespace llvm {
 class raw_ostream;
 }
 
 namespace lldb_private {
 
 //----------------------------------------------------------------------
 /// @class Status Status.h "lldb/Utility/Status.h"
public:
   ///
   /// @return
   ///     A const reference to this object.
   //------------------------------------------------------------------
   const Status &operator=(uint32_t err);
 
   ~Status();
 
+  // llvm::Error support
+  explicit Status(llvm::Error error);
+  llvm::Error ToError() const;
+
   //------------------------------------------------------------------
   /// Get the error string associated with the current error.
   //
   /// Gets the error value as a NULL terminated C string. The error
   /// string will be fetched and cached on demand. The error string
   /// will be retrieved from a callback that is appropriate for the
   /// type of the error and will be cached until the error value is
   /// changed or cleared.

 
 namespace llvm {
 template <> struct format_provider<lldb_private::Status> {
   static void format(const lldb_private::Status &error, llvm::raw_ostream &OS,
                      llvm::StringRef Options);
 };
 }
 
-#endif // #if defined(__cplusplus)
-#endif // #ifndef __DCError_h__
+#endif // #ifndef LLDB_UTILITY_STATUS_H

source/Utility/Status.cpp

Status::Status(const char *format, ...)
     : m_code(0), m_type(eErrorTypeInvalid), m_string() {
   va_list args;
   va_start(args, format);
   SetErrorToGenericError();
   SetErrorStringWithVarArg(format, args);
   va_end(args);
 }
 
+Status::Status(llvm::Error error)
+    : m_code(0), m_type(ErrorType::eErrorTypeGeneric) {
+  if (!error)
+    return;
+
+  // if the error happens to be a errno error, preserve the error code
+  error = llvm::handleErrors(
+      std::move(error), [&](std::unique_ptr<llvm::ECError> e) -> llvm::Error {
+        std::error_code ec = e->convertToErrorCode();
+        if (ec.category() == std::generic_category()) {
+          m_code = ec.value();
+          m_type = ErrorType::eErrorTypePOSIX;
+          return llvm::Error::success();
+        }
+        return llvm::Error(std::move(e));
+      });
+
+  // Otherwise, just preserve the message
+  if (error)
+    SetErrorString(llvm::toString(std::move(error)));
+}
+
+llvm::Error Status::ToError() const {
+  if (Success())
+    return llvm::Error::success();
+  if (m_type == ErrorType::eErrorTypePOSIX)
+    return llvm::errorCodeToError(std::error_code(m_code, std::generic_category()));
+  return llvm::make_error<llvm::StringError>(AsCString(),
+                                             llvm::inconvertibleErrorCode());
+}
+
 //----------------------------------------------------------------------
 // Assignment operator
 //----------------------------------------------------------------------
 const Status &Status::operator=(const Status &rhs) {
   if (this != &rhs) {
     m_code = rhs.m_code;
     m_type = rhs.m_type;
     m_string = rhs.m_string;

unittests/Utility/StatusTest.cpp

 //===-- StatusTest.cpp ------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Utility/Status.h"
 #include "gtest/gtest.h"
 
 using namespace lldb_private;
+using namespace lldb;
 
 TEST(StatusTest, Formatv) {
   EXPECT_EQ("", llvm::formatv("{0}", Status()).str());
   EXPECT_EQ("Hello Status", llvm::formatv("{0}", Status("Hello Status")).str());
   EXPECT_EQ("Hello", llvm::formatv("{0:5}", Status("Hello Error")).str());
 }
+
+TEST(StatusTest, ErrorConstructor) {
+  EXPECT_TRUE(Status(llvm::Error::success()).Success());
+
+  Status eagain(
+      llvm::errorCodeToError(std::error_code(EAGAIN, std::generic_category())));
+  EXPECT_TRUE(eagain.Fail());
+  EXPECT_EQ(eErrorTypePOSIX, eagain.GetType());
+  EXPECT_EQ(Status::ValueType(EAGAIN), eagain.GetError());
+
+  Status foo(llvm::make_error<llvm::StringError>(
+      "foo", llvm::inconvertibleErrorCode()));
+  EXPECT_TRUE(foo.Fail());
+  EXPECT_EQ(eErrorTypeGeneric, foo.GetType());
+  EXPECT_STREQ("foo", foo.AsCString());
+}
+
+TEST(StatusTest, ErrorConversion) {
+  EXPECT_FALSE(bool(Status().ToError()));
+
+  llvm::Error eagain = Status(EAGAIN, ErrorType::eErrorTypePOSIX).ToError();
+  EXPECT_TRUE(bool(eagain));
+  std::error_code ec = llvm::errorToErrorCode(std::move(eagain));
+  EXPECT_EQ(EAGAIN, ec.value());
+  EXPECT_EQ(std::generic_category(), ec.category());
+
+  llvm::Error foo = Status("foo").ToError();
+  EXPECT_TRUE(bool(foo));
+  EXPECT_EQ("foo", llvm::toString(std::move(foo)));
+}