labath created this revision.
labath added reviewers: davide, zturner, jingham, clayborg.
Herald added a subscriber: emaste.

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?


https://reviews.llvm.org/D42182

Files:
  include/lldb/Utility/Log.h
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  unittests/Utility/LogTest.cpp

Index: unittests/Utility/LogTest.cpp
===================================================================
--- unittests/Utility/LogTest.cpp
+++ unittests/Utility/LogTest.cpp
@@ -240,6 +240,23 @@
             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.
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===================================================================
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -3491,19 +3491,18 @@
        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);
Index: include/lldb/Utility/Log.h
===================================================================
--- include/lldb/Utility/Log.h
+++ include/lldb/Utility/Log.h
@@ -17,6 +17,7 @@
 #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"
@@ -140,6 +141,15 @@
     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);
@@ -218,4 +228,17 @@
       log_private->Format(__FILE__, __FUNCTION__, __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__,           \
+                               __FUNCTION__, __VA_ARGS__);                     \
+    } else                                                                     \
+      ::llvm::consumeError(::std::move(error_private));                        \
+  } while (0)
+
 #endif // LLDB_UTILITY_LOG_H
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to