[Lldb-commits] [PATCH] D29514: Change Error::PutToLog to LLDB_LOG_ERROR
This revision was automatically updated to reflect the committed changes.
Closed by commit rL294210: Get rid of Error::PutToLog(). (authored by zturner).
Changed prior to commit:
https://reviews.llvm.org/D29514?vs=87036=87260#toc
Repository:
rL LLVM
https://reviews.llvm.org/D29514
Files:
lldb/trunk/include/lldb/Utility/Error.h
lldb/trunk/source/Core/Communication.cpp
lldb/trunk/source/Host/common/Host.cpp
lldb/trunk/source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp
lldb/trunk/source/Utility/Error.cpp
Index: lldb/trunk/include/lldb/Utility/Error.h
===
--- lldb/trunk/include/lldb/Utility/Error.h
+++ lldb/trunk/include/lldb/Utility/Error.h
@@ -12,6 +12,7 @@
#if defined(__cplusplus)
#include "llvm/Support/DataTypes.h"
+#include "llvm/Support/FormatAdapters.h"
#include "llvm/Support/FormatVariadic.h"
#include
@@ -24,8 +25,6 @@
namespace lldb_private {
-class Log;
-
//--
/// @class Error Error.h "lldb/Utility/Error.h"
/// @brief An error handling class.
@@ -148,48 +147,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,10 +261,7 @@
namespace llvm {
template <> struct format_provider {
static void format(const lldb_private::Error , llvm::raw_ostream ,
- llvm::StringRef Options) {
-llvm::format_provider::format(error.AsCString(), OS,
- Options);
- }
+ llvm::StringRef Options);
};
}
Index: lldb/trunk/source/Host/common/Host.cpp
===
--- lldb/trunk/source/Host/common/Host.cpp
+++ lldb/trunk/source/Host/common/Host.cpp
@@ -685,10 +685,10 @@
posix_spawnattr_t attr;
error.SetError(::posix_spawnattr_init(), eErrorTypePOSIX);
- if (error.Fail() || log)
-error.PutToLog(log, "::posix_spawnattr_init ( )");
- if (error.Fail())
+ if (error.Fail()) {
+LLDB_LOG(log, "error: {0}, ::posix_spawnattr_init ( )", error);
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,12 @@
short flags = GetPosixspawnFlags(launch_info);
error.SetError(::posix_spawnattr_setflags(, flags), eErrorTypePOSIX);
- if (error.Fail() || log)
-error.PutToLog(log, "::posix_spawnattr_setflags ( , flags=0x%8.8x )",
- flags);
- if (error.Fail())
+ if (error.Fail()) {
+LLDB_LOG(log,
+ "error: {0}, ::posix_spawnattr_setflags ( , flags={1:x} )",
+ error, 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
[Lldb-commits] [PATCH] D29514: Change Error::PutToLog to LLDB_LOG_ERROR
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
looks good, just make sure it compiles.
Comment at: lldb/include/lldb/Core/Log.h:18
#include "lldb/Utility/ConstString.h"
+#include "lldb/Utility/Error.h"
#include "lldb/lldb-private.h"
This is also unnecessary.
Comment at: lldb/include/lldb/Utility/Error.h:14
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
All these includes are now unnecessary :)
Comment at: lldb/source/Host/common/Host.cpp:911
eErrorTypePOSIX);
- if (log && (error.Fail() || log))
-error.PutToLog(log,
lol :)
Comment at: lldb/source/Host/common/Host.cpp:952
+ if (error.Fail())
+LLDB_LOG_ERROR(
+log, error, "posix_spawn_file_actions_addopen (action={0}, "
looks like you forgot this one.
https://reviews.llvm.org/D29514
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D29514: Change Error::PutToLog to LLDB_LOG_ERROR
zturner updated this revision to Diff 87036.
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
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(_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(_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::format(
+const lldb_private::Error , llvm::raw_ostream ,
+llvm::StringRef Options) {
+ llvm::format_provider::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(log, "error: {0}", err);
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(), eErrorTypePOSIX);
- if (error.Fail() || log)
-error.PutToLog(log, "::posix_spawnattr_init ( )");
- if (error.Fail())
+ if (error.Fail()) {
+LLDB_LOG(log, "error: {0}, ::posix_spawnattr_init ( )", error);
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,12 @@
short flags = GetPosixspawnFlags(launch_info);
error.SetError(::posix_spawnattr_setflags(, flags), eErrorTypePOSIX);
- if (error.Fail() || log)
-
[Lldb-commits] [PATCH] D29514: Change Error::PutToLog to LLDB_LOG_ERROR
zturner added a comment. Fair enough, I can do that. https://reviews.llvm.org/D29514 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D29514: Change Error::PutToLog to LLDB_LOG_ERROR
zturner added a comment.
In https://reviews.llvm.org/D29514#666285, @labath wrote:
> I'm not opposed to this patch, if people really want it, but I don't really
> see the value of this macro.
> Why couldn't I write
> `LLDB_LOG(log, "foo: {0}", error);`
> instead of
> `LLDB_LOG_ERROR(log, error, "foo");`
> Am I missing something?
I'm all for making it simpler. The reason I did it this way is that I wanted
to keep the exact same syntax as before to minimize the impact of the change.
It's rather confusing, but suppose you have these two errors:
1. E1 = Success, message = "The operation completed successfully", code = 0
2. E2 = Error, message = "The operation failed", code = 0x12345
When you call `PutToLog(E1, "While reading the %d'th item", 7)` it will log the
string `While reading the 7'th item err = 0x0`
When you call `PutToLog(E2, "While reading the %d'th item", 7)` it will log the
string `error: While reading the 7'th item err = The operation failed (0x12345)`
If we do what you suggest, then we are relying on the `lldb::Error` format
provider, which does not have the ability to accept this kind of "contextual
reason" string (i.e. the string "While reading the %d'th item" in the above
example). So if we did this:
LLDB_LOG(log, error, "While reading the {0}'th item", 7)
Then the entire error must be formatted independently of the entire context
string. So we could produce the output `error: The operation failed (0x12345),
context: While reading the 7'th item` but this is slightly different from what
was being printed before.
I didn't actually try this approach and see if anything broke, because Windows
runs much fewer tests than the rest of LLDB, so even if my test suite passed,
it wouldn't guarantee that another test somewhere else wasn't relying on the
message being exactly as it currently prints.
https://reviews.llvm.org/D29514
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D29514: Change Error::PutToLog to LLDB_LOG_ERROR
clayborg added a comment. So I agree with Pavel. Let us know what you think https://reviews.llvm.org/D29514 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D29514: Change Error::PutToLog to LLDB_LOG_ERROR
clayborg added a comment. I realize the functionality would add a "error: " prefix if it wasn't there, but it seems like we could add this as a formatting option of the lldb_private::Error class? Then we can just switch the logging code to put the error in as a log item and add the extra flag to ensure we print "error:" it is isn't already prefixed with that? https://reviews.llvm.org/D29514 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D29514: Change Error::PutToLog to LLDB_LOG_ERROR
labath added a comment. As for the modules part, I assumed the Log class will end up in the lowest layer also. I intended to move it there after I am done with it, but we can do it sooner if that is stopping you from doing anything. ( I do agree that it makes sense to reverse the dependency though). https://reviews.llvm.org/D29514 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D29514: Change Error::PutToLog to LLDB_LOG_ERROR
labath added a comment.
I'm not opposed to this patch, if people really want it, but I don't really see
the value of this macro.
Why couldn't I write
`LLDB_LOG(log, "foo: {0}", error);`
instead of
`LLDB_LOG_ERROR(log, error, "foo");`
Am I missing something?
https://reviews.llvm.org/D29514
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D29514: Change Error::PutToLog to LLDB_LOG_ERROR
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 {
+ static void format(const Twine , raw_ostream , StringRef Style) {
+Stream << T;
+ }
+};
+
/// Implementation of format_provider 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(_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(_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::format(
+const lldb_private::Error , llvm::raw_ostream ,
+llvm::StringRef Options) {
+ llvm::format_provider::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) {
