[StreamExecutor] Add error handling library

[jhen]

Summary

Error handling in StreamExecutor is based on llvm::Error and llvm::Expected. This CL sets up the StreamExecutor wrapper classes in the streamexecutor namespace.

All the other StreamExecutor code makes use of this error handling code, so this is the first CL for checking in StreamExecutor.

[jlebar]

streamexecutor/include/streamexecutor/Utils/Demangle.h

23 ↗

(On Diff #65093)

I would be pretty surprised if we didn't already have a function for this in llvm. I think you can call LLVMSymbolizer::DemangleName with a null ModInfo?

streamexecutor/include/streamexecutor/Utils/Error.h

1 ↗

(On Diff #65093)

We already have support/Error.h, which I think is our idiom, subsuming this, Status, and StatusOr. This is the first time I've looked at these facilities, so I don't know e.g. whether we want to use ErrorOr<> or Expected<>.

If we end up keeping this, it seems like we probably don't need many of these error codes? But we can cross that bridge.

streamexecutor/include/streamexecutor/Utils/Macros.h

24 ↗

(On Diff #65093)

Surely we have one of these in LLVM.

39 ↗

(On Diff #65093)

LLVM_ATTRIBUTE_UNUSED_RESULT

53 ↗

(On Diff #65093)

Hm. This is un-llvm-ish. I'd rather not introduce this idiom into LLVM, since nobody else is using it.

Is this used all over the place?

streamexecutor/include/streamexecutor/Utils/Mutex.h

50 ↗

(On Diff #65093)

LLVM has a Mutex class.

streamexecutor/include/streamexecutor/Utils/PtrUtil.h

60 ↗

(On Diff #65093)

This already exists in STLExtras.h.

streamexecutor/lib/Utils/StrUtil.cpp

1 ↗

(On Diff #65093)

I am not sure we need these utils.

StringRef has lower(), upper(), startswith(), and endswith(), and StringExtras.h has SplitString. The only one I'm not sure about is stripSuffixString. That one seems less general-purpose, but if we use it a lot, maybe we can move it to StringExtras.h?

[jhen]

• Fixes based on jlebar's suggestions

[jhen]

I followed most of jlebar's suggestions to use standard LLVM utilities and this patch is now much smaller.

streamexecutor/include/streamexecutor/Utils/Demangle.h

23 ↗

(On Diff #65093)

You're right, LLVMSymbolizer::DemangleName seems to work. So, I got rid of all the SE custom "Demangle" stuff.

streamexecutor/include/streamexecutor/Utils/Error.h

1 ↗

(On Diff #65093)

I did a lot of experiments and Expected<> looks like it does what we want, so I removed Status and StatusOr and replaced them with Error.h which wraps llvm::Error and llvm::Expected.

streamexecutor/include/streamexecutor/Utils/Macros.h

24 ↗

(On Diff #65093)

I searched but couldn't find one. I even grepped in include/llvm using grep -R '\<sizeof(.*)[[:space:]]*/[[:space:]]*sizeof(' and it only showed up some unrelated stuff in Support/MachO.h.

39 ↗

(On Diff #65093)

Now that Status has been deleted, this is not needed anymore anyway, so I got rid of it.

53 ↗

(On Diff #65093)

This isn't widely used and it's easy to replace. It's gone now.

streamexecutor/include/streamexecutor/Utils/Mutex.h

50 ↗

(On Diff #65093)

Cool, I got rid of this file and I'll switch to using LLVM's RWMutex when I want a shared_mutex.

streamexecutor/include/streamexecutor/Utils/PtrUtil.h

60 ↗

(On Diff #65093)

Great! I switched to using the STLExtras version and removed this.

streamexecutor/lib/Utils/StrUtil.cpp

1 ↗

(On Diff #65093)

You're right, they're not needed. I switch to using the StringRef stuff now and removed StrUtil.

[chandlerc]

(somewhat of a drive-by comment below, sorry for that, but just wanted to try and push comments and documentation in a small way)

streamexecutor/include/streamexecutor/Utils/Error.h

25–27 ↗

(On Diff #65848)

If I were a naive reader, I might not understand the context that leads you to specifically pull these types into the streamexecutor namespace.

I think it would be really useful to expand the comments in the code to include more rationale behind the design the code is following. As an example, the file comment above might talk about the fact that these aren't just internal utilities of streamexecutor, but types that will in some cases appear on the user-facing side of the streamexecutor APIs, and thus form some of the building blocks for streamexecutor's user-facing API. As a consequence, while they are implemented in terms of the LLVM primitives, they are specialized and provided under that namespace to make things more clear for users, blah blalh blah.

Anyways, a very meta comment, and not even necessarily something you want to address in this patch vs. other patches, and probably this is just one place where it might be relevant.

[jlebar]

streamexecutor/include/streamexecutor/Utils/Error.h

30 ↗

(On Diff #65848)

It looks like most subclasses are named FooError, rather than FooErrorInfo. Maybe SEError?

32 ↗

(On Diff #65848)

Nit, if you take the string by value, probably better to std::move it. But better still would be to take an llvm::StringRef, I think, and stringify it here. Then we can create errors from Twines or whatever.

38 ↗

(On Diff #65848)

Looking through existing code, this is usually called getErrorMessage.

45 ↗

(On Diff #65848)

nit, capital E

50 ↗

(On Diff #65848)

Given that there's only one constructor, I'm not sure we need a full-blown forwarding constructor.

But would it be so bad to just do "return make_error<SEError>("foo")" everywhere? That is, do we need this wrapper? "Explicit is better than implicit."

58 ↗

(On Diff #65848)

Should probably note that Error must be success or an SEError.

Kind of weird that we have no type-safety...

59 ↗

(On Diff #65848)

This seems like a strange name, because we're not really consuming the message. consumeAndGetMessage? Or is that too verbose?

79 ↗

(On Diff #65848)

What if we didn't have this, and all callsites did

consumeAndGetMessage(Exp.takeError());

AIUI you don't need the if (Exp) branch because takeError returns Error::success() if there's no error.

To me that would be more explicit, and it's not a ton more complicated. But you have the context about the rest of the code.

94 ↗

(On Diff #65848)

Hm, why would we sometimes want to std::move the error and sometimes not? You don't want to std::move all the time because then we defeat rvo (and, I suppose, potentially get a warning about a pessimizing move)?

Do we really need these three macros? On other projects I've worked on, they are widely viewed as a mistake, and in particular the distinction between the first two is mega-confusing. And the precedent in llvm, such as it is, is to do without. Maybe we can sit down and see whether the code becomes really ugly without them?

streamexecutor/include/streamexecutor/Utils/Macros.h

25 ↗

(On Diff #65848)

Found it: STLExtras.h array_lengthof.

40 ↗

(On Diff #65848)

Maybe you forgot to update this file?

54 ↗

(On Diff #65848)

Same for this one.

streamexecutor/include/streamexecutor/Utils/ThreadAnnotations.h

28 ↗

(On Diff #65848)

Would rather not pollute the global namespace with SE_THREAD_ANNOTATION_ATTRIBUTE if we can help it. (That is, would rather declare this with one macro rather than two.)

But this also seems like something that would be better in e.g. mutex.h.

[jhen]

• More changes from jlebar and chandlerc

streamexecutor/include/streamexecutor/Utils/Error.h

25–27 ↗

(On Diff #65848)

Great point. I've added a ton of comments now.

50 ↗

(On Diff #65848)

I've refactored so that "SEError" is not present in the header. This makes my original intention clear, that the user should not know about StreamExecutor's custom llvm::ErrorInfo class. I've retained the make_error function, but now it's not templated, but instead just takes a StringRef.

58 ↗

(On Diff #65848)

I've added a few comments that talk about how only the public functions should be used to deal with these types. It is weird that there's no type safety, but if people stick to the public interface in namespace streamexecutor, I don't think they can go wrong.

59 ↗

(On Diff #65848)

It's not too verbose. I changed the name as you suggested.

94 ↗

(On Diff #65848)

We don't really need these macros. They just came from emulating Google internal StreamExecutor. I have removed them now.

I'll say why I had move and non-move versions here, but feel free to skip as they are gone now. The move version was for functions that returned Expected<T>. Expected<T> has a constructor that takes an rvalue ref to Error, so we would have to return a moved temporary error in order to be able to call that constructor.

streamexecutor/include/streamexecutor/Utils/Macros.h

25 ↗

(On Diff #65848)

Awesome! Thanks. Now I can get rid of this whole macros file.

40 ↗

(On Diff #65848)

Yep, I just forgot to update it. The whole file should be gone now.

streamexecutor/include/streamexecutor/Utils/ThreadAnnotations.h

28 ↗

(On Diff #65848)

Consolidated to one macro and moved to Mutex.h.

[jlebar]

I'll look over the changes to error.h after I get some food (and finally make my way into the office), but here are a few quick things.

streamexecutor/CMakeLists.txt

50 ↗

(On Diff #65848)

FWIW I have studiously avoided learning CMake. If this isn't just copy/pasted from somewhere, maybe Art can sign off on it. But if you're happy with it, that's good enough for me.

streamexecutor/include/streamexecutor/Utils/ThreadAnnotations.h

28 ↗

(On Diff #65848)

Ah, sorry, I meant, it seems that this belongs in LLVM's mutex.h (as a separate patch).

[jlebar]

streamexecutor/include/streamexecutor/Utils/Error.h

27 ↗

(On Diff #65967)

Probably worth capitalizing "Error" when we're talking about the class.

31 ↗

(On Diff #65967)

from an Error object

78 ↗

(On Diff #65967)

Nit, ", where"

82 ↗

(On Diff #65967)

suggest s/the * pointer dereference operator/operator*/. Similarly below for ->

121 ↗

(On Diff #65967)

Maybe we should say

"You can also write the same code if doTask3() returns an Expected<U> instead of an Error." One less example, and it calls out the equivalence of the code rather than asking the reader to figure out that it's the same.

On another note, could I do

return ExpectedInt;

instead of

return ExpectedInt.takeError();

I think so based on the rules above. Not sure if that's worth mentioning, or if we have a preference between these two ways of doing it.

180 ↗

(On Diff #65967)

I wonder if we need this one. It may encourage people to do string comparisons to check whether a function returned success, and that's kind of lame.

What do you think, based on the code you haven't released yet?

streamexecutor/lib/Utils/Error.cpp

17 ↗

(On Diff #65967)

Why <> instead of ""? Same in the header. llvm and clang don't contain the string "#include <llvm".

25 ↗

(On Diff #65967)

Nit, suggest Message.str() -- a bit simpler.

[jhen]

• More comments from jlebar

streamexecutor/include/streamexecutor/Utils/Error.h

121 ↗

(On Diff #65967)

I combined the task3 and task4 examples to show a few different ways of passing errors up the stack.

180 ↗

(On Diff #65967)

I'm fine to get rid of it.

[jlebar]

This looks great to me except it looks like the changes to mutex.h are still there.

[jhen]

• Remove mutex stuff

[jhen]

Ah yes, the mutex stuff. It turns out there is a whole story to deal with there. I'll do it in another CL.

[jlebar]

lgtm!

parallel-libs/trunk/streamexecutor/CMakeLists.txt

+cmake_minimum_required(VERSION 3.1)
+
+option(STREAM_EXECUTOR_UNIT_TESTS "enable unit tests" ON)
+
+# First find includes relative to the streamexecutor top-level source path.
+include_directories(BEFORE ${CMAKE_CURRENT_SOURCE_DIR}/include)
+
+# If we are not building as part of LLVM, build StreamExecutor as a standalone
+# project using LLVM as an external library:
+string(
+    COMPARE
+    EQUAL
+    "${CMAKE_SOURCE_DIR}"
+    "${CMAKE_CURRENT_SOURCE_DIR}"
+    STREAM_EXECUTOR_STANDALONE)
+
+if(STREAM_EXECUTOR_STANDALONE)
+    project(StreamExecutor)
+
+    find_package(LLVM REQUIRED CONFIG)
+    message(STATUS "Found LLVM ${LLVM_PACKAGE_VERSION}")
+    message(STATUS "Using LLVMConfig.cmake in: ${LLVM_DIR}")
+
+    include_directories(${LLVM_INCLUDE_DIRS})
+    add_definitions(${LLVM_DEFINITIONS})
+
+    # Find the libraries that correspond to the LLVM components
+    # that we wish to use
+    llvm_map_components_to_libnames(llvm_libs support)
+
+    if(STREAM_EXECUTOR_UNIT_TESTS)
+        enable_testing()
+        find_package(GTest REQUIRED)
+        include_directories(${GTEST_INCLUDE_DIRS})
+    endif()
+else(NOT STREAM_EXECUTOR_STANDALONE)
+    if(STREAM_EXECUTOR_UNIT_TESTS)
+        include_directories(
+            "${LLVM_MAIN_SRC_DIR}/utils/unittest/googletest/include")
+    endif()
+endif(STREAM_EXECUTOR_STANDALONE)
+
+# Insist on C++ 11 features.
+set(CMAKE_CXX_STANDARD 11)
+set(CMAKE_CXX_STANDARD_REQUIRED ON)
+
+# Add warning flags.
+set(CMAKE_CXX_FLAGS  "${CMAKE_CXX_FLAGS} -Wall -Wextra -Wno-unused-parameter")
+
+add_subdirectory(lib)

parallel-libs/trunk/streamexecutor/README.txt

-This directory will contain the StreamExecutor library.
+StreamExecutor
+==============
+
+StreamExecutor is a wrapper around CUDA and OpenCL (host-side) programming
+models (runtimes). This abstraction cleanly permits host code to target either
+CUDA or OpenCL devices with identically-functioning data parallel kernels. It
+manages the execution of concurrent work targeting the accelerator, similar to a
+host-side Executor.
+
+This version of StreamExecutor can be built either as a sub-project of the LLVM
+project or as a standalone project depending on LLVM as an external package.

parallel-libs/trunk/streamexecutor/include/streamexecutor/Utils/Error.h

+//===-- Error.h - Error handling --------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// Error types used in the public API and internally in StreamExecutor.
+///
+/// StreamExecutor's error handling is based on the types streamexecutor::Error
+/// and streamexecutor::Expected<T>.
+///
+///
+/// \section error The Error Class
+///
+/// The Error class either represents success or contains an error message
+/// describing the cause of the error. Error instances are created by calling
+/// Error::success for successes or make_error for errors.
+///
+/// \code{.cpp}
+/// Error achieveWorldPeace() {
+///   if (WorldPeaceAlreadyAchieved) {
+///     return Error::success();
+///   } else {
+///     return make_error("Can't someone else do it?");
+///   }
+/// }
+/// \endcode
+///
+/// Error instances are implicitly convertable to bool. Error values convert to
+/// true and successes convert to false. Error instances must have their boolean
+/// values checked or they must be moved before they go out of scope, otherwise
+/// their destruction will cause the program to abort with a warning about an
+/// unchecked Error.
+///
+/// If the Error represents success, then checking the boolean value is all that
+/// is required, but if the Error represents a real error, the Error value must
+/// be consumed. The function consumeAndGetMessage is the way to extract the
+/// error message from an Error and consume the Error at the same time, so
+/// typical error handling will first check whether there was an error and then
+/// extract the error message if so. Here is an example:
+///
+/// \code{.cpp}
+/// if (Error E = achieveWorldPeace()) {
+///   printf("An error occurred: %s\n", consumeAndGetMessage(E).c_str());
+///   exit(EXIT_FAILURE):
+/// }
+/// \endcode
+///
+/// It is also common to simply pass an error along up the call stack if it
+/// cannot be handled in the current function.
+///
+/// \code{.cpp}
+/// Error doTask() {
+///   if (Error E = achieveWorldPeace()) {
+///     return E;
+///   }
+///   ...
+/// }
+/// \endcode
+///
+/// There is also a function consumeError that consumes an error value without
+/// fetching the error message. This is useful when we want to ignore an error.
+///
+///
+/// \section expected The Expected Class
+///
+/// The Expected<T> class either represents a value of type T or an Error.
+/// Expected<T> has one constructor that takes a T value and another constructor
+/// that takes an Error rvalue reference, so Expected instances can be
+/// constructed either from values or from errors:
+///
+/// \code{.cpp}
+/// Expected<int> getMyFavoriteInt() {
+///   int MyFavorite = 42;
+///   if (IsThereAFavorite) {
+///     return MyFavorite;
+///   } else {
+///     return make_error("I don't have a favorite");
+///   }
+/// }
+/// \endcode
+///
+/// Expected<T> instances are implicitly convertible to bool and are true if
+/// they contain a value and false if they contain an error. Note that this is
+/// the opposite convention of the Error type conversion to bool, where true
+/// meant error and false meant success.
+///
+/// If the Expected<T> instance is not an error, the stored value can be
+/// obtained by using operator*. If access to members of the value are desired
+/// instead of the value itself, operator-> can be used as well.
+///
+/// Expected<T> instances must have their boolean value checked or they must be
+/// moved before they go out of scope, otherwise they will cause the program to
+/// abort with a warning about an unchecked error. If the Expected<T> instance
+/// contains a value, then checking the boolean value is all that is required,
+/// but if it contains an Error object, that Error object must be handled by
+/// calling Expected<T>::takeError() to get the underlying error.
+///
+/// Here is an example of the use of an Expected<T> value returned from a
+/// function:
+///
+/// \code{.cpp}
+/// Expected<int> ExpectedInt = getMyFavoriteInt();
+/// if (ExpectedInt) {
+///   printf("My favorite integer is %d\n", *ExpectedInt);
+/// } else {
+///   printf("An error occurred: %s\n",
+///     consumeAndGetMessage(ExpectedInt.takeError()));
+///   exit(EXIT_FAILURE);
+/// }
+/// \endcode
+///
+/// The following snippet shows some examples of how Errors and Expected values
+/// can be passed up the stack if they should not be handled in the current
+/// function.
+///
+/// \code{.cpp}
+/// Expected<double> doTask3() {
+///   Error WorldPeaceError = achieveWorldPeace();
+///   if (!WorldPeaceError) {
+///     return WorldPeaceError;
+///   }
+///
+///   Expected<martian> ExpectedMartian = getMyFavoriteMartian();
+///   if (!ExpectedMartian) {
+///     // Must extract the error because martian cannot be converted to double.
+///     return ExpectedMartian.takeError():
+///   }
+///
+///   // It's fine to return Expected<int> for Expected<double> because int can
+///   // be converted to double.
+///   return getMyFavoriteInt();
+/// }
+/// \endcode
+///
+///
+/// \section llvm Relation to llvm::Error and llvm::Expected
+///
+/// The streamexecutor::Error and streamexecutor::Expected classes are actually
+/// just their LLVM counterparts redeclared in the streamexectuor namespace, but
+/// they should be treated as separate types, even so.
+///
+/// StreamExecutor does not support any underlying llvm::ErrorInfo class except
+/// the one it defines internally for itself, so a streamexecutor::Error can be
+/// thought of as a restricted llvm::Error that is guaranteed to hold a specific
+/// error type.
+///
+/// Although code may compile if llvm functions used to handle these
+/// StreamExecutor error types, it is likely that code will lead to runtime
+/// errors, so it is strongly recommended that only the functions from the
+/// streamexecutor namespace are used on these StreamExecutor error types.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef STREAMEXECUTOR_UTILS_ERROR_H
+#define STREAMEXECUTOR_UTILS_ERROR_H
+
+#include <memory>
+#include <string>
+
+#include "llvm/Support/Error.h"
+
+namespace streamexecutor {
+
+using llvm::consumeError;
+using llvm::Error;
+using llvm::Expected;
+using llvm::StringRef;
+
+// Makes an Error object from an error message.
+Error make_error(StringRef Message);
+
+// Consumes the input error and returns its error message.
+//
+// Assumes the input was created by the make_error function above.
+std::string consumeAndGetMessage(Error &&E);
+
+} // namespace streamexecutor
+
+#endif // STREAMEXECUTOR_UTILS_ERROR_H

parallel-libs/trunk/streamexecutor/lib/CMakeLists.txt

+add_library(
+    utils
+    OBJECT
+    Utils/Error.cpp)

parallel-libs/trunk/streamexecutor/lib/Utils/Error.cpp

+//===-- Error.cpp - Error handling ----------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// Types for returning recoverable errors.
+///
+//===----------------------------------------------------------------------===//
+
+#include "streamexecutor/Utils/Error.h"
+
+#include "llvm/ADT/StringRef.h"
+
+namespace {
+
+// An error with a string message describing the cause.
+class StreamExecutorError : public llvm::ErrorInfo<StreamExecutorError> {
+public:
+  StreamExecutorError(llvm::StringRef Message) : Message(Message.str()) {}
+
+  void log(llvm::raw_ostream &OS) const override { OS << Message; }
+
+  std::error_code convertToErrorCode() const override {
+    llvm_unreachable(
+        "StreamExecutorError does not support convertion to std::error_code");
+  }
+
+  std::string getErrorMessage() const { return Message; }
+
+  static char ID;
+
+private:
+  std::string Message;
+};
+
+char StreamExecutorError::ID = 0;
+
+} // namespace
+
+namespace streamexecutor {
+
+Error make_error(StringRef Message) {
+  return llvm::make_error<StreamExecutorError>(Message);
+}
+
+std::string consumeAndGetMessage(Error &&E) {
+  if (!E) {
+    return "success";
+  }
+  std::string Message;
+  llvm::handleAllErrors(std::move(E),
+                        [&Message](const StreamExecutorError &SEE) {
+                          Message = SEE.getErrorMessage();
+                        });
+  return Message;
+}
+
+} // namespace streamexecutor