Skip to content

Commit 089f8e8

Browse files
adrian-prantladrian-prantl
committed
[lldb] Change the implementation of Status to store an llvm::Error (NFC)
Most APIs that currently vend a Status would be better served by returning llvm::Expected<> instead. If possibles APIs should be refactored to avoid Status. The only legitimate long-term uses of Status are objects that need to store an error for a long time (which should be questioned as a design decision, too). This patch makes the transition to llvm::Error easier by making the places that cannot switch to llvm::Error explicit: They are marked with a call to Status::clone(). Every other API can and should be refactored to use llvm::Expected. In the end Status should only be used in very few places. Whenever an unchecked Error is dropped by Status it logs this to the verbose API channel. Implementation notes: This patch introduces two new kinds of error_category as well as new llvm::Error types. Here is the mapping of lldb::ErrorType to llvm::Errors: (eErrorTypeInvalid) eErrorTypeGeneric llvm::StringError eErrorTypePOSIX llvm::ECError eErrorTypeMachKernel MachKernelError eErrorTypeExpression llvm::ErrorList<ExpressionError> eErrorTypeWin32 Win32Error
1 parent a777a93 commit 089f8e8

File tree

81 files changed

+429
-315
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

81 files changed

+429
-315
lines changed

‎lldb/bindings/python/python-swigsafecast.swig

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ PythonObject SWIGBridge::ToSWIGWrapper(lldb::BreakpointSP breakpoint_sp) {
3434
}
3535

3636
PythonObject SWIGBridge::ToSWIGWrapper(const Status& status) {
37-
return ToSWIGHelper(new lldb::SBError(status), SWIGTYPE_p_lldb__SBError);
37+
return ToSWIGHelper(new lldb::SBError(status.clone()), SWIGTYPE_p_lldb__SBError);
3838
}
3939

