-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[lldb-dap] Refactoring lldb-dap 'launch' request to use typed RequestHandler<>. #133624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 16 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
3240fe4
[lldb-dap] Refactoring lldb-dap 'launch' request to use typed Request…
ashgti e1f2916
Merge branch 'main' into launch-req
ashgti 70e3e2d
Merge remote-tracking branch 'origin/main' into launch-req
ashgti 6965a74
Fixing up after merging.
ashgti 9e6f343
Merge branch 'launch-req' of github.com:ashgti/llvm-project into laun…
ashgti c15be66
Apply clang-format.
ashgti b42bce8
Apply python format.
ashgti f436482
Moving the defaulted values to optional.
ashgti 309b9eb
Adjusting a comment block and associated if block.
ashgti 0b2f180
Merge branch 'main' into launch-req
ashgti e7b7914
Applying python format.
ashgti 7a40f50
Apply clang-format to organize imports.
ashgti 335468a
Merge branch 'main' into launch-req
ashgti 2b6bb92
Moving 'program', 'targetTriple' and 'platform' to the protocol::Conf…
ashgti 2a90a23
Merge branch 'main' into launch-req
ashgti e883eec
Fixing "launch" request handler post merge.
ashgti 3c09391
Apply suggestions from code review
ashgti 83eeffb
Adjusting the error handling for launches and adding extra tests for …
ashgti a2c7d0f
Applying formatting.
ashgti File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -82,7 +82,7 @@ DAP::DAP(Log *log, const ReplMode default_repl_mode, | |
std::vector<std::string> pre_init_commands, Transport &transport) | ||
: log(log), transport(transport), broadcaster("lldb-dap"), | ||
exception_breakpoints(), focus_tid(LLDB_INVALID_THREAD_ID), | ||
stop_at_entry(false), is_attach(false), | ||
is_attach(false), stop_at_entry(false), | ||
restarting_process_id(LLDB_INVALID_PROCESS_ID), | ||
configuration_done_sent(false), waiting_for_run_in_terminal(false), | ||
progress_event_reporter( | ||
|
@@ -659,9 +659,7 @@ void DAP::RunTerminateCommands() { | |
configuration.terminateCommands); | ||
} | ||
|
||
lldb::SBTarget | ||
DAP::CreateTargetFromArguments(const llvm::json::Object &arguments, | ||
lldb::SBError &error) { | ||
lldb::SBTarget DAP::CreateTarget(lldb::SBError &error) { | ||
// Grab the name of the program we need to debug and create a target using | ||
// the given program as an argument. Executable file can be a source of target | ||
// architecture and platform, if they differ from the host. Setting exe path | ||
|
@@ -672,21 +670,17 @@ DAP::CreateTargetFromArguments(const llvm::json::Object &arguments, | |
// enough information to determine correct arch and platform (or ELF can be | ||
// omitted at all), so it is good to leave the user an apportunity to specify | ||
// those. Any of those three can be left empty. | ||
const llvm::StringRef target_triple = | ||
GetString(arguments, "targetTriple").value_or(""); | ||
const llvm::StringRef platform_name = | ||
GetString(arguments, "platformName").value_or(""); | ||
const llvm::StringRef program = GetString(arguments, "program").value_or(""); | ||
auto target = this->debugger.CreateTarget( | ||
program.data(), target_triple.data(), platform_name.data(), | ||
configuration.program.value_or("").data(), | ||
configuration.targetTriple.value_or("").data(), | ||
configuration.platformName.value_or("").data(), | ||
true, // Add dependent modules. | ||
error); | ||
|
||
if (error.Fail()) { | ||
// Update message if there was an error. | ||
error.SetErrorStringWithFormat( | ||
"Could not create a target for a program '%s': %s.", program.data(), | ||
error.GetCString()); | ||
"Could not create a target for a program: %s.", error.GetCString()); | ||
} | ||
|
||
return target; | ||
|
@@ -944,7 +938,7 @@ llvm::Error DAP::Loop() { | |
return queue_reader.get(); | ||
} | ||
|
||
lldb::SBError DAP::WaitForProcessToStop(uint32_t seconds) { | ||
lldb::SBError DAP::WaitForProcessToStop(std::chrono::seconds seconds) { | ||
lldb::SBError error; | ||
lldb::SBProcess process = target.GetProcess(); | ||
if (!process.IsValid()) { | ||
|
@@ -979,8 +973,8 @@ lldb::SBError DAP::WaitForProcessToStop(uint32_t seconds) { | |
} | ||
std::this_thread::sleep_for(std::chrono::microseconds(250)); | ||
} | ||
error.SetErrorStringWithFormat("process failed to stop within %u seconds", | ||
seconds); | ||
error.SetErrorStringWithFormat("process failed to stop within %lld seconds", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Warning on AArch64 Linux compiling with clang:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sent #137371 to address the warning. |
||
seconds.count()); | ||
return error; | ||
} | ||
|
||
|
@@ -1177,6 +1171,36 @@ bool SendEventRequestHandler::DoExecute(lldb::SBDebugger debugger, | |
return true; | ||
} | ||
|
||
void DAP::ConfigureSourceMaps() { | ||
if (configuration.sourceMap.empty() && !configuration.sourcePath) | ||
return; | ||
|
||
std::string sourceMapCommand; | ||
llvm::raw_string_ostream strm(sourceMapCommand); | ||
strm << "settings set target.source-map "; | ||
|
||
if (!configuration.sourceMap.empty()) { | ||
for (const auto &kv : configuration.sourceMap) { | ||
strm << "\"" << kv.first << "\" \"" << kv.second << "\" "; | ||
} | ||
} else if (configuration.sourcePath) { | ||
strm << "\".\" \"" << *configuration.sourcePath << "\""; | ||
} | ||
|
||
RunLLDBCommands("Setting source map:", {sourceMapCommand}); | ||
} | ||
|
||
void DAP::SetConfiguration(const protocol::Configuration &config, | ||
bool is_attach) { | ||
configuration = config; | ||
this->is_attach = is_attach; | ||
|
||
if (configuration.customFrameFormat) | ||
SetFrameFormat(*configuration.customFrameFormat); | ||
if (configuration.customThreadFormat) | ||
SetThreadFormat(*configuration.customThreadFormat); | ||
} | ||
|
||
void DAP::SetFrameFormat(llvm::StringRef format) { | ||
if (format.empty()) | ||
return; | ||
|
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're touching this: error messages should start with a lowercase and not end with periods so they can be concatenated. I realize we're not very consistent in that aspect but since you're here I figured I'd at least mention it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously the error message would print something along the lines of:
I'm not sure the 'Could not create target' part adds anything, removing it the error message becomes:
Here is an example using:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed