[lldb] Make conversions from llvm::Error explicit with Status::FromEr… #107163
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adrian-prantl
requested review from
bulbazord and
medismailben
and removed request for
JDevlieghere and
bulbazord
|
@llvm/pr-subscribers-lldb Author: Adrian Prantl (adrian-prantl) Changes…ror() [NFC] Broken out from #106774 for easier review. Patch is 52.99 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/107163.diff 45 Files Affected:
diff --git a/lldb/include/lldb/Utility/Status.h b/lldb/include/lldb/Utility/Status.h
index b304291ffae00e..aaf082648c0889 100644
--- a/lldb/include/lldb/Utility/Status.h
+++ b/lldb/include/lldb/Utility/Status.h
@@ -91,9 +91,8 @@ class Status {
~Status();
- // llvm::Error support
- explicit Status(llvm::Error error) { *this = std::move(error); }
- const Status &operator=(llvm::Error error);
+ /// Avoid using this in new code. Migrate APIs to llvm::Expected instead.
+ static Status FromError(llvm::Error &&error);
llvm::Error ToError() const;
/// Get the error string associated with the current error.
@@ -145,6 +144,7 @@ class Status {
bool Success() const;
protected:
+ Status(llvm::Error &&);
/// Status code as an integer value.
ValueType m_code = 0;
/// The type of the above error code.
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 72501570320d57..c226acc15018ef 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -220,7 +220,7 @@ lldb::SBError SBDebugger::InitializeWithErrorHandling() {
SBError error;
if (auto e = g_debugger_lifetime->Initialize(
std::make_unique<SystemInitializerFull>(), LoadPlugin)) {
- error.SetError(Status(std::move(e)));
+ error.SetError(Status::FromError(std::move(e)));
}
return error;
}
diff --git a/lldb/source/API/SBTarget.cpp b/lldb/source/API/SBTarget.cpp
index e927cb854cd88c..41eb77e5506bc5 100644
--- a/lldb/source/API/SBTarget.cpp
+++ b/lldb/source/API/SBTarget.cpp
@@ -1658,7 +1658,7 @@ SBError SBTarget::SetLabel(const char *label) {
if (!target_sp)
return Status::FromErrorString("Couldn't get internal target object.");
- return Status(target_sp->SetLabel(label));
+ return Status::FromError(target_sp->SetLabel(label));
}
uint32_t SBTarget::GetDataByteSize() {
diff --git a/lldb/source/Commands/CommandObjectBreakpoint.cpp b/lldb/source/Commands/CommandObjectBreakpoint.cpp
index ede3dd2f2a864c..494d6c50e94ac3 100644
--- a/lldb/source/Commands/CommandObjectBreakpoint.cpp
+++ b/lldb/source/Commands/CommandObjectBreakpoint.cpp
@@ -89,14 +89,16 @@ class lldb_private::BreakpointOptionGroup : public OptionGroup {
if (success)
m_bp_opts.SetAutoContinue(value);
else
- error = CreateOptionParsingError(option_arg, short_option, long_option,
- g_bool_parsing_error_message);
+ error = Status::FromError(
+ CreateOptionParsingError(option_arg, short_option, long_option,
+ g_bool_parsing_error_message));
} break;
case 'i': {
uint32_t ignore_count;
if (option_arg.getAsInteger(0, ignore_count))
- error = CreateOptionParsingError(option_arg, short_option, long_option,
- g_int_parsing_error_message);
+ error = Status::FromError(
+ CreateOptionParsingError(option_arg, short_option, long_option,
+ g_int_parsing_error_message));
else
m_bp_opts.SetIgnoreCount(ignore_count);
} break;
@@ -106,29 +108,31 @@ class lldb_private::BreakpointOptionGroup : public OptionGroup {
if (success) {
m_bp_opts.SetOneShot(value);
} else
- error = CreateOptionParsingError(option_arg, short_option, long_option,
- g_bool_parsing_error_message);
+ error = Status::FromError(
+ CreateOptionParsingError(option_arg, short_option, long_option,
+ g_bool_parsing_error_message));
} break;
case 't': {
lldb::tid_t thread_id = LLDB_INVALID_THREAD_ID;
if (option_arg == "current") {
if (!execution_context) {
- error = CreateOptionParsingError(
+ error = Status::FromError(CreateOptionParsingError(
option_arg, short_option, long_option,
- "No context to determine current thread");
+ "No context to determine current thread"));
} else {
ThreadSP ctx_thread_sp = execution_context->GetThreadSP();
if (!ctx_thread_sp || !ctx_thread_sp->IsValid()) {
- error =
+ error = Status::FromError(
CreateOptionParsingError(option_arg, short_option, long_option,
- "No currently selected thread");
+ "No currently selected thread"));
} else {
thread_id = ctx_thread_sp->GetID();
}
}
} else if (option_arg.getAsInteger(0, thread_id)) {
- error = CreateOptionParsingError(option_arg, short_option, long_option,
- g_int_parsing_error_message);
+ error = Status::FromError(
+ CreateOptionParsingError(option_arg, short_option, long_option,
+ g_int_parsing_error_message));
}
if (thread_id != LLDB_INVALID_THREAD_ID)
m_bp_opts.SetThreadID(thread_id);
@@ -142,8 +146,9 @@ class lldb_private::BreakpointOptionGroup : public OptionGroup {
case 'x': {
uint32_t thread_index = UINT32_MAX;
if (option_arg.getAsInteger(0, thread_index)) {
- error = CreateOptionParsingError(option_arg, short_option, long_option,
- g_int_parsing_error_message);
+ error = Status::FromError(
+ CreateOptionParsingError(option_arg, short_option, long_option,
+ g_int_parsing_error_message));
} else {
m_bp_opts.GetThreadSpec()->SetIndex(thread_index);
}
@@ -286,9 +291,9 @@ class CommandObjectBreakpointSet : public CommandObjectParsed {
case 'u':
if (option_arg.getAsInteger(0, m_column))
- error =
+ error = Status::FromError(
CreateOptionParsingError(option_arg, short_option, long_option,
- g_int_parsing_error_message);
+ g_int_parsing_error_message));
break;
case 'E': {
@@ -326,8 +331,8 @@ class CommandObjectBreakpointSet : public CommandObjectParsed {
error_context = "Unsupported language type for exception breakpoint";
}
if (!error_context.empty())
- error = CreateOptionParsingError(option_arg, short_option,
- long_option, error_context);
+ error = Status::FromError(CreateOptionParsingError(
+ option_arg, short_option, long_option, error_context));
} break;
case 'f':
@@ -343,9 +348,9 @@ class CommandObjectBreakpointSet : public CommandObjectParsed {
bool success;
m_catch_bp = OptionArgParser::ToBoolean(option_arg, true, &success);
if (!success)
- error =
+ error = Status::FromError(
CreateOptionParsingError(option_arg, short_option, long_option,
- g_bool_parsing_error_message);
+ g_bool_parsing_error_message));
} break;
case 'H':
@@ -362,24 +367,24 @@ class CommandObjectBreakpointSet : public CommandObjectParsed {
m_skip_prologue = eLazyBoolNo;
if (!success)
- error =
+ error = Status::FromError(
CreateOptionParsingError(option_arg, short_option, long_option,
- g_bool_parsing_error_message);
+ g_bool_parsing_error_message));
} break;
case 'l':
if (option_arg.getAsInteger(0, m_line_num))
- error =
+ error = Status::FromError(
CreateOptionParsingError(option_arg, short_option, long_option,
- g_int_parsing_error_message);
+ g_int_parsing_error_message));
break;
case 'L':
m_language = Language::GetLanguageTypeFromString(option_arg);
if (m_language == eLanguageTypeUnknown)
- error =
+ error = Status::FromError(
CreateOptionParsingError(option_arg, short_option, long_option,
- g_language_parsing_error_message);
+ g_language_parsing_error_message));
break;
case 'm': {
@@ -392,9 +397,9 @@ class CommandObjectBreakpointSet : public CommandObjectParsed {
m_move_to_nearest_code = eLazyBoolNo;
if (!success)
- error =
+ error = Status::FromError(
CreateOptionParsingError(option_arg, short_option, long_option,
- g_bool_parsing_error_message);
+ g_bool_parsing_error_message));
break;
}
@@ -412,8 +417,9 @@ class CommandObjectBreakpointSet : public CommandObjectParsed {
if (BreakpointID::StringIsBreakpointName(option_arg, error))
m_breakpoint_names.push_back(std::string(option_arg));
else
- error = CreateOptionParsingError(
- option_arg, short_option, long_option, "Invalid breakpoint name");
+ error = Status::FromError(
+ CreateOptionParsingError(option_arg, short_option, long_option,
+ "Invalid breakpoint name"));
break;
}
@@ -451,9 +457,9 @@ class CommandObjectBreakpointSet : public CommandObjectParsed {
bool success;
m_throw_bp = OptionArgParser::ToBoolean(option_arg, true, &success);
if (!success)
- error =
+ error = Status::FromError(
CreateOptionParsingError(option_arg, short_option, long_option,
- g_bool_parsing_error_message);
+ g_bool_parsing_error_message));
} break;
case 'X':
@@ -465,8 +471,8 @@ class CommandObjectBreakpointSet : public CommandObjectParsed {
OptionValueFileColonLine value;
Status fcl_err = value.SetValueFromString(option_arg);
if (!fcl_err.Success()) {
- error = CreateOptionParsingError(option_arg, short_option,
- long_option, fcl_err.AsCString());
+ error = Status::FromError(CreateOptionParsingError(
+ option_arg, short_option, long_option, fcl_err.AsCString()));
} else {
m_filenames.AppendIfUnique(value.GetFileSpec());
m_line_num = value.GetLineNumber();
@@ -1551,13 +1557,15 @@ class BreakpointNameOptionGroup : public OptionGroup {
break;
case 'B':
if (m_breakpoint.SetValueFromString(option_arg).Fail())
- error = CreateOptionParsingError(option_arg, short_option, long_option,
- g_int_parsing_error_message);
+ error = Status::FromError(
+ CreateOptionParsingError(option_arg, short_option, long_option,
+ g_int_parsing_error_message));
break;
case 'D':
if (m_use_dummy.SetValueFromString(option_arg).Fail())
- error = CreateOptionParsingError(option_arg, short_option, long_option,
- g_bool_parsing_error_message);
+ error = Status::FromError(
+ CreateOptionParsingError(option_arg, short_option, long_option,
+ g_bool_parsing_error_message));
break;
case 'H':
m_help_string.SetValueFromString(option_arg);
@@ -1610,8 +1618,9 @@ class BreakpointAccessOptionGroup : public OptionGroup {
if (success) {
m_permissions.SetAllowList(value);
} else
- error = CreateOptionParsingError(option_arg, short_option, long_option,
- g_bool_parsing_error_message);
+ error = Status::FromError(
+ CreateOptionParsingError(option_arg, short_option, long_option,
+ g_bool_parsing_error_message));
} break;
case 'A': {
bool value, success;
@@ -1619,8 +1628,9 @@ class BreakpointAccessOptionGroup : public OptionGroup {
if (success) {
m_permissions.SetAllowDisable(value);
} else
- error = CreateOptionParsingError(option_arg, short_option, long_option,
- g_bool_parsing_error_message);
+ error = Status::FromError(
+ CreateOptionParsingError(option_arg, short_option, long_option,
+ g_bool_parsing_error_message));
} break;
case 'D': {
bool value, success;
@@ -1628,8 +1638,9 @@ class BreakpointAccessOptionGroup : public OptionGroup {
if (success) {
m_permissions.SetAllowDelete(value);
} else
- error = CreateOptionParsingError(option_arg, short_option, long_option,
- g_bool_parsing_error_message);
+ error = Status::FromError(
+ CreateOptionParsingError(option_arg, short_option, long_option,
+ g_bool_parsing_error_message));
} break;
default:
llvm_unreachable("Unimplemented option");
@@ -2113,8 +2124,8 @@ class CommandObjectBreakpointRead : public CommandObjectParsed {
Status name_error;
if (!BreakpointID::StringIsBreakpointName(llvm::StringRef(option_arg),
name_error)) {
- error = CreateOptionParsingError(option_arg, short_option,
- long_option, name_error.AsCString());
+ error = Status::FromError(CreateOptionParsingError(
+ option_arg, short_option, long_option, name_error.AsCString()));
}
m_names.push_back(std::string(option_arg));
break;
diff --git a/lldb/source/Commands/CommandObjectMemoryTag.cpp b/lldb/source/Commands/CommandObjectMemoryTag.cpp
index f45d6eacab3d0e..bc76319018da96 100644
--- a/lldb/source/Commands/CommandObjectMemoryTag.cpp
+++ b/lldb/source/Commands/CommandObjectMemoryTag.cpp
@@ -77,7 +77,7 @@ class CommandObjectMemoryTagRead : public CommandObjectParsed {
process->GetMemoryTagManager();
if (!tag_manager_or_err) {
- result.SetError(Status(tag_manager_or_err.takeError()));
+ result.SetError(Status::FromError(tag_manager_or_err.takeError()));
return;
}
@@ -102,7 +102,7 @@ class CommandObjectMemoryTagRead : public CommandObjectParsed {
tag_manager->MakeTaggedRange(start_addr, end_addr, memory_regions);
if (!tagged_range) {
- result.SetError(Status(tagged_range.takeError()));
+ result.SetError(Status::FromError(tagged_range.takeError()));
return;
}
@@ -110,7 +110,7 @@ class CommandObjectMemoryTagRead : public CommandObjectParsed {
tagged_range->GetRangeBase(), tagged_range->GetByteSize());
if (!tags) {
- result.SetError(Status(tags.takeError()));
+ result.SetError(Status::FromError(tags.takeError()));
return;
}
@@ -230,7 +230,7 @@ class CommandObjectMemoryTagWrite : public CommandObjectParsed {
process->GetMemoryTagManager();
if (!tag_manager_or_err) {
- result.SetError(Status(tag_manager_or_err.takeError()));
+ result.SetError(Status::FromError(tag_manager_or_err.takeError()));
return;
}
@@ -282,7 +282,7 @@ class CommandObjectMemoryTagWrite : public CommandObjectParsed {
memory_regions);
if (!tagged_range) {
- result.SetError(Status(tagged_range.takeError()));
+ result.SetError(Status::FromError(tagged_range.takeError()));
return;
}
diff --git a/lldb/source/Commands/CommandObjectStats.cpp b/lldb/source/Commands/CommandObjectStats.cpp
index 53855e7d03165c..7d333afc231ba1 100644
--- a/lldb/source/Commands/CommandObjectStats.cpp
+++ b/lldb/source/Commands/CommandObjectStats.cpp
@@ -87,21 +87,21 @@ class CommandObjectStatsDump : public CommandObjectParsed {
OptionArgParser::ToBoolean("--targets", option_arg))
m_stats_options.SetIncludeTargets(*bool_or_error);
else
- error = bool_or_error.takeError();
+ error = Status::FromError(bool_or_error.takeError());
break;
case 'm':
if (llvm::Expected<bool> bool_or_error =
OptionArgParser::ToBoolean("--modules", option_arg))
m_stats_options.SetIncludeModules(*bool_or_error);
else
- error = bool_or_error.takeError();
+ error = Status::FromError(bool_or_error.takeError());
break;
case 't':
if (llvm::Expected<bool> bool_or_error =
OptionArgParser::ToBoolean("--transcript", option_arg))
m_stats_options.SetIncludeTranscript(*bool_or_error);
else
- error = bool_or_error.takeError();
+ error = Status::FromError(bool_or_error.takeError());
break;
default:
llvm_unreachable("Unimplemented option");
diff --git a/lldb/source/Commands/CommandObjectTrace.cpp b/lldb/source/Commands/CommandObjectTrace.cpp
index 5bcbc236301cc1..5e212e05461a61 100644
--- a/lldb/source/Commands/CommandObjectTrace.cpp
+++ b/lldb/source/Commands/CommandObjectTrace.cpp
@@ -361,7 +361,7 @@ class CommandObjectTraceSchema : public CommandObjectParsed {
Trace::FindPluginSchema(plugin_name))
result.AppendMessage(*schemaOrErr);
else
- error = schemaOrErr.takeError();
+ error = Status::FromError(schemaOrErr.takeError());
}
if (error.Success()) {
diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp
index fd5cb792c101a4..a5219025495a91 100644
--- a/lldb/source/Core/PluginManager.cpp
+++ b/lldb/source/Core/PluginManager.cpp
@@ -723,7 +723,7 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp,
llvm::Expected<bool> ret =
process_sp->SaveCore(options.GetOutputFile()->GetPath());
if (!ret)
- return Status(ret.takeError());
+ return Status::FromError(ret.takeError());
if (ret.get())
return Status();
}
diff --git a/lldb/source/Core/ThreadedCommunication.cpp b/lldb/source/Core/ThreadedCommunication.cpp
index d8b567c9bd0de6..649ce71c293740 100644
--- a/lldb/source/Core/ThreadedCommunication.cpp
+++ b/lldb/source/Core/ThreadedCommunication.cpp
@@ -178,7 +178,7 @@ bool ThreadedCommunication::StartReadThread(Status *error_ptr) {
m_read_thread = *maybe_thread;
} else {
if (error_ptr)
- *error_ptr = Status(maybe_thread.takeError());
+ *error_ptr = Status::FromError(maybe_thread.takeError());
else {
LLDB_LOG_ERROR(GetLog(LLDBLog::Host), maybe_thread.takeError(),
"failed to launch host thread: {0}");
diff --git a/lldb/source/Core/ValueObjectVTable.cpp b/lldb/source/Core/ValueObjectVTable.cpp
index 66e0750b63f8f9..e38f0a83df9940 100644
--- a/lldb/source/Core/ValueObjectVTable.cpp
+++ b/lldb/source/Core/ValueObjectVTable.cpp
@@ -220,7 +220,7 @@ bool ValueObjectVTable::UpdateValue() {
llvm::Expected<LanguageRuntime::VTableInfo> vtable_info_or_err =
language_runtime->GetVTableInfo(*parent, /*check_type=*/true);
if (!vtable_info_or_err) {
- m_error = vtable_info_or_err.takeError();
+ m_error = Status::FromError(vtable_info_or_err.takeError());
return false;
}
diff --git a/lldb/source/Core/ValueObjectVariable.cpp b/lldb/source/Core/ValueObjectVariable.cpp
index 01f871a6f8bc49..29aefb270c92c8 100644
--- a/lldb/source/Core/ValueObjectVariable.cpp
+++ b/lldb/source/Core/ValueObjectVariable.cpp
@@ -249,7 +249,7 @@ bool ValueObjectVariable::UpdateValue() {
SetValueIsValid(m_error.Success());
} else {
- m_error = maybe_value.takeError();
+ m_error = Status::FromError(maybe_value.takeError());
// could not find location, won't allow editing
m_resolved_value.SetContext(Value::ContextType::Invalid, nullptr);
}
diff --git a/lldb/source/DataFormatters/VectorType.cpp b/lldb/source/DataFormatters/VectorType.cpp
index 19de204c24353a..f6c38f76fea31f 100644
--- a/lldb/source/DataFormatters/VectorType.cpp
+++ b/lldb/source/...
[truncated]
|
adrian-prantl
force-pushed
the
status-explicit-fromerror
branch
from
012b0a7 to
98a5687
Compare
labath
reviewed
Sep 4, 2024
There was a problem hiding this comment.
I don't think this usage of rvalue references is very idiomatic. It saves an object copy (move), but:
- that should be very cheap anyway
- it increases the risk of improper handling and weird side-effects -- a function like
void use(Error E) {}will always crash at the closing bracket (due to not checking the error), butuse(Error &&E){}will never crash inside the function (because it doesn't own the error), and whether the program as a whole crashes depends on whether the caller does any other operation that would clear the error object.
So, I think it would be better to stick to value semantics. Other than that, this seems fine.
@labath Are you suggesting to replace all assignments of Status (everything in this patch) with calls to Clone()? In #106774 I'll change Status to wrap llvm::Error(), which can only be moved. While the end-goal is to replace most uses of Status with llvm::Error, we're going to be stuck with a lot of APIs that use Status during the transition and that would be a lot of unnecessary copying. And in all APIs we convert to llvm::Error, we'll also have to use the move semantics for assignment. |
labath
reviewed
Sep 4, 2024
adrian-prantl
force-pushed
the
status-explicit-fromerror
branch
from
98a5687 to
3a13386
Compare
|
@labath Done. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
adrian-prantl
force-pushed
the
status-explicit-fromerror
branch
from
3a13386 to
1c258c6
Compare
labath
approved these changes
Sep 5, 2024
adrian-prantl
force-pushed
the
status-explicit-fromerror
branch
from
1c258c6 to
a56b0ef
Compare
|
Looks like this breaks building on windows: http://45.33.8.238/win/93597/step_3.txt Please take a look and revert for now if it takes a while to fix. |
kusmour
added a commit
to kusmour/llvm-project
that referenced
this pull request
Sep 14, 2024
…atus::FromError Summary: This introduced from upstream llvm#107163 Test Plan: I can build
kusmour
added a commit
to kusmour/llvm-project
that referenced
this pull request
Sep 16, 2024
…atus::FromError Summary: This introduced from upstream llvm#107163 Test Plan: I can build
kusmour
added a commit
that referenced
this pull request
Sep 17, 2024
hamphet
pushed a commit
to hamphet/llvm-project
that referenced
this pull request
Sep 18, 2024
…atus::FromError (llvm#108719) Summary: This introduced from upstream [llvm#107163](llvm#107163) Test Plan: I can build Closes: llvm#107580
tmsri
pushed a commit
to tmsri/llvm-project
that referenced
this pull request
Sep 19, 2024
…atus::FromError (llvm#108719) Summary: This introduced from upstream [llvm#107163](llvm#107163) Test Plan: I can build Closes: llvm#107580
adrian-prantl
added a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Oct 11, 2024
llvm#107163) …ror() [NFC] (cherry picked from commit a0dd90e)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…ror() [NFC]
Broken out from #106774 for easier review.