4040
PythonObject SWIGBridge::ToSWIGWrapper(std::unique_ptr<lldb::SBStructuredData> data_sb) {

‎lldb/include/lldb/API/SBError.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ class LLDB_API SBError {
9797
friend class lldb_private::ScriptInterpreter;
9898
friend class lldb_private::python::SWIGBridge;
9999

100-
SBError(const lldb_private::Status &error);
100+
SBError(lldb_private::Status &&error);
101101

102102
lldb_private::Status *get();
103103

@@ -107,7 +107,7 @@ class LLDB_API SBError {
107107

108108
lldb_private::Status &ref();
109109

110-
void SetError(const lldb_private::Status &lldb_error);
110+
void SetError(lldb_private::Status &&lldb_error);
111111

112112
private:
113113
std::unique_ptr<lldb_private::Status> m_opaque_up;

‎lldb/include/lldb/API/SBValueList.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ class LLDB_API SBValueList {
9696

9797
std::unique_ptr<ValueListImpl> m_opaque_up;
9898

99-
void SetError(const lldb_private::Status &status);
99+
void SetError(lldb_private::Status &&status);
100100
};
101101

102102
} // namespace lldb

‎lldb/include/lldb/Core/ValueObjectConstResult.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ class ValueObjectConstResult : public ValueObject {
6161

6262
// When an expression fails to evaluate, we return an error
6363
static lldb::ValueObjectSP Create(ExecutionContextScope *exe_scope,
64-
const Status &error);
64+
Status &&error);
6565

6666
std::optional<uint64_t> GetByteSize() override;
6767

@@ -146,7 +146,7 @@ class ValueObjectConstResult : public ValueObject {
146146
ConstString name, Module *module = nullptr);
147147

148148
ValueObjectConstResult(ExecutionContextScope *exe_scope,
149-
ValueObjectManager &manager, const Status &error);
149+
ValueObjectManager &manager, Status &&error);
150150

151151
ValueObject *CreateChildAtIndex(size_t idx) override {
152152
return m_impl.CreateChildAtIndex(idx);

‎lldb/include/lldb/Target/Process.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1296,8 +1296,6 @@ class Process : public std::enable_shared_from_this<Process>,
12961296
const EvaluateExpressionOptions &options,
12971297
DiagnosticManager &diagnostic_manager);
12981298

1299-
static const char *ExecutionResultAsCString(lldb::ExpressionResults result);
1300-
13011299
void GetStatus(Stream &ostrm);
13021300

13031301
size_t GetThreadStatus(Stream &ostrm, bool only_threads_with_stop_reason,

‎lldb/include/lldb/Utility/Status.h

Lines changed: 57 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,34 @@ class raw_ostream;
2626

2727
namespace lldb_private {
2828

29+
class MachKernelError
30+
: public llvm::ErrorInfo<MachKernelError, llvm::StringError> {
31+
public:
32+
using llvm::ErrorInfo<MachKernelError, llvm::StringError>::ErrorInfo;
33+
MachKernelError(std::error_code ec, const llvm::Twine &msg = {})
34+
: ErrorInfo(msg, ec) {}
35+
static char ID;
36+
};
37+
38+
class Win32Error : public llvm::ErrorInfo<Win32Error, llvm::StringError> {
39+
public:
40+
using llvm::ErrorInfo<Win32Error, llvm::StringError>::ErrorInfo;
41+
Win32Error(std::error_code ec, const llvm::Twine &msg = {})
42+
: ErrorInfo(msg, ec) {}
43+
static char ID;
44+
};
45+
46+
class ExpressionError
47+
: public llvm::ErrorInfo<ExpressionError, llvm::StringError> {
48+
public:
49+
using llvm::ErrorInfo<ExpressionError, llvm::StringError>::ErrorInfo;
50+
ExpressionError(std::error_code ec, std::string msg = {})
51+
: ErrorInfo(msg, ec) {}
52+
static char ID;
53+
};
54+
55+
const char *ExecutionResultAsCString(lldb::ExpressionResults result);
56+
2957
/// \class Status Status.h "lldb/Utility/Status.h" An error handling class.
3058
///
3159
/// This class is designed to be able to hold any error code that can be
@@ -41,13 +69,32 @@ namespace lldb_private {
4169
/// of themselves for printing results and error codes. The string value will
4270
/// be fetched on demand and its string value will be cached until the error
4371
/// is cleared of the value of the error changes.
72+
///
73+
/// API design notes:
74+
///
75+
/// Most APIs that currently vend a Status would be better served by
76+
/// returning llvm::Expected<> instead. If possibles APIs should be
77+
/// refactored to avoid Status. The only legitimate long-term uses of
78+
/// Status are objects that need to store an error for a long time
79+
/// (which should be questioned as a design decision, too).
80+
///
81+
/// Implementation notes:
82+
///
83+
/// Internally, Status stores an llvm::Error.
84+
/// eErrorTypeInvalid
85+
/// eErrorTypeGeneric llvm::StringError
86+
/// eErrorTypePOSIX llvm::ECError
87+
/// eErrorTypeMachKernel MachKernelError
88+
/// eErrorTypeExpression llvm::ErrorList<ExpressionError>
89+
/// eErrorTypeWin32 Win32Error
90+
4491
class Status {
4592
public:
46-
/// Every error value that this object can contain needs to be able to fit
4793
/// into ValueType.
4894
typedef uint32_t ValueType;
4995

5096
Status();
97+
Status(Status &&other) = default;
5198

5299
/// Initialize the error object with a generic success value.
53100
///
@@ -79,9 +126,7 @@ class Status {
79126
}
80127

81128
static Status FromExpressionError(lldb::ExpressionResults result,
82-
std::string msg) {
83-
return Status(result, lldb::eErrorTypeExpression, msg);
84-
}
129+
std::string msg);
85130

86131
/// Set the current error to errno.
87132
///
@@ -91,10 +136,14 @@ class Status {
91136

92137
~Status();
93138

94-
// llvm::Error support
95-
explicit Status(llvm::Error error) { *this = std::move(error); }
139+
/// Avoid using this in new code. Migrate APIs to llvm::Expected instead.
140+
static Status FromError(llvm::Error &&error);
96141
const Status &operator=(llvm::Error error);
142+
Status &operator=(Status &&);
143+
/// FIXME: Replace this with a takeError() method.
97144
llvm::Error ToError() const;
145+
/// Don't call this function in new code. Redesign the API instead.
146+
Status clone() const { return Status(ToError()); }
98147

99148
/// Get the error string associated with the current error.
100149
//
@@ -145,11 +194,8 @@ class Status {
145194
bool Success() const;
146195

147196
protected:
148-
/// Status code as an integer value.
149-
ValueType m_code = 0;
150-
/// The type of the above error code.
151-
lldb::ErrorType m_type = lldb::eErrorTypeInvalid;
152-
/// A string representation of the error code.
197+
Status(llvm::Error &&error) : m_error(std::move(error)) {}
198+
mutable llvm::Error m_error;
153199
mutable std::string m_string;
154200
};
155201

‎lldb/source/API/SBBreakpoint.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -622,7 +622,7 @@ SBError SBBreakpoint::SetScriptCallbackFunction(
622622
callback_function_name,
623623
extra_args.m_impl_up
624624
->GetObjectSP());
625-
sb_error.SetError(error);
625+
sb_error.SetError(std::move(error));
626626
} else
627627
sb_error = Status::FromErrorString("invalid breakpoint");
628628

@@ -645,7 +645,7 @@ SBError SBBreakpoint::SetScriptCallbackBody(const char *callback_body_text) {
645645
.GetScriptInterpreter()
646646
->SetBreakpointCommandCallback(bp_options, callback_body_text,
647647
/*is_callback=*/false);
648-
sb_error.SetError(error);
648+
sb_error.SetError(std::move(error));
649649
} else
650650
sb_error = Status::FromErrorString("invalid breakpoint");
651651

@@ -670,7 +670,7 @@ SBError SBBreakpoint::AddNameWithErrorHandling(const char *new_name) {
670670
bkpt_sp->GetTarget().GetAPIMutex());
671671
Status error;
672672
bkpt_sp->GetTarget().AddNameToBreakpoint(bkpt_sp, new_name, error);
673-
status.SetError(error);
673+
status.SetError(std::move(error));
674674
} else {
675675
status = Status::FromErrorString("invalid breakpoint");
676676
}

‎lldb/source/API/SBBreakpointLocation.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,7 @@ SBError SBBreakpointLocation::SetScriptCallbackFunction(
239239
callback_function_name,
240240
extra_args.m_impl_up
241241
->GetObjectSP());
242-
sb_error.SetError(error);
242+
sb_error.SetError(std::move(error));
243243
} else
244244
sb_error = Status::FromErrorString("invalid breakpoint");
245245

@@ -264,7 +264,7 @@ SBBreakpointLocation::SetScriptCallbackBody(const char *callback_body_text) {
264264
.GetScriptInterpreter()
265265
->SetBreakpointCommandCallback(bp_options, callback_body_text,
266266
/*is_callback=*/false);
267-
sb_error.SetError(error);
267+
sb_error.SetError(std::move(error));
268268
} else
269269
sb_error = Status::FromErrorString("invalid breakpoint");
270270

‎lldb/source/API/SBBreakpointName.cpp

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -570,14 +570,13 @@ SBError SBBreakpointName::SetScriptCallbackFunction(
570570
m_impl_up->GetTarget()->GetAPIMutex());
571571

572572
BreakpointOptions &bp_options = bp_name->GetOptions();
573-
Status error;
574-
error = m_impl_up->GetTarget()
575-
->GetDebugger()
576-
.GetScriptInterpreter()
577-
->SetBreakpointCommandCallbackFunction(
578-
bp_options, callback_function_name,
579-
extra_args.m_impl_up->GetObjectSP());
580-
sb_error.SetError(error);
573+
Status error = m_impl_up->GetTarget()
574+
->GetDebugger()
575+
.GetScriptInterpreter()
576+
->SetBreakpointCommandCallbackFunction(
577+
bp_options, callback_function_name,
578+
extra_args.m_impl_up->GetObjectSP());
579+
sb_error.SetError(std::move(error));
581580
UpdateName(*bp_name);
582581
return sb_error;
583582
}
@@ -600,7 +599,7 @@ SBBreakpointName::SetScriptCallbackBody(const char *callback_body_text) {
600599
.GetScriptInterpreter()
601600
->SetBreakpointCommandCallback(
602601
bp_options, callback_body_text, /*is_callback=*/false);
603-
sb_error.SetError(error);
602+
sb_error.SetError(std::move(error));
604603
if (!sb_error.Fail())
605604
UpdateName(*bp_name);
606605

‎lldb/source/API/SBDebugger.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ lldb::SBError SBDebugger::InitializeWithErrorHandling() {
220220
SBError error;
221221
if (auto e = g_debugger_lifetime->Initialize(
222222
std::make_unique<SystemInitializerFull>(), LoadPlugin)) {
223-
error.SetError(Status(std::move(e)));
223+
error.SetError(Status::FromError(std::move(e)));
224224
}
225225
return error;
226226
}
@@ -1360,7 +1360,7 @@ SBError SBDebugger::SetInternalVariable(const char *var_name, const char *value,
13601360
"invalid debugger instance name '%s'", debugger_instance_name);
13611361
}
13621362
if (error.Fail())
1363-
sb_error.SetError(error);
1363+
sb_error.SetError(std::move(error));
13641364
return sb_error;
13651365
}
13661366

