Add LLDB_LOG_ERROR (?)

[labath]

Summary

The difference between this and regular LLDB_LOG is that this one clears
the error object unconditionally. This was inspired by the
ObjectFileELF bug (r322664), where the error object was being cleared
only if logging was enabled.

Of course, one could argue that since our logging is optional, it does
not really qualify as "handling" the error and we should leave it up to
the programmer to explicitly clear it if he really wants to.

It's not really clear to me which position is better. What do you think?

[davide]

I think this is fine, but I'll defer to Zachary.

[davide]

Forgot to click accept.

[jingham]

This seems fine as a bit of functionality but I'm worried about the name. There's nothing in "LLDB_LOG_ERROR" that indicates that the error gets cleared, but that's actually a pretty important piece of its business. Before this propagates further is there a better name we can come up with that's not too horrible to type or scan but conveys this? LLDB_LOG_AND_CLEAR_ERROR is not too bad, though if there's a better single word that conveys logging & handling that would be nicer. But I can't think of one off the top of my head.

[zturner]

In a way it's kind of built into the semantics of llvm::Error that this is the only way it could possibly work, since it's a move only type. If you do this, for example:

Error E = doSomething();
LLDB_LOG_ERROR(E);

you'd get a compilation failure. The only way to make it compile would be to do this:

Error E = doSomething();
LLDB_LOG_ERROR(std::move(E));

And since you've written std::move(E) you already can't use it again anyway. And if you had written it as one line:

LLDB_LOG_ERROR(doSomething());

Then you weren't holding onto the error anyway and it shouldn't matter whether or not it was cleared.

So I'm not sold on the idea of lengthening the name, because you couldn't have used the Error anyway after calling this.

[jingham]

Seems important to me that a name tells what it does without having to know the implementation details of what it acts on. Particularly for folks new to the code, having to know one less thing to understand how something that's important to the logic flow of the program behaves will make comprehending the code easier. It's only a little bump to understanding, but the fewer of those we have the better, IMO.

[labath]

The behavior of llvm::Error is one more quirk that you need to know about, but I don't think the logging machinery is the place that should teach you that. If you're working with llvm::Error, in all likelyhook you've already had to learn about this behavior anyway, in which case seeing the std::move should be enough of a signal to explain what happens. So, I'd prefer to stick to a shorter name here.

[labath]

It looks like we haven't come up with a better name, and there's new code in D42468 which would benefit a lot from this functionality, so I'm going to land this. I don't expect hundreds of uses of this macro appearing any time soon, so it should be easy enough to change it if we find something better.

lldb/trunk/include/lldb/Utility/Log.h

 
 #include "lldb/Utility/Flags.h"
 #include "lldb/Utility/Logging.h"
 #include "lldb/lldb-defines.h"
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringMap.h" // for StringMap
 #include "llvm/ADT/StringRef.h" // for StringRef, StringLiteral
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/RWMutex.h"
 
 #include <atomic>
 #include <cstdarg>
 #include <cstdint>
 #include <memory>      // for shared_ptr
public:
   void PutString(llvm::StringRef str);
 
   template <typename... Args>
   void Format(llvm::StringRef file, llvm::StringRef function,
               const char *format, Args &&... args) {
     Format(file, function, llvm::formatv(format, std::forward<Args>(args)...));
   }
 
+  template <typename... Args>
+  void FormatError(llvm::Error error, llvm::StringRef file,
+                   llvm::StringRef function, const char *format,
+                   Args &&... args) {
+    Format(file, function,
+           llvm::formatv(format, llvm::toString(std::move(error)),
+                         std::forward<Args>(args)...));
+  }
+
   void Printf(const char *format, ...) __attribute__((format(printf, 2, 3)));
 
   void VAPrintf(const char *format, va_list args);
 
   void Error(const char *fmt, ...) __attribute__((format(printf, 2, 3)));
 
   void VAError(const char *format, va_list args);
 

 
 #define LLDB_LOGV(log, ...)                                                    \
   do {                                                                         \
     ::lldb_private::Log *log_private = (log);                                  \
     if (log_private && log_private->GetVerbose())                              \
       log_private->Format(__FILE__, __func__, __VA_ARGS__);                    \
   } while (0)
 
