zturner created this revision.
Instead of having the Error class (which is in Utility) know about logging
(which is in Core), it seems more appropriate to put all logging code together,
and teach Log about errors rather than teaching Error about logs. This patch
does so.
https://reviews.llvm.org/D29514
Files:
lldb/include/lldb/Core/Log.h
lldb/include/lldb/Utility/Error.h
lldb/source/Core/Communication.cpp
lldb/source/Host/common/Host.cpp
lldb/source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp
lldb/source/Utility/Error.cpp
llvm/include/llvm/Support/FormatProviders.h
Index: llvm/include/llvm/Support/FormatProviders.h
===================================================================
--- llvm/include/llvm/Support/FormatProviders.h
+++ llvm/include/llvm/Support/FormatProviders.h
@@ -18,6 +18,7 @@
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringSwitch.h"
+#include "llvm/ADT/Twine.h"
#include "llvm/Support/FormatVariadicDetails.h"
#include "llvm/Support/NativeFormatting.h"
@@ -262,6 +263,12 @@
}
};
+template <> struct format_provider<Twine> {
+ static void format(const Twine &T, raw_ostream &Stream, StringRef Style) {
+ Stream << T;
+ }
+};
+
/// Implementation of format_provider<T> for floating point types.
///
/// The options string of a floating point type has the format:
Index: lldb/source/Utility/Error.cpp
===================================================================
--- lldb/source/Utility/Error.cpp
+++ lldb/source/Utility/Error.cpp
@@ -130,72 +130,6 @@
bool Error::Fail() const { return m_code != 0; }
//----------------------------------------------------------------------
-// Log the error given a string with format. If the this object
-// contains an error code, update the error string to contain the
-// "error: " followed by the formatted string, followed by the error
-// value and any string that describes the current error. This
-// allows more context to be given to an error string that remains
-// cached in this object. Logging always occurs even when the error
-// code contains a non-error value.
-//----------------------------------------------------------------------
-void Error::PutToLog(Log *log, const char *format, ...) {
- char *arg_msg = nullptr;
- va_list args;
- va_start(args, format);
- ::vasprintf(&arg_msg, format, args);
- va_end(args);
-
- if (arg_msg != nullptr) {
- if (Fail()) {
- const char *err_str = AsCString();
- if (err_str == nullptr)
- err_str = "???";
-
- SetErrorStringWithFormat("error: %s err = %s (0x%8.8x)", arg_msg, err_str,
- m_code);
- if (log != nullptr)
- log->Error("%s", m_string.c_str());
- } else {
- if (log != nullptr)
- log->Printf("%s err = 0x%8.8x", arg_msg, m_code);
- }
- ::free(arg_msg);
- }
-}
-
-//----------------------------------------------------------------------
-// Log the error given a string with format. If the this object
-// contains an error code, update the error string to contain the
-// "error: " followed by the formatted string, followed by the error
-// value and any string that describes the current error. This
-// allows more context to be given to an error string that remains
-// cached in this object. Logging only occurs even when the error
-// code contains a error value.
-//----------------------------------------------------------------------
-void Error::LogIfError(Log *log, const char *format, ...) {
- if (Fail()) {
- char *arg_msg = nullptr;
- va_list args;
- va_start(args, format);
- ::vasprintf(&arg_msg, format, args);
- va_end(args);
-
- if (arg_msg != nullptr) {
- const char *err_str = AsCString();
- if (err_str == nullptr)
- err_str = "???";
-
- SetErrorStringWithFormat("%s err = %s (0x%8.8x)", arg_msg, err_str,
- m_code);
- if (log != nullptr)
- log->Error("%s", m_string.c_str());
-
- ::free(arg_msg);
- }
- }
-}
-
-//----------------------------------------------------------------------
// Set accesssor for the error value to "err" and the type to
// "eErrorTypeMachKernel"
//----------------------------------------------------------------------
@@ -334,3 +268,10 @@
bool Error::WasInterrupted() const {
return (m_type == eErrorTypePOSIX && m_code == EINTR);
}
+
+void llvm::format_provider<lldb_private::Error>::format(
+ const lldb_private::Error &error, llvm::raw_ostream &OS,
+ llvm::StringRef Options) {
+ llvm::format_provider<llvm::StringRef>::format(error.AsCString(), OS,
+ Options);
+}
Index: lldb/source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp
===================================================================
--- lldb/source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp
+++ lldb/source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp
@@ -319,7 +319,7 @@
for (uint64_t i = 0; i < allglen; ++i) {
goroutines.push_back(CreateGoroutineAtIndex(i, err));
if (err.Fail()) {
- err.PutToLog(log, "OperatingSystemGo::UpdateThreadList");
+ LLDB_LOG_ERROR(log, err, "OperatingSystemGo::UpdateThreadList");
return new_thread_list.GetSize(false) > 0;
}
}
Index: lldb/source/Host/common/Host.cpp
===================================================================
--- lldb/source/Host/common/Host.cpp
+++ lldb/source/Host/common/Host.cpp
@@ -685,10 +685,10 @@
posix_spawnattr_t attr;
error.SetError(::posix_spawnattr_init(&attr), eErrorTypePOSIX);
- if (error.Fail() || log)
- error.PutToLog(log, "::posix_spawnattr_init ( &attr )");
- if (error.Fail())
+ if (error.Fail()) {
+ LLDB_LOG_ERROR(log, error, "::posix_spawnattr_init ( &attr )");
return error;
+ }
// Make a quick class that will cleanup the posix spawn attributes in case
// we return in the middle of this function.
@@ -709,11 +709,10 @@
short flags = GetPosixspawnFlags(launch_info);
error.SetError(::posix_spawnattr_setflags(&attr, flags), eErrorTypePOSIX);
- if (error.Fail() || log)
- error.PutToLog(log, "::posix_spawnattr_setflags ( &attr, flags=0x%8.8x )",
- flags);
- if (error.Fail())
+ if (error.Fail()) {
+ LLDB_LOG_ERROR(log, error, "::posix_spawnattr_setflags ( &attr, flags={0:x} )", flags);
return error;
+ }
// posix_spawnattr_setbinpref_np appears to be an Apple extension per:
// http://www.unix.com/man-page/OSX/3/posix_spawnattr_setbinpref_np/
@@ -740,10 +739,9 @@
size_t ocount = 0;
error.SetError(::posix_spawnattr_setbinpref_np(&attr, 1, &cpu, &ocount),
eErrorTypePOSIX);
- if (error.Fail() || log)
- error.PutToLog(log, "::posix_spawnattr_setbinpref_np ( &attr, 1, "
- "cpu_type = 0x%8.8x, count => %llu )",
- cpu, (uint64_t)ocount);
+ if (error.Fail())
+ LLDB_LOG_ERROR(log, error, "::posix_spawnattr_setbinpref_np ( &attr, 1, "
+ "cpu_type = {0:x}, count => {1} )", cpu, ocount);
if (error.Fail() || ocount != 1)
return error;
@@ -813,10 +811,10 @@
posix_spawn_file_actions_t file_actions;
error.SetError(::posix_spawn_file_actions_init(&file_actions),
eErrorTypePOSIX);
- if (error.Fail() || log)
- error.PutToLog(log, "::posix_spawn_file_actions_init ( &file_actions )");
- if (error.Fail())
+ if (error.Fail()) {
+ LLDB_LOG_ERROR(log, error, "::posix_spawn_file_actions_init ( &file_actions )");
return error;
+ }
// Make a quick class that will cleanup the posix spawn attributes in case
// we return in the middle of this function.
@@ -838,16 +836,13 @@
::posix_spawnp(&result_pid, exe_path, &file_actions, &attr, argv, envp),
eErrorTypePOSIX);
- if (error.Fail() || log) {
- error.PutToLog(
- log, "::posix_spawnp ( pid => %i, path = '%s', file_actions = %p, "
- "attr = %p, argv = %p, envp = %p )",
- result_pid, exe_path, static_cast<void *>(&file_actions),
- static_cast<void *>(&attr), reinterpret_cast<const void *>(argv),
- reinterpret_cast<const void *>(envp));
+ if (error.Fail()) {
+ LLDB_LOG_ERROR(log, error, "::posix_spawnp(pid => {0}, path = '{1}', file_actions = {2}, "
+ "attr = {3}, argv = {4}, envp = {5} )",
+ result_pid, exe_path, &file_actions, &attr, argv, envp);
if (log) {
for (int ii = 0; argv[ii]; ++ii)
- log->Printf("argv[%i] = '%s'", ii, argv[ii]);
+ LLDB_LOG(log, "argv[{0}] = '{1}'", ii, argv[ii]);
}
}
@@ -856,16 +851,13 @@
::posix_spawnp(&result_pid, exe_path, NULL, &attr, argv, envp),
eErrorTypePOSIX);
- if (error.Fail() || log) {
- error.PutToLog(log, "::posix_spawnp ( pid => %i, path = '%s', "
- "file_actions = NULL, attr = %p, argv = %p, envp = "
- "%p )",
- result_pid, exe_path, static_cast<void *>(&attr),
- reinterpret_cast<const void *>(argv),
- reinterpret_cast<const void *>(envp));
+ if (error.Fail()) {
+ LLDB_LOG_ERROR(log, error, "::posix_spawnp ( pid => {0}, path = '{1}', "
+ "file_actions = NULL, attr = {2}, argv = {3}, envp = {4} )",
+ result_pid, exe_path, &attr, argv, envp);
if (log) {
for (int ii = 0; argv[ii]; ++ii)
- log->Printf("argv[%i] = '%s'", ii, argv[ii]);
+ LLDB_LOG("argv[{0}] = '{1}'", ii, argv[ii]);
}
}
}
@@ -908,10 +900,9 @@
error.SetError(
::posix_spawn_file_actions_addclose(file_actions, info->GetFD()),
eErrorTypePOSIX);
- if (log && (error.Fail() || log))
- error.PutToLog(log,
- "posix_spawn_file_actions_addclose (action=%p, fd=%i)",
- static_cast<void *>(file_actions), info->GetFD());
+ if (error.Fail())
+ LLDB_LOG_ERROR(log, error, "posix_spawn_file_actions_addclose (action={0}, fd={1})",
+ file_actions, info->GetFD());
}
break;
@@ -927,12 +918,10 @@
::posix_spawn_file_actions_adddup2(file_actions, info->GetFD(),
info->GetActionArgument()),
eErrorTypePOSIX);
- if (log && (error.Fail() || log))
- error.PutToLog(
- log,
- "posix_spawn_file_actions_adddup2 (action=%p, fd=%i, dup_fd=%i)",
- static_cast<void *>(file_actions), info->GetFD(),
- info->GetActionArgument());
+ if (error.Fail())
+ LLDB_LOG_ERROR(log, error,
+ "posix_spawn_file_actions_adddup2 (action={0}, fd={1}, dup_fd={2})",
+ file_actions, info->GetFD(), info->GetActionArgument());
}
break;
@@ -952,11 +941,10 @@
file_actions, info->GetFD(),
info->GetPath().str().c_str(), oflag, mode),
eErrorTypePOSIX);
- if (error.Fail() || log)
- error.PutToLog(log, "posix_spawn_file_actions_addopen (action=%p, "
- "fd=%i, path='%s', oflag=%i, mode=%i)",
- static_cast<void *>(file_actions), info->GetFD(),
- info->GetPath().str().c_str(), oflag, mode);
+ if (error.Fail())
+ LLDB_LOG_ERROR(log, error, "posix_spawn_file_actions_addopen (action={0}, "
+ "fd={1}, path='{2}', oflag={3}, mode={4})",
+ file_actions, info->GetFD(), info->GetPath(), oflag, mode);
}
break;
}
Index: lldb/source/Core/Communication.cpp
===================================================================
--- lldb/source/Core/Communication.cpp
+++ lldb/source/Core/Communication.cpp
@@ -322,10 +322,9 @@
comm->Disconnect();
done = true;
}
- if (log)
- error.LogIfError(
- log, "%p Communication::ReadFromConnection () => status = %s", p,
- Communication::ConnectionStatusAsCString(status));
+ if (error.Fail())
+ LLDB_LOG_ERROR(log, error, "status = {0}",
+ Communication::ConnectionStatusAsCString(status));
break;
case eConnectionStatusInterrupted: // Synchronization signal from
// SynchronizeWithReadThread()
@@ -340,10 +339,9 @@
done = true;
LLVM_FALLTHROUGH;
case eConnectionStatusTimedOut: // Request timed out
- if (log)
- error.LogIfError(
- log, "%p Communication::ReadFromConnection () => status = %s", p,
- Communication::ConnectionStatusAsCString(status));
+ if (error.Fail())
+ LLDB_LOG_ERROR(log, error, "status = {0}",
+ Communication::ConnectionStatusAsCString(status));
break;
}
}
Index: lldb/include/lldb/Utility/Error.h
===================================================================
--- lldb/include/lldb/Utility/Error.h
+++ lldb/include/lldb/Utility/Error.h
@@ -11,7 +11,10 @@
#define __DCError_h__
#if defined(__cplusplus)
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
#include "llvm/Support/DataTypes.h"
+#include "llvm/Support/FormatAdapters.h"
#include "llvm/Support/FormatVariadic.h"
#include <cstdarg>
@@ -24,8 +27,6 @@
namespace lldb_private {
-class Log;
-
//----------------------------------------------------------------------
/// @class Error Error.h "lldb/Utility/Error.h"
/// @brief An error handling class.
@@ -148,48 +149,6 @@
lldb::ErrorType GetType() const;
//------------------------------------------------------------------
- /// Log an error to Log().
- ///
- /// Log the error given a formatted string \a format. If the this
- /// object contains an error code, update the error string to
- /// contain the prefix "error: ", followed by the formatted string,
- /// followed by the error value and any string that describes the
- /// error value. This allows more context to be given to an error
- /// string that remains cached in this object. Logging always occurs
- /// even when the error code contains a non-error value.
- ///
- /// @param[in] format
- /// A printf style format string.
- ///
- /// @param[in] ...
- /// Variable arguments that are needed for the printf style
- /// format string \a format.
- //------------------------------------------------------------------
- void PutToLog(Log *log, const char *format, ...)
- __attribute__((format(printf, 3, 4)));
-
- //------------------------------------------------------------------
- /// Log an error to Log() if the error value is an error.
- ///
- /// Log the error given a formatted string \a format only if the
- /// error value in this object describes an error condition. If the
- /// this object contains an error, update the error string to
- /// contain the prefix "error: " followed by the formatted string,
- /// followed by the error value and any string that describes the
- /// error value. This allows more context to be given to an error
- /// string that remains cached in this object.
- ///
- /// @param[in] format
- /// A printf style format string.
- ///
- /// @param[in] ...
- /// Variable arguments that are needed for the printf style
- /// format string \a format.
- //------------------------------------------------------------------
- void LogIfError(Log *log, const char *format, ...)
- __attribute__((format(printf, 3, 4)));
-
- //------------------------------------------------------------------
/// Set accessor from a kern_return_t.
///
/// Set accesssor for the error value to \a err and the error type
@@ -304,12 +263,44 @@
namespace llvm {
template <> struct format_provider<lldb_private::Error> {
static void format(const lldb_private::Error &error, llvm::raw_ostream &OS,
- llvm::StringRef Options) {
- llvm::format_provider<llvm::StringRef>::format(error.AsCString(), OS,
- Options);
+ llvm::StringRef Options);
+};
+}
+
+namespace lldb_private {
+namespace detail {
+class ErrorWithContextAdapter final : public llvm::FormatAdapter<Error> {
+ std::string Context;
+
+public:
+ template <typename... Ts>
+ ErrorWithContextAdapter(Error &&Err, const char *Context, Ts &&... Args)
+ : FormatAdapter<Error>(std::forward<Error>(Err)) {
+ this->Context = llvm::formatv(Context, std::forward<Ts>(Args)...).str();
+ }
+
+ void format(llvm::raw_ostream &Stream, llvm::StringRef Style) {
+ if (Item.Fail()) {
+ const char *err_str = Item.AsCString();
+ if (err_str == nullptr)
+ err_str = "???";
+
+ Stream << llvm::formatv("error: {0} err = {1} ({2:x})", Context, err_str,
+ Item.GetError());
+ } else {
+ Stream << llvm::formatv("{0} err = {1:x}", Context, Item.GetError());
+ }
}
};
}
+template <typename... Ts>
+inline detail::ErrorWithContextAdapter
+fmt_error_context(Error &&Err, const char *Context, Ts &&... Args) {
+ return detail::ErrorWithContextAdapter(std::forward<Error>(Err), Context,
+ std::forward<Ts>(Args)...);
+}
+}
+
#endif // #if defined(__cplusplus)
#endif // #ifndef __DCError_h__
Index: lldb/include/lldb/Core/Log.h
===================================================================
--- lldb/include/lldb/Core/Log.h
+++ lldb/include/lldb/Core/Log.h
@@ -15,6 +15,7 @@
#include "lldb/Core/Logging.h"
#include "lldb/Core/PluginInterface.h"
#include "lldb/Utility/ConstString.h"
+#include "lldb/Utility/Error.h"
#include "lldb/lldb-private.h"
// Other libraries and framework includes
@@ -200,4 +201,15 @@
log_private->Format(__FILE__, __FUNCTION__, __VA_ARGS__); \
} while (0)
+#define LLDB_LOG_ERROR(log, error, ...) \
+ do { \
+ ::lldb_private::Log *log_private = (log); \
+ if (log_private) { \
+ log_private->Format( \
+ __FILE__, __FUNCTION__, "{0}", \
+ fmt_error_context(std::forward<lldb_private::Error>(error), \
+ __VA_ARGS__)); \
+ } \
+ } while (0)
+
#endif // liblldb_Log_h_
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