‎lldb/source/API/SBError.cpp

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ SBError::SBError() { LLDB_INSTRUMENT_VA(this); }
2323
SBError::SBError(const SBError &rhs) {
2424
LLDB_INSTRUMENT_VA(this, rhs);
2525

26-
m_opaque_up = clone(rhs.m_opaque_up);
26+
if (rhs.m_opaque_up)
27+
m_opaque_up = std::make_unique<Status>(rhs.m_opaque_up->clone());
2728
}
2829

2930
SBError::SBError(const char *message) {
@@ -32,8 +33,8 @@ SBError::SBError(const char *message) {
3233
SetErrorString(message);
3334
}
3435

35-
SBError::SBError(const lldb_private::Status &status)
36-
: m_opaque_up(new Status(status)) {
36+
SBError::SBError(lldb_private::Status &&status)
37+
: m_opaque_up(new Status(std::move(status))) {
3738
LLDB_INSTRUMENT_VA(this, status);
3839
}
3940

@@ -43,7 +44,9 @@ const SBError &SBError::operator=(const SBError &rhs) {
4344
LLDB_INSTRUMENT_VA(this, rhs);
4445

4546
if (this != &rhs)
46-
m_opaque_up = clone(rhs.m_opaque_up);
47+
if (rhs.m_opaque_up)
48+
m_opaque_up = std::make_unique<Status>(rhs.m_opaque_up->clone());
49+
4750
return *this;
4851
}
4952

@@ -111,9 +114,9 @@ void SBError::SetError(uint32_t err, ErrorType type) {
111114
*m_opaque_up = Status(err, type);
112115
}
113116

114-
void SBError::SetError(const Status &lldb_error) {
117+
void SBError::SetError(Status &&lldb_error) {
115118
CreateIfNeeded();
116-
*m_opaque_up = lldb_error;
119+
*m_opaque_up = std::move(lldb_error);
117120
}
118121

119122
void SBError::SetErrorToErrno() {

‎lldb/source/API/SBFile.cpp

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,7 @@ SBError SBFile::Read(uint8_t *buf, size_t num_bytes, size_t *bytes_read) {
6262
error = Status::FromErrorString("invalid SBFile");
6363
*bytes_read = 0;
6464
} else {
65-
Status status = m_opaque_sp->Read(buf, num_bytes);
66-
error.SetError(status);
65+
error.SetError(m_opaque_sp->Read(buf, num_bytes));
6766
*bytes_read = num_bytes;
6867
}
6968
return error;
@@ -78,8 +77,7 @@ SBError SBFile::Write(const uint8_t *buf, size_t num_bytes,
7877
error = Status::FromErrorString("invalid SBFile");
7978
*bytes_written = 0;
8079
} else {
81-
Status status = m_opaque_sp->Write(buf, num_bytes);
82-
error.SetError(status);
80+
error.SetError(m_opaque_sp->Write(buf, num_bytes));
8381
*bytes_written = num_bytes;
8482
}
8583
return error;
@@ -92,8 +90,7 @@ SBError SBFile::Flush() {
9290
if (!m_opaque_sp) {
9391
error = Status::FromErrorString("invalid SBFile");
9492
} else {
95-
Status status = m_opaque_sp->Flush();
96-
error.SetError(status);
93+
error.SetError(m_opaque_sp->Flush());
9794
}
9895
return error;
9996
}
@@ -106,10 +103,8 @@ bool SBFile::IsValid() const {
106103
SBError SBFile::Close() {
107104
LLDB_INSTRUMENT_VA(this);
108105
SBError error;
109-
if (m_opaque_sp) {
110-
Status status = m_opaque_sp->Close();
111-
error.SetError(status);
112-
}
106+
if (m_opaque_sp)
107+
error.SetError(m_opaque_sp->Close());
113108
return error;
114109
}
115110

‎lldb/source/API/SBFormat.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ SBFormat::SBFormat(const char *format, lldb::SBError &error) {
3636
FormatEntrySP format_entry_sp = std::make_shared<FormatEntity::Entry>();
3737
Status status = FormatEntity::Parse(format, *format_entry_sp);
3838

39-
error.SetError(status);
39+
error.SetError(std::move(status));
4040
if (error.Success())
4141
m_opaque_sp = format_entry_sp;
4242
}

‎lldb/source/API/SBFrame.cpp

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -809,7 +809,7 @@ SBValueList SBFrame::GetVariables(const lldb::SBVariablesOptions &options) {
809809
Status var_error;
810810
variable_list = frame->GetVariableList(true, &var_error);
811811
if (var_error.Fail())
812-
value_list.SetError(var_error);
812+
value_list.SetError(std::move(var_error));
813813
if (variable_list) {
814814
const size_t num_variables = variable_list->GetSize();
815815
if (num_variables) {
@@ -1033,7 +1033,8 @@ SBValue SBFrame::EvaluateExpression(const char *expr) {
10331033
Status error;
10341034
error = Status::FromErrorString("can't evaluate expressions when the "
10351035
"process is running.");
1036-
ValueObjectSP error_val_sp = ValueObjectConstResult::Create(nullptr, error);
1036+
ValueObjectSP error_val_sp =
1037+
ValueObjectConstResult::Create(nullptr, std::move(error));
10371038
result.SetSP(error_val_sp, false);
10381039
}
10391040
return result;
@@ -1129,13 +1130,13 @@ lldb::SBValue SBFrame::EvaluateExpression(const char *expr,
11291130
Status error;
11301131
error = Status::FromErrorString("can't evaluate expressions when the "
11311132
"process is running.");
1132-
expr_value_sp = ValueObjectConstResult::Create(nullptr, error);
1133+
expr_value_sp = ValueObjectConstResult::Create(nullptr, std::move(error));
11331134
expr_result.SetSP(expr_value_sp, false);
11341135
}
11351136
} else {
11361137
Status error;
11371138
error = Status::FromErrorString("sbframe object is not valid.");
1138-
expr_value_sp = ValueObjectConstResult::Create(nullptr, error);
1139+
expr_value_sp = ValueObjectConstResult::Create(nullptr, std::move(error));
11391140
expr_result.SetSP(expr_value_sp, false);
11401141
}
11411142

0 commit comments

Comments
 (0)