llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
Author: Yao Qi (qiyao)
<details>
<summary>Changes</summary>
`CommandReturnObject::SetError` silently drops the message of a
`DiagnosticError`-derived error that has no `DiagnosticDetail` entries.
The command's status becomes `eReturnStatusFailed`, but nothing at all
gets printed, not even `error:`.
The handler passed to `llvm::handleErrors` returns `void`:
```cpp
error = llvm::handleErrors(std::move(error), [&](DiagnosticError
&error) {
SetStatus(eReturnStatusFailed);
m_diagnostics = error.GetDetails();
});
if (error) {
AppendError(llvm::toString(std::move(error)));
}
```
Because of that, `llvm::handleErrors` picks the
`ErrorHandlerTraits<void (&)(ErrT &)>` specialization
(`llvm/include/llvm/Support/Error.h`), whose `apply()` unconditionally
returns `Error::success()` once the handler runs, no matter what the
handler did. The matched error's payload, and its `message()`, is gone
at that point. `m_diagnostics` only receives the *structured*
`GetDetails()` list, which can be empty even when the error carries a
perfectly good plain-text message. `AppendError`, the only call that
would actually print something, never runs.
I could not find a built-in command that hits this upstream today.
`ClangExpressionParser`, `LLVMUserExpression::DoExecute`, and
`ClangUtilityFunction::Install` all call
`diagnostic_manager.PutString`/`Printf` before every failure return, so
the `ExpressionError`s they produce always carry at least one detail.
`OptionParseError` and `DILDiagnosticError` are hardcoded to always
carry one too. The defect is real regardless: any `DiagnosticError`
subclass that reports a plain message without details hits it, which is
what a downstream lldb fork's Swift expression evaluator does, making
a failed `expression` command print nothing at all.
Give the handler return type `llvm::Error` instead of `void`. That
selects `ErrorHandlerTraits<Error (&)(ErrT &)>::apply()`, which
forwards the handler's return value instead of discarding it, so
returning a fresh error when `GetDetails()` is empty makes it through
to the existing `AppendError` call.
Added `CommandReturnObjectTest.SetErrorFromDiagnosticErrorWithoutDetails`,
with a minimal `DiagnosticError` subclass whose `GetDetails()` is empty.
Before this change it fails:
```
.../TestCommandReturnObject.cpp:74: Failure
Expected: (result.GetErrorString().find("boom")) != (std::string::npos),
actual: 18446744073709551615 vs 18446744073709551615
```
---
Full diff: https://github.com/llvm/llvm-project/pull/221770.diff
2 Files Affected:
- (modified) lldb/source/Interpreter/CommandReturnObject.cpp (+9-4)
- (modified) lldb/unittests/Interpreter/TestCommandReturnObject.cpp (+37)
``````````diff
diff --git a/lldb/source/Interpreter/CommandReturnObject.cpp
b/lldb/source/Interpreter/CommandReturnObject.cpp
index c21b9b3a92c05..e56722b0a1e3c 100644
--- a/lldb/source/Interpreter/CommandReturnObject.cpp
+++ b/lldb/source/Interpreter/CommandReturnObject.cpp
@@ -124,10 +124,15 @@ void CommandReturnObject::SetError(Status error) {
void CommandReturnObject::SetError(llvm::Error error) {
// Retrieve any diagnostics.
- error = llvm::handleErrors(std::move(error), [&](DiagnosticError &error) {
- SetStatus(eReturnStatusFailed);
- m_diagnostics = error.GetDetails();
- });
+ error = llvm::handleErrors(
+ std::move(error), [&](DiagnosticError &error) -> llvm::Error {
+ SetStatus(eReturnStatusFailed);
+ m_diagnostics = error.GetDetails();
+ // Return one whenever there are no details to show.
+ if (m_diagnostics.empty())
+ return llvm::createStringError(error.message());
+ return llvm::Error::success();
+ });
if (error) {
AppendError(llvm::toString(std::move(error)));
}
diff --git a/lldb/unittests/Interpreter/TestCommandReturnObject.cpp
b/lldb/unittests/Interpreter/TestCommandReturnObject.cpp
index e8335dd6b45fa..16b267ca00f37 100644
--- a/lldb/unittests/Interpreter/TestCommandReturnObject.cpp
+++ b/lldb/unittests/Interpreter/TestCommandReturnObject.cpp
@@ -7,11 +7,41 @@
//===----------------------------------------------------------------------===//
#include "lldb/Interpreter/CommandReturnObject.h"
+
+#include "lldb/Host/common/DiagnosticsRendering.h"
+#include "llvm/Support/Error.h"
+
#include "gtest/gtest.h"
+#include <system_error>
+
using namespace lldb;
using namespace lldb_private;
+namespace {
+/// A DiagnosticError with no structured DiagnosticDetail entries, used to
+/// exercise the plain-message fallback in CommandReturnObject::SetError.
+class NoDetailsDiagnosticError
+ : public llvm::ErrorInfo<NoDetailsDiagnosticError, DiagnosticError> {
+public:
+ static char ID;
+
+ explicit NoDetailsDiagnosticError(std::string message)
+ : ErrorInfo(std::make_error_code(std::errc::invalid_argument)),
+ m_message(std::move(message)) {}
+
+ std::string message() const override { return m_message; }
+ llvm::ArrayRef<DiagnosticDetail> GetDetails() const override { return {}; }
+ std::unique_ptr<CloneableError> Clone() const override {
+ return std::make_unique<NoDetailsDiagnosticError>(m_message);
+ }
+
+private:
+ std::string m_message;
+};
+char NoDetailsDiagnosticError::ID;
+} // namespace
+
TEST(CommandReturnObjectTest, DefaultStatusIsInvalid) {
CommandReturnObject result(/*colors=*/false);
EXPECT_EQ(result.GetStatus(), eReturnStatusInvalid);
@@ -36,3 +66,10 @@ TEST(CommandReturnObjectTest, ClearResetsToInvalid) {
result.Clear();
EXPECT_EQ(result.GetStatus(), eReturnStatusInvalid);
}
+
+TEST(CommandReturnObjectTest, SetErrorFromDiagnosticErrorWithoutDetails) {
+ CommandReturnObject result(false);
+ result.SetError(llvm::make_error<NoDetailsDiagnosticError>("boom"));
+ EXPECT_EQ(result.GetStatus(), eReturnStatusFailed);
+ EXPECT_NE(result.GetErrorString().find("boom"), std::string::npos);
+}
``````````
</details>
https://github.com/llvm/llvm-project/pull/221770
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