+// Write message to log, if error is set. In the log message refer to the error
+// with {0}. Error is cleared regardless of whether logging is enabled.
+#define LLDB_LOG_ERROR(log, error, ...)                                        \
+  do {                                                                         \
+    ::lldb_private::Log *log_private = (log);                                  \
+    ::llvm::Error error_private = (error);                                     \
+    if (log_private && error_private) {                                        \
+      log_private->FormatError(::std::move(error_private), __FILE__, __func__, \
+                               __VA_ARGS__);                                   \
+    } else                                                                     \
+      ::llvm::consumeError(::std::move(error_private));                        \
+  } while (0)
+
 #endif // LLDB_UTILITY_LOG_H

lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

if (result == 0 || !section->Test(SHF_COMPRESSED))
     return result;
 
   auto Decompressor = llvm::object::Decompressor::create(
       section->GetName().GetStringRef(),
       {reinterpret_cast<const char *>(section_data.GetDataStart()),
        size_t(section_data.GetByteSize())},
       GetByteOrder() == eByteOrderLittle, GetAddressByteSize() == 8);
   if (!Decompressor) {
-    LLDB_LOG(log, "Unable to initialize decompressor for section {0}: {1}",
-             section->GetName(), llvm::toString(Decompressor.takeError()));
-    consumeError(Decompressor.takeError());
+    LLDB_LOG_ERROR(log, Decompressor.takeError(),
+                   "Unable to initialize decompressor for section {0}",
+                   section->GetName());
     return result;
   }
   auto buffer_sp =
       std::make_shared<DataBufferHeap>(Decompressor->getDecompressedSize(), 0);
   if (auto Error = Decompressor->decompress(
           {reinterpret_cast<char *>(buffer_sp->GetBytes()),
            size_t(buffer_sp->GetByteSize())})) {
-    LLDB_LOG(log, "Decompression of section {0} failed: {1}",
-             section->GetName(), llvm::toString(std::move(Error)));
-    consumeError(std::move(Error));
+    LLDB_LOG_ERROR(log, std::move(Error), "Decompression of section {0} failed",
+                   section->GetName());
     return result;
   }
   section_data.SetData(buffer_sp);
   return buffer_sp->GetByteSize();
 }

lldb/trunk/unittests/Utility/LogTest.cpp

TEST_F(LogChannelEnabledTest, log_options) {
   EXPECT_TRUE(EnableChannel(
       getStream(), LLDB_LOG_OPTION_PREPEND_PROC_AND_THREAD, "chan", {}, Err));
   EXPECT_EQ(llvm::formatv("[{0,0+4}/{1,0+4}] Hello World\n", ::getpid(),
                           llvm::get_threadid())
                 .str(),
             logAndTakeOutput("Hello World"));
 }
 
+TEST_F(LogChannelEnabledTest, LLDB_LOG_ERROR) {
+  LLDB_LOG_ERROR(getLog(), llvm::Error::success(), "Foo failed: {0}");
+  ASSERT_EQ("", takeOutput());
+
+  LLDB_LOG_ERROR(getLog(),
+                 llvm::make_error<llvm::StringError>(
+                     "My Error", llvm::inconvertibleErrorCode()),
+                 "Foo failed: {0}");
+  ASSERT_EQ("Foo failed: My Error\n", takeOutput());
+
+  // Doesn't log, but doesn't assert either
+  LLDB_LOG_ERROR(nullptr,
+                 llvm::make_error<llvm::StringError>(
+                     "My Error", llvm::inconvertibleErrorCode()),
+                 "Foo failed: {0}");
+}
+
 TEST_F(LogChannelEnabledTest, LogThread) {
   // Test that we are able to concurrently write to a log channel and disable
   // it.
   std::string err;
 
   // Start logging on one thread. Concurrently, try disabling the log channel.
   std::thread log_thread([this] { LLDB_LOG(getLog(), "Hello World"); });
   EXPECT_TRUE(DisableChannel("chan", {}, err));