[Support] Make consumeError, toString callable from code compiled with RTTI.

[lhames]

Summary

This moves the toString(Error) function, and the failure-handling case of
consumeError, out-of-line. This enables calls to consumeError, toString,
errorToBool, and expectedToOptional from code compiled with RTTI turned on,
even if LLVM is built with RTTI turned off.

[lhames]

Vassil -- This is a generalization of the error fix that we discussed. Could you double check that it fixes the linker error that you saw?

After thinking about it for a while I'm on the fence about this approach -- on the one hand it's a reasonable cleanup, on the other hand I don't think we generally support linking -no-rtti LLVM libraries from -rtti code, and support for doing that (even partial support) might be an explicit non-goal for the project.

Another approach to fixing the example might just be to split it into two files. Pros of that approach: it deals with no-rtti/rtti crossover code The Right Way, Con: it complicates the example for everyone else.

[v.g.vassilev]

In D111198#3044293, @lhames wrote:

Vassil -- This is a generalization of the error fix that we discussed. Could you double check that it fixes the linker error that you saw?

After thinking about it for a while I'm on the fence about this approach -- on the one hand it's a reasonable cleanup, on the other hand I don't think we generally support linking -no-rtti LLVM libraries from -rtti code, and support for doing that (even partial support) might be an explicit non-goal for the project.

Another approach to fixing the example might just be to split it into two files. Pros of that approach: it deals with no-rtti/rtti crossover code The Right Way, Con: it complicates the example for everyone else.

@lhames, thanks a lot for the patch -- this makes D107049 compile. I do not have a strong opinion on how/should we bridge rtti/no-rtti. I feel this will come up more often due to clang-repl...

[v.g.vassilev]

In D111198#3048647, @v.g.vassilev wrote:

In D111198#3044293, @lhames wrote:

Vassil -- This is a generalization of the error fix that we discussed. Could you double check that it fixes the linker error that you saw?

After thinking about it for a while I'm on the fence about this approach -- on the one hand it's a reasonable cleanup, on the other hand I don't think we generally support linking -no-rtti LLVM libraries from -rtti code, and support for doing that (even partial support) might be an explicit non-goal for the project.

Another approach to fixing the example might just be to split it into two files. Pros of that approach: it deals with no-rtti/rtti crossover code The Right Way, Con: it complicates the example for everyone else.

@lhames, thanks a lot for the patch -- this makes D107049 compile. I do not have a strong opinion on how/should we bridge rtti/no-rtti. I feel this will come up more often due to clang-repl...

Going via C also works for me:

diff
diff --git a/clang/unittests/Interpreter/ExceptionTests/InterpreterExceptionTest.cpp b/clang/unittests/Interpreter/ExceptionTests/InterpreterExceptionTest.cpp
index 744a98bcdc87..fbdeddf0300b 100644
--- a/clang/unittests/Interpreter/ExceptionTests/InterpreterExceptionTest.cpp
+++ b/clang/unittests/Interpreter/ExceptionTests/InterpreterExceptionTest.cpp
@@ -25,6 +25,7 @@
 #include "llvm/ExecutionEngine/Orc/LLJIT.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/TargetSelect.h"
+#include "llvm-c/Error.h"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -52,9 +53,7 @@ TEST(InterpreterTest, CatchException) {
     auto J = llvm::orc::LLJITBuilder().create();
     if (!J) {
       // The platform does not support JITs.
-      // We can't use llvm::consumeError as it needs typeinfo for ErrorInfoBase.
-      auto E = J.takeError();
-      (void)E;
+      LLVMConsumeError(llvm::wrap(J.takeError()));
       return;
     }
   }

I think that would be the least intrusive way to go...

[lhames]

Ok. I'm going to abandon this review -- we can revisit in the future if we find more instances of this issue.

llvm/include/llvm/Support/Error.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_ERROR_H
 #define LLVM_SUPPORT_ERROR_H
 
 #include "llvm-c/Error.h"
 #include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/SmallVector.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"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"

 /// can pass any of them as the OS. Please consider using them instead of
 /// including 'error: ' in the ErrorBanner.
 ///
 /// This is useful in the base level of your program to allow clean termination
 /// (allowing clean deallocation of resources, etc.), while reporting error
 /// information to the user.
 void logAllUnhandledErrors(Error E, raw_ostream &OS, Twine ErrorBanner = {});
 
+namespace detail {
+// Consume an Error in libSupport.
+// This is used to make consumeErrors callable from code compiled with
+// RTTI enabled, which would otherwise complain about missing type info for
+// ErrorInfoBase.
+void consumeErrorOutOfLine(Error Err);
+} // end namespace detail.
+
 /// Write all error messages (if any) in E to a string. The newline character
 /// is used to separate error messages.
-inline std::string toString(Error E) {
-  SmallVector<std::string, 2> Errors;
-  handleAllErrors(std::move(E), [&Errors](const ErrorInfoBase &EI) {
-    Errors.push_back(EI.message());
-  });
-  return join(Errors.begin(), Errors.end(), "\n");
-}
+std::string toString(Error E);
 
 /// Consume a Error without doing anything. This method should be used
 /// only where an error can be considered a reasonable and expected return
 /// value.
 ///
 /// Uses of this method are potentially indicative of design problems: If it's
 /// legitimate to do nothing while processing an "error", the error-producer
 /// might be more clearly refactored to return an Optional<T>.
 inline void consumeError(Error Err) {
-  handleAllErrors(std::move(Err), [](const ErrorInfoBase &) {});
+  if (Err)
+    detail::consumeErrorOutOfLine(std::move(Err));
 }
 
 /// Convert an Expected to an Optional without doing anything. This method
 /// should be used only where an error can be considered a reasonable and
 /// expected return value.
 ///
 /// Uses of this method are potentially indicative of problems: perhaps the
 /// error should be propagated further, or the error-producer should just
 /// return an Optional in the first place.
 template <typename T> Optional<T> expectedToOptional(Expected<T> &&E) {
   if (E)
     return std::move(*E);
-  consumeError(E.takeError());
+  detail::consumeErrorOutOfLine(E.takeError());
   return None;
 }
 
 /// Helper for converting an Error to a bool.
 ///
 /// This method returns true if Err is in an error state, or false if it is
 /// in a success state.  Puts Err in a checked state in both cases (unlike
 /// Error::operator bool(), which only does this for success states).
 inline bool errorToBool(Error Err) {
-  bool IsError = static_cast<bool>(Err);
-  if (IsError)
-    consumeError(std::move(Err));
-  return IsError;
+  if (!Err)
+    return false;
+
+  detail::consumeErrorOutOfLine(std::move(Err));
+  return true;
 }
 
 /// Helper for Errors used as out-parameters.
 ///
 /// This helper is for use with the Error-as-out-parameter idiom, where an error
 /// is passed to a function or method by reference, rather than being returned.
 /// In such cases it is helpful to set the checked bit on entry to the function
 /// so that the error can be written to (unchecked Errors abort on assignment)

llvm/lib/Support/Error.cpp

 //===----- lib/Support/Error.cpp - Error and associated utilities ---------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/Error.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ManagedStatic.h"
 #include <system_error>
 
 using namespace llvm;
 
 namespace {
if (!E)
     return;
   OS << ErrorBanner;
   handleAllErrors(std::move(E), [&](const ErrorInfoBase &EI) {
     EI.log(OS);
     OS << "\n";
   });
 }
 
+std::string toString(Error E) {
+  SmallVector<std::string, 2> Errors;
+  handleAllErrors(std::move(E), [&Errors](const ErrorInfoBase &EI) {
+    Errors.push_back(EI.message());
+  });
+  return join(Errors.begin(), Errors.end(), "\n");
+}
+
+namespace detail {
+void consumeErrorOutOfLine(Error Err) {
+  handleAllErrors(std::move(Err), [](const ErrorInfoBase &) {});
+}
+} // namespace detail
 
 std::error_code ErrorList::convertToErrorCode() const {
   return std::error_code(static_cast<int>(ErrorErrorCode::MultipleErrors),
                          *ErrorErrorCat);
 }
 
 std::error_code inconvertibleErrorCode() {
   return std::error_code(static_cast<int>(ErrorErrorCode::InconvertibleError),