Skip to content

Commit b798f4b

Browse files
adrian-prantladrian-prantl
authored
[lldb] Make deep copies of Status explicit (NFC) (#107170)
1 parent 53d5ffe commit b798f4b

Some content is hidden

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

57 files changed

+172
-141
lines changed

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@ PythonObject SWIGBridge::ToSWIGWrapper(lldb::BreakpointSP breakpoint_sp) {
3333
SWIGTYPE_p_lldb__SBBreakpoint);
3434
}
3535

36-
PythonObject SWIGBridge::ToSWIGWrapper(const Status& status) {
37-
return ToSWIGHelper(new lldb::SBError(status), SWIGTYPE_p_lldb__SBError);
36+
PythonObject SWIGBridge::ToSWIGWrapper(Status status) {
37+
return ToSWIGHelper(new lldb::SBError(std::move(status)), 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/Utility/Status.h

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,13 +43,32 @@ const char *ExpressionResultAsCString(lldb::ExpressionResults result);
4343
/// of themselves for printing results and error codes. The string value will
4444
/// be fetched on demand and its string value will be cached until the error
4545
/// is cleared of the value of the error changes.
46+
///
47+
/// API design notes:
48+
///
49+
/// Most APIs that currently vend a Status would be better served by
50+
/// returning llvm::Expected<> instead. If possibles APIs should be
51+
/// refactored to avoid Status. The only legitimate long-term uses of
52+
/// Status are objects that need to store an error for a long time
53+
/// (which should be questioned as a design decision, too).
54+
///
55+
/// Implementation notes:
56+
///
57+
/// Internally, Status stores an llvm::Error.
58+
/// eErrorTypeInvalid
59+
/// eErrorTypeGeneric llvm::StringError
60+
/// eErrorTypePOSIX llvm::ECError
61+
/// eErrorTypeMachKernel MachKernelError
62+
/// eErrorTypeExpression llvm::ErrorList<ExpressionError>
63+
/// eErrorTypeWin32 Win32Error
64+
4665
class Status {
4766
public:
48-
/// Every error value that this object can contain needs to be able to fit
4967
/// into ValueType.
5068
typedef uint32_t ValueType;
5169

5270
Status();
71+
Status(Status &&other) = default;
5372

5473
/// Initialize the error object with a generic success value.
5574
///
@@ -93,10 +112,14 @@ class Status {
93112

94113
~Status();
95114

115+
const Status &operator=(Status &&);
96116
/// Avoid using this in new code. Migrate APIs to llvm::Expected instead.
97117
static Status FromError(llvm::Error error);
98-
/// FIXME: Replace this with a takeError method.
118+
/// FIXME: Replace this with a takeError() method.
99119
llvm::Error ToError() const;
120+
/// Don't call this function in new code. Instead, redesign the API
121+
/// to use llvm::Expected instead of Status.
122+
Status Clone() const { return Status(ToError()); }
100123

101124
/// Get the error string associated with the current error.
102125
//

‎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: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -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() {

0 commit comments

Comments
 (0)