[lldb] add stop-at-user-entry option to process launch #67019
Conversation
|
@llvm/pr-subscribers-lldb ChangesDescriptionThis pull request adds a new MotivationThe Changes Made
Usage
Full diff: https://github.com/llvm/llvm-project/pull/67019.diff 4 Files Affected:
diff --git a/lldb/source/Commands/CMakeLists.txt b/lldb/source/Commands/CMakeLists.txt
index 6a36c5376d5c574..54c62e0f5284beb 100644
--- a/lldb/source/Commands/CMakeLists.txt
+++ b/lldb/source/Commands/CMakeLists.txt
@@ -30,6 +30,7 @@ add_lldb_library(lldbCommands NO_PLUGIN_DEPENDENCIES
CommandObjectSession.cpp
CommandObjectSettings.cpp
CommandObjectSource.cpp
+ CommandObjectStart.cpp
CommandObjectStats.cpp
CommandObjectTarget.cpp
CommandObjectThread.cpp
diff --git a/lldb/source/Commands/CommandObjectStart.cpp b/lldb/source/Commands/CommandObjectStart.cpp
new file mode 100644
index 000000000000000..7406143c50fec1b
--- /dev/null
+++ b/lldb/source/Commands/CommandObjectStart.cpp
@@ -0,0 +1,102 @@
+//===-- CommandObjectStart.cpp -------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "CommandObjectStart.h"
+#include "lldb/Interpreter/CommandObject.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+// Constructor for CommandObjectStart
+CommandObjectStart::CommandObjectStart(CommandInterpreter &interpreter)
+ : CommandObjectParsed(
+ interpreter, "start",
+ "Launches the process and pauses execution at main function",
+ "start args [optional args]") {
+ // Define command arguments
+ CommandArgumentData pause_location{eArgTypeName, eArgRepeatPlain};
+ m_arguments.push_back({pause_location});
+}
+
+CommandObjectStart::~CommandObjectStart() = default;
+
+// Execute the 'start' command
+bool CommandObjectStart::DoExecute(Args &command, CommandReturnObject &result) {
+ // Check if the 'first' subcommand is specified
+ bool pause_at_first_instruction = false;
+
+ if (command.GetArgumentCount() == 1 &&
+ strcmp(command.GetArgumentAtIndex(0), "first") == 0) {
+ pause_at_first_instruction = true;
+ }
+
+ // Get the current selected target
+ TargetSP target_sp = GetDebugger().GetSelectedTarget();
+ if (!target_sp) {
+ result.AppendError("No target selected.\n");
+ return false;
+ }
+
+ // Create the breakpoint at main or the first instruction
+ BreakpointSP bp_sp;
+ if (pause_at_first_instruction) {
+ ModuleSP exe_module_sp = target_sp->GetExecutableModule();
+ ObjectFile *object = exe_module_sp->GetObjectFile();
+ Address address = object->GetEntryPointAddress();
+
+ if (!address.IsValid()) {
+ result.AppendError("Failed to get the entry point address");
+ result.SetStatus(eReturnStatusFailed);
+ return false;
+ }
+
+ bp_sp = target_sp->CreateBreakpoint(address, false, false);
+
+ } else {
+ // Create a breakpoint at the main function
+ bp_sp = target_sp->CreateBreakpoint(
+ nullptr, nullptr, "main", eFunctionNameTypeAuto, eLanguageTypeUnknown,
+ 0, eLazyBoolNo, false, false);
+ }
+
+ if (!bp_sp) {
+ result.AppendError("Breakpoint creation failed.\n");
+ result.SetStatus(eReturnStatusFailed);
+ return false;
+ } else {
+ result.GetOutputStream().Printf("Breakpoint created%s.\n",
+ pause_at_first_instruction
+ ? " at first instruction"
+ : " in main function");
+ result.SetStatus(eReturnStatusSuccessFinishResult);
+ }
+
+ // Construct the process launch info
+ ProcessLaunchInfo launch_info;
+ launch_info.SetShell(HostInfo::GetDefaultShell());
+ ModuleSP exe_module_sp = target_sp->GetExecutableModule();
+ if (!exe_module_sp) {
+ result.AppendError("No executable module found.\n");
+ return false;
+ }
+
+ launch_info.SetExecutableFile(exe_module_sp->GetPlatformFileSpec(), true);
+ // Launch the process
+ StreamString stream;
+ Status error = target_sp->Launch(launch_info, &stream);
+
+ if (error.Success()) {
+ result.SetStatus(eReturnStatusSuccessFinishResult);
+ result.GetOutputStream().Printf("Process launched successfully.\n");
+ } else {
+ result.AppendErrorWithFormat("Process launch failed: %s\n",
+ error.AsCString());
+ return false;
+ }
+ return result.Succeeded();
+}
diff --git a/lldb/source/Commands/CommandObjectStart.h b/lldb/source/Commands/CommandObjectStart.h
new file mode 100644
index 000000000000000..8475a17c347f7be
--- /dev/null
+++ b/lldb/source/Commands/CommandObjectStart.h
@@ -0,0 +1,32 @@
+//===-- CommandObjectStart.h ---------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_SOURCE_COMMANDS_COMMANDOBJECTStart_H
+#define LLDB_SOURCE_COMMANDS_COMMANDOBJECTStart_H
+
+#include "lldb/Core/Module.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Host/ProcessLaunchInfo.h"
+#include "lldb/Interpreter/CommandInterpreter.h"
+#include "lldb/Interpreter/CommandObject.h"
+#include "lldb/Interpreter/CommandObjectMultiword.h"
+#include "lldb/Interpreter/CommandReturnObject.h"
+#include "lldb/Symbol/SymbolContext.h"
+
+namespace lldb_private {
+class CommandObjectStart : public CommandObjectParsed {
+public:
+ CommandObjectStart(CommandInterpreter &interpreter);
+ ~CommandObjectStart() override;
+
+protected:
+ bool DoExecute(Args &args, CommandReturnObject &result) override;
+};
+} // namespace lldb_private
+
+#endif // LLDB_SOURCE_COMMANDS_COMMANDOBJECTStart_H
diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp
index dcff53ff843f328..5d6ad7f0dbb1e8e 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -36,6 +36,7 @@
#include "Commands/CommandObjectSession.h"
#include "Commands/CommandObjectSettings.h"
#include "Commands/CommandObjectSource.h"
+#include "Commands/CommandObjectStart.h"
#include "Commands/CommandObjectStats.h"
#include "Commands/CommandObjectTarget.h"
#include "Commands/CommandObjectThread.h"
@@ -559,6 +560,7 @@ void CommandInterpreter::LoadCommandDictionary() {
REGISTER_COMMAND_OBJECT("settings", CommandObjectMultiwordSettings);
REGISTER_COMMAND_OBJECT("session", CommandObjectSession);
REGISTER_COMMAND_OBJECT("source", CommandObjectMultiwordSource);
+ REGISTER_COMMAND_OBJECT("start", CommandObjectStart);
REGISTER_COMMAND_OBJECT("statistics", CommandObjectStats);
REGISTER_COMMAND_OBJECT("target", CommandObjectMultiwordTarget);
REGISTER_COMMAND_OBJECT("thread", CommandObjectMultiwordThread);
|
There was a problem hiding this comment.
Half of this command is just process launch --stop-at-entry. If you wanted to can the "break at main" part, then add a --stop-at-main to process launch. And if you want a "start" command, the lldb way to do that would be to make a regex command that either does process launch --stop-at-entry or the new process launch --stop-at-main.
We don't add real top level commands that are just aliases to extant behavior.
|
Even if this were enough behavior to warrant a new command, it would live under the |
|
Thank you for the explanation here and in Discourse. I added the option |
There was a problem hiding this comment.
The reason that gdb has a "start" command is that not all languages and their runtimes use the same name for the the entry point function. So start does "figure out what the start function in the main executable is and set a breakpoint there". Or maybe it just knows the start points of all the languages and breaks symbolically on each one, I can't remember how gdb does this...
In our case, the languages we currently support all use "main" for the entry point function, but since we're going to the trouble of making an canned bit of functionality for this, it might be nice to add that abstraction.
|
I can't tell how to read the "Files Changed" part of the GitHub PR change. It looks like the --stop-at-main is additive, but to be clear, the second of these two commits looks okay to me, but the first commit is still not something we want to do. |
|
Btw, I'm building support for the mojo language, and very likely the |
|
The other improvement I'd suggest here is that the breakpoints you set to implement the "stop at main" behavior need to be be "one-shot" breakpoints. Otherwise, if you do: (lldb) process launch --stop-at-main and then: (lldb) process launch you're still going to stop at main, which is surprising. |
|
This might be a dumb question, but I committed the implementation as an option as the second commit thinking you would/could discard my first commit. Is that possible? What would be the best way to correct this? |
|
That's file. You can have multiple commits in the same PR, because when it gets approved and merged, github will squash all your commits into a single one. |
|
Concretely, you can revert the first commit and push that so that your branch has your first commit, second commit, and a revert commit for the first one. As Walter said, it should squash down and resolve correctly. |
|
You also may want to change the title of your PR to reflect the actual change you'll be making as well. When you squash and merge, you'll have an opportunity to rewrite the commit message to more accurately reflect what your change does as well. |
|
Thank you @bulbazord & @clayborg for the step-by-step, it was really helpful. I reverted the first commit, removed the shell modification you mentioned and the created breakpoint is now one-shot, @jimingham. |
junior-jl
changed the title
There was a problem hiding this comment.
See my inline comments. I am wondering if we want to get the platform involved here so they can do the right thing for any platform and it might involve renaming the option away from containing "main" since that might not be the user entry point into a program for all platforms.
|
On Sep 26, 2023, at 7:55 AM, José Lira Junior ***@***.***> wrote:
@junior-jl commented on this pull request.
In lldb/source/Commands/CommandOptionsProcessLaunch.cpp <#67019 (comment)>:
> @@ -38,7 +38,18 @@ Status CommandOptionsProcessLaunch::SetOptionValue(
case 's': // Stop at program entry point
launch_info.GetFlags().Set(eLaunchFlagStopAtEntry);
break;
-
+ case 'm': // Stop at main function
+ {
+ TargetSP target_sp =
+ execution_context ? execution_context->GetTargetSP() : TargetSP();
+ BreakpointSP bp_sp = target_sp->CreateBreakpoint(
+ nullptr, nullptr, "main", eFunctionNameTypeAuto, eLanguageTypeUnknown,
If you find more than one name, there's a Target::CreateBreakpoint overload that takes a const char *func_names[] - that you can use to make one portmanteau breakpoint for all the entry point names.
Is this meant for scenarios where a program is written in more than one programming language?
Right. Mixing programming languages in a single executable is pretty common these days, so we need to support that scenario.
You either have to be able to figure out from the executable that "the main entry point for this code is implemented by language X and so I know which symbol to set a breakpoint on" or you just set a breakpoint on all the entry points you know and let which ones find BreakpointLocations or get hit tell you the answer to that question. The latter method is easier to implement, and maybe the only way if there's nothing telling you which language contributed the "main" function.
Jim
… —
Reply to this email directly, view it on GitHub <#67019 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW5E24AQUKSNTY22PVLX4LUHTANCNFSM6AAAAAA5BUJK5M>.
You are receiving this because you were mentioned.
|
|
Yes that's right. Normally what happens in program execution is the main executable gets loaded along with the dynamic loader, and then the loader runs, leading the program through several rounds of shared library loading (that can be a lot of shared libraries these days) and THEN runs the user entry point. Adding this filter means that we won't try to resolve a "main" breakpoint in every one of those shared libraries.
Jim
… On Sep 26, 2023, at 7:39 AM, José Lira Junior ***@***.***> wrote:
@junior-jl commented on this pull request.
In lldb/source/Commands/CommandOptionsProcessLaunch.cpp <#67019 (comment)>:
> @@ -38,7 +38,18 @@ Status CommandOptionsProcessLaunch::SetOptionValue(
case 's': // Stop at program entry point
launch_info.GetFlags().Set(eLaunchFlagStopAtEntry);
break;
-
+ case 'm': // Stop at main function
+ {
+ TargetSP target_sp =
+ execution_context ? execution_context->GetTargetSP() : TargetSP();
+ BreakpointSP bp_sp = target_sp->CreateBreakpoint(
+ nullptr, nullptr, "main", eFunctionNameTypeAuto, eLanguageTypeUnknown,
It's also a good idea to add a shared library filter instead of the first nullptr in the code above, and specify the main executable. You know the entry point has to be in the main executable, and so it would be inefficient to search the whole world for this breakpoint. It could also end up not being what you want, because some other shared library might call a function named "main" as part of it's shared library loading code (which happens before the language's entry point) so you would in fact stop too early.
I see. Something like this?
ModuleSP main_module_sp = target_sp->GetExecutableModule();
FileSpecList shared_lib_filter;
shared_lib_filter.Append(main_module_sp->GetFileSpec());
BreakpointSP bp_sp = target_sp->CreateBreakpoint(
&shared_lib_filter, nullptr, "main", eFunctionNameTypeFull, eLanguageTypeUnknown,
0, eLazyBoolNo, false, false);
—
Reply to this email directly, view it on GitHub <#67019 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW5NFBYC7PEIMKPRGZTX4LSIZANCNFSM6AAAAAA5BUJK5M>.
You are receiving this because you were mentioned.
|
|
On Sep 26, 2023, at 7:29 AM, José Lira Junior ***@***.***> wrote:
@junior-jl commented on this pull request.
In lldb/source/Commands/CommandOptionsProcessLaunch.cpp <#67019 (comment)>:
> @@ -38,7 +38,18 @@ Status CommandOptionsProcessLaunch::SetOptionValue(
case 's': // Stop at program entry point
launch_info.GetFlags().Set(eLaunchFlagStopAtEntry);
break;
-
+ case 'm': // Stop at main function
+ {
+ TargetSP target_sp =
+ execution_context ? execution_context->GetTargetSP() : TargetSP();
+ BreakpointSP bp_sp = target_sp->CreateBreakpoint(
+ nullptr, nullptr, "main", eFunctionNameTypeAuto, eLanguageTypeUnknown,
We should add a new API to the Language plugin: Language::GetUserEntrypointName() and then this code should iterate over all the supported languages building up a unique list of entry point names using this API. At present there's going to be one: "main" repeated many times but I think Walter promised us another one soon...
About this, Walter had proposed the following: "I'd suggest adding a virtual method GetMainSymbol to the Language.h interface, which could be implemented by each language plugin.". I guess it's a good approach. What are your thoughts?
Right, that's also what I was suggesting. W.r.t. Walter's suggestion, I still think it's better to use a generic term (I suggested GetUserEntrypointName) rather than explicitly using the word "main", which is the C name for this feature... Greg suggested making this change to the option name as well earlier in this thread.
Jim
… —
Reply to this email directly, view it on GitHub <#67019 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW42YJDE57AFPW3Y7CDX4LRFPANCNFSM6AAAAAA5BUJK5M>.
You are receiving this because you were mentioned.
|
|
This is the case in the current commit. I was trying to implement this with SmallSet and const char* to use the overload you mentioned, Jim, but I was having trouble with it, specially because I needed to hard code a 'max number of languages'. So I used the overload that takes a std::vectorstd::string. Let me know if that's not good. case 'm': // Stop at user entry point
{
TargetSP target_sp =
execution_context ? execution_context->GetTargetSP() : TargetSP();
ModuleSP main_module_sp = target_sp->GetExecutableModule();
FileSpecList shared_lib_filter;
shared_lib_filter.Append(main_module_sp->GetFileSpec());
std::vector<std::string> entryPointNames;
for (LanguageType lang_type : Language::GetSupportedLanguages()) {
Language *lang = Language::FindPlugin(lang_type);
if (lang) {
std::string entryPointName = lang->GetUserEntryPointName();
if (!entryPointName.empty()) {
entryPointNames.push_back(entryPointName);
}
}
}
BreakpointSP bp_sp = target_sp->CreateBreakpoint(
/*containingModules=*/&shared_lib_filter,
/*containingSourceFiles=*/nullptr,
/*func_names=*/entryPointNames,
/*func_name_type_mask=*/eFunctionNameTypeFull,
/*language=*/eLanguageTypeUnknown,
/*offset=*/0, /*skip_prologue=*/eLazyBoolNo, /*internal=*/false,
/*hardware=*/false);
if (!bp_sp)
error.SetErrorString("Breakpoint creation failed.\n");
bp_sp->SetOneShot(true);
break;
} |
|
Also, I just noticed that |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
junior-jl
changed the title
|
What are your thoughts on creating an alias for Also, if I wanted to do that, the only change would be an |
|
We try to only add really frequently used aliases to the default lldb set. In particular, the "unique first one and two character" space in the lldb command set is valuable, that's for your most common every-day commands, and we try to leave as much of it open as possible for users to customize lldb for their own purposes.
In the case of "start", that overlaps with "step" so it doesn't actually take up a new slot in that one or two character space, so it's not so bad. However, we certainly don't want to make any of the partial spellings of "step" become ambiguous. In the lldb command line exact matches to aliases always win. There's one for `s` but it doesn't look like there's one for `st`. So if you are going to add `start` as an alias, you should also add an "st" alias to "thread step" since that's a much more common command.
I personally wouldn't use "start" enough to warrant having it in the default command set.
One of the main reasons that "stopping at main" was handy in gdb was that gdb didn't originally do "future-break" so you always had to run to main to get everything loaded, then set your breakpoints. That's not necessary in lldb (or really modern gdb's either)... I think if you are using toy programs, stopping at main is pretty common, but I don't think it's all that common once you are debugging significant programs, main is generally pretty far from anything interesting.
But I don't have a strong opinion about that, if other people want to weigh in.
And to answer your direct question, you are right, you add aliases to the default command set by putting a new AddAlias in CommandInterpreter.cpp.
Jim
… On Sep 27, 2023, at 7:31 AM, José Lira Junior ***@***.***> wrote:
What are your thoughts on creating an alias for process launch --stop-at-user-entry such as the intended start (or begin, or even run -m).
Also, if I wanted to do that, the only change would be an AddAlias in CommandInterpreter.cpp? Or there's something else?
—
Reply to this email directly, view it on GitHub <#67019 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW7K226YIHINLF4CR7TX4Q2FLANCNFSM6AAAAAA5BUJK5M>.
You are receiving this because you were mentioned.
|
|
I understand, thank you, Jim. Is there anything else I should change in this PR? |
|
I think you're good for the implementing however it would be nice to add a shell test to make sure it works well and it doesn't break in the future. |
|
You can use |
Using this file as a template, I wrote But I wanted to add the check line
Is this behaviour expected? |
I can reproduce it: By default the debugger runs in asynchronous mode so the stop events can be handled in a nondetermistic way. However, I think this is confusing and we should do something about it (in a separate PR). Could you file a new issue describing this behavior and pasting the link here. Your test looks fine to me, so we can move on with it to merge your PR. I'll update it after we fix that other bug. |
Done! #68035
Great. Added the test in the latest commit. |
|
@clayborg what do you think ? Can we merge this ? |
|
@junior-jl I think everyone approved the PR so you just need to address the formatting issues so we can merge it. Would you mind squashing your 3 commits into a single one and running |
junior-jl
force-pushed
the
start
branch
2 times, most recently
from
5e03656
to
c5466f3
Compare
lldb/test/Shell/Commands/command-process-launch-user-entry.test
Outdated
Show resolved
Hide resolved
[lldb] add stop-at-user-entry option to process launch refactor CreateBreakpointAtUserEntry method remove unecessary includes
|
Addressed @JDevlieghere comments regarding formatting and inlined comments. |
|
very nice! Thanks for doing all of our suggested improvements, great patch. I would love to see a new API in SBTarget that exposes this feature: |
|
Thanks for working on this! It's going to save me a lot of "b main" in the future. |
Description
This pull request adds a new
stop-at-user-entryoption to LLDBprocess launchcommand, allowing users to launch a process and pause execution at the entry point of the program (for C-based languages,mainfunction).Motivation
This option provides a convenient way to begin debugging a program by launching it and breaking at the desired entry point.
Changes Made
stop-at-user-entryoption toOptions.tdand the corresponding case inCommandOptionsProcessLaunch.cpp(short option is 'm')GetUserEntryPointNamemethod in the Language plugins available at the moment.CreateBreakpointAtUserEntrymethod in the Target API.command-process-launch-user-entry.test.Usage
process launch --stop-at-user-entryorprocess launch -mlaunches the process and pauses execution at the entry point of the program.