-
Notifications
You must be signed in to change notification settings - Fork 15.2k
update lldb-server platform help parsing #162730
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
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-lldb Author: Chad Smith (cs01) ChangesThe lldb-server platform help text is inconsistent with lldb-server gdbserver help text. This PR modernizes the platform server to use LLVM's TableGen-based option parsing (matching the existing gdbserver implementation), which auto-generates option parsing code and help text. The changes improve documentation quality by adding comprehensive option descriptions, fixing incorrect option names (e.g., before
after
Full diff: https://github.com/llvm/llvm-project/pull/162730.diff 3 Files Affected:
diff --git a/lldb/tools/lldb-server/CMakeLists.txt b/lldb/tools/lldb-server/CMakeLists.txt
index 1d8dc72a3f872..50f294f1b5712 100644
--- a/lldb/tools/lldb-server/CMakeLists.txt
+++ b/lldb/tools/lldb-server/CMakeLists.txt
@@ -2,6 +2,10 @@ set(LLVM_TARGET_DEFINITIONS LLGSOptions.td)
tablegen(LLVM LLGSOptions.inc -gen-opt-parser-defs)
add_public_tablegen_target(LLGSOptionsTableGen)
+set(LLVM_TARGET_DEFINITIONS LLPlatformOptions.td)
+tablegen(LLVM LLPlatformOptions.inc -gen-opt-parser-defs)
+add_public_tablegen_target(LLPlatformOptionsTableGen)
+
set(LLDB_PLUGINS)
if(CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
@@ -67,6 +71,7 @@ add_lldb_tool(lldb-server
add_dependencies(lldb-server
LLGSOptionsTableGen
+ LLPlatformOptionsTableGen
${tablegen_deps}
)
target_include_directories(lldb-server PRIVATE "${LLDB_SOURCE_DIR}/source")
diff --git a/lldb/tools/lldb-server/LLPlatformOptions.td b/lldb/tools/lldb-server/LLPlatformOptions.td
new file mode 100644
index 0000000000000..76f0f264ecd53
--- /dev/null
+++ b/lldb/tools/lldb-server/LLPlatformOptions.td
@@ -0,0 +1,65 @@
+include "llvm/Option/OptParser.td"
+
+class F<string name>: Flag<["--", "-"], name>;
+class R<list<string> prefixes, string name>
+ : Option<prefixes, name, KIND_REMAINING_ARGS>;
+
+multiclass SJ<string name, string help> {
+ def NAME: Separate<["--", "-"], name>,
+ HelpText<help>;
+ def NAME # _eq: Joined<["--", "-"], name # "=">,
+ Alias<!cast<Separate>(NAME)>;
+}
+
+def grp_connect : OptionGroup<"connection">, HelpText<"CONNECTION OPTIONS">;
+
+defm listen: SJ<"listen", "Host and port to listen on. Format: [host]:port or protocol://[host]:port (e.g., tcp://localhost:1234, unix:///path/to/socket).">,
+ MetaVarName<"<[host]:port>">,
+ Group<grp_connect>;
+
+defm socket_file: SJ<"socket-file", "Write listening socket information (port number for TCP or path for Unix domain sockets) to the specified file.">,
+ MetaVarName<"<path>">,
+ Group<grp_connect>;
+
+defm gdbserver_port: SJ<"gdbserver-port", "Port to use for spawned gdbserver instances. If 0 or unspecified, a port will be chosen automatically.">,
+ MetaVarName<"<port>">,
+ Group<grp_connect>;
+
+defm child_platform_fd: SJ<"child-platform-fd", "File descriptor for communication with parent platform process (internal use only).">,
+ MetaVarName<"<fd>">,
+ Group<grp_connect>,
+ Flags<[HelpHidden]>;
+
+def grp_general : OptionGroup<"general options">, HelpText<"GENERAL OPTIONS">;
+
+def server: F<"server">,
+ HelpText<"Run in server mode, accepting multiple client connections sequentially. Without this flag, the server exits after handling the first connection.">,
+ Group<grp_general>;
+
+defm log_channels: SJ<"log-channels", "Channels to log. A colon-separated list of entries. Each entry starts with a channel followed by a space-separated list of categories. Common channels: lldb, gdb-remote, platform, process.">,
+ MetaVarName<"<channel1 categories...:channel2 categories...>">,
+ Group<grp_general>;
+
+defm log_file: SJ<"log-file", "Destination file to log to. If empty, log to stderr.">,
+ MetaVarName<"<file>">,
+ Group<grp_general>;
+
+def debug: F<"debug">,
+ HelpText<"(Unused, kept for backward compatibility)">,
+ Group<grp_general>,
+ Flags<[HelpHidden]>;
+
+def verbose: F<"verbose">,
+ HelpText<"(Unused, kept for backward compatibility)">,
+ Group<grp_general>,
+ Flags<[HelpHidden]>;
+
+def help: F<"help">,
+ HelpText<"Display this help message and exit.">,
+ Group<grp_general>;
+def: Flag<["-"], "h">, Alias<help>,
+ Group<grp_general>;
+
+def REM : R<["--"], "">,
+ HelpText<"Arguments to pass to launched gdbserver instances.">,
+ MetaVarName<"program args">;
diff --git a/lldb/tools/lldb-server/lldb-platform.cpp b/lldb/tools/lldb-server/lldb-platform.cpp
index 0bd928507ba89..37f0e1b95570c 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -21,6 +21,9 @@
#include <fstream>
#include <optional>
+#include "llvm/Option/ArgList.h"
+#include "llvm/Option/OptTable.h"
+#include "llvm/Option/Option.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/ScopedPrinter.h"
#include "llvm/Support/WithColor.h"
@@ -56,22 +59,69 @@ using namespace llvm;
// of target CPUs. For now, let's just use 100.
static const int backlog = 100;
static const int socket_error = -1;
-static int g_debug = 0;
-static int g_verbose = 0;
-static int g_server = 0;
-
-// option descriptors for getopt_long_only()
-static struct option g_long_options[] = {
- {"debug", no_argument, &g_debug, 1},
- {"verbose", no_argument, &g_verbose, 1},
- {"log-file", required_argument, nullptr, 'l'},
- {"log-channels", required_argument, nullptr, 'c'},
- {"listen", required_argument, nullptr, 'L'},
- {"gdbserver-port", required_argument, nullptr, 'P'},
- {"socket-file", required_argument, nullptr, 'f'},
- {"server", no_argument, &g_server, 1},
- {"child-platform-fd", required_argument, nullptr, 2},
- {nullptr, 0, nullptr, 0}};
+
+namespace {
+using namespace llvm::opt;
+
+enum ID {
+ OPT_INVALID = 0, // This is not an option ID.
+#define OPTION(...) LLVM_MAKE_OPT_ID(__VA_ARGS__),
+#include "LLPlatformOptions.inc"
+#undef OPTION
+};
+
+#define OPTTABLE_STR_TABLE_CODE
+#include "LLPlatformOptions.inc"
+#undef OPTTABLE_STR_TABLE_CODE
+
+#define OPTTABLE_PREFIXES_TABLE_CODE
+#include "LLPlatformOptions.inc"
+#undef OPTTABLE_PREFIXES_TABLE_CODE
+
+static constexpr opt::OptTable::Info InfoTable[] = {
+#define OPTION(...) LLVM_CONSTRUCT_OPT_INFO(__VA_ARGS__),
+#include "LLPlatformOptions.inc"
+#undef OPTION
+};
+
+class LLPlatformOptTable : public opt::GenericOptTable {
+public:
+ LLPlatformOptTable()
+ : opt::GenericOptTable(OptionStrTable, OptionPrefixesTable, InfoTable) {}
+
+ void PrintHelp(llvm::StringRef Name) {
+ std::string Usage =
+ (Name + " [options] --listen <[host]:port> [[--] program args...]")
+ .str();
+
+ std::string Title = "lldb-server platform";
+
+ OptTable::printHelp(llvm::outs(), Usage.c_str(), Title.c_str());
+
+ llvm::outs() << R"(
+DESCRIPTION
+ Acts as a platform server for remote debugging. When LLDB clients connect,
+ the platform server handles platform operations (file transfers, process
+ launching) and spawns debug server instances (lldb-server gdbserver) to
+ handle actual debugging sessions.
+
+ By default, the server exits after handling one connection. Use --server
+ to keep running and accept multiple connections sequentially.
+
+EXAMPLES
+ # Listen on port 1234, exit after first connection
+ lldb-server platform --listen tcp://0.0.0.0:1234
+
+ # Listen on port 5555, accept multiple connections
+ lldb-server platform --server --listen tcp://localhost:5555
+
+ # Listen on Unix domain socket
+ lldb-server platform --listen unix:///tmp/lldb-server.sock
+
+)";
+ }
+};
+} // namespace
#if defined(__APPLE__)
#define LOW_PORT (IPPORT_RESERVED)
@@ -97,12 +147,11 @@ static void signal_handler(int signo) {
}
#endif
-static void display_usage(const char *progname, const char *subcommand) {
- fprintf(stderr, "Usage:\n %s %s [--log-file log-file-name] [--log-channels "
- "log-channel-list] [--port-file port-file-path] --server "
- "--listen port\n",
- progname, subcommand);
- exit(0);
+static void display_usage(LLPlatformOptTable &Opts, const char *progname,
+ const char *subcommand) {
+ std::string Name =
+ (llvm::sys::path::filename(progname) + " " + subcommand).str();
+ Opts.PrintHelp(Name);
}
static Status parse_listen_host_port(Socket::SocketProtocol &protocol,
@@ -261,7 +310,8 @@ static Status spawn_process(const char *progname, const FileSpec &prog,
const Socket *conn_socket, uint16_t gdb_port,
const lldb_private::Args &args,
const std::string &log_file,
- const StringRef log_channels, MainLoop &main_loop) {
+ const StringRef log_channels, MainLoop &main_loop,
+ bool multi_client) {
Status error;
SharedSocket shared_socket(conn_socket, error);
if (error.Fail())
@@ -297,9 +347,12 @@ static Status spawn_process(const char *progname, const FileSpec &prog,
launch_info.SetLaunchInSeparateProcessGroup(false);
- if (g_server)
+ // Set up process monitor callback based on whether we're in server mode
+ if (multi_client)
+ // In server mode: empty callback (don't terminate when child exits)
launch_info.SetMonitorProcessCallback([](lldb::pid_t, int, int) {});
else
+ // In single-client mode: terminate main loop when child exits
launch_info.SetMonitorProcessCallback([&main_loop](lldb::pid_t, int, int) {
main_loop.AddPendingCallback(
[](MainLoopBase &loop) { loop.RequestTermination(); });
@@ -371,107 +424,107 @@ int main_platform(int argc, char *argv[]) {
signal(SIGPIPE, SIG_IGN);
signal(SIGHUP, signal_handler);
#endif
- int long_option_index = 0;
+
+ // Special handling for 'help' as first argument
+ if (argc > 0 && strcmp(argv[0], "help") == 0) {
+ LLPlatformOptTable Opts;
+ display_usage(Opts, progname, subcommand);
+ return 0;
+ }
+
Status error;
std::string listen_host_port;
- int ch;
-
std::string log_file;
- StringRef
- log_channels; // e.g. "lldb process threads:gdb-remote default:linux all"
-
+ StringRef log_channels;
shared_fd_t fd = SharedSocket::kInvalidFD;
-
uint16_t gdbserver_port = 0;
-
FileSpec socket_file;
- bool show_usage = false;
- int option_error = 0;
-
- std::string short_options(OptionParser::GetShortOptionString(g_long_options));
-
-#if __GLIBC__
- optind = 0;
-#else
- optreset = 1;
- optind = 1;
-#endif
-
- while ((ch = getopt_long_only(argc, argv, short_options.c_str(),
- g_long_options, &long_option_index)) != -1) {
- switch (ch) {
- case 0: // Any optional that auto set themselves will return 0
- break;
-
- case 'L':
- listen_host_port.append(optarg);
- break;
+ bool multi_client = false;
+ [[maybe_unused]] bool debug = false;
+ [[maybe_unused]] bool verbose = false;
+
+ LLPlatformOptTable Opts;
+ llvm::BumpPtrAllocator Alloc;
+ llvm::StringSaver Saver(Alloc);
+ bool HasError = false;
+
+ opt::InputArgList Args =
+ Opts.parseArgs(argc, argv, OPT_UNKNOWN, Saver, [&](llvm::StringRef Msg) {
+ WithColor::error() << Msg << "\n";
+ HasError = true;
+ });
+
+ std::string Name =
+ (llvm::sys::path::filename(progname) + " " + subcommand).str();
+ std::string HelpText =
+ "Use '" + Name + " --help' for a complete list of options.\n";
+
+ if (HasError) {
+ llvm::errs() << HelpText;
+ return 1;
+ }
- case 'l': // Set Log File
- if (optarg && optarg[0])
- log_file.assign(optarg);
- break;
+ if (Args.hasArg(OPT_help)) {
+ display_usage(Opts, progname, subcommand);
+ return 0;
+ }
- case 'c': // Log Channels
- if (optarg && optarg[0])
- log_channels = StringRef(optarg);
- break;
+ // Parse arguments
+ listen_host_port = Args.getLastArgValue(OPT_listen).str();
+ log_file = Args.getLastArgValue(OPT_log_file).str();
+ log_channels = Args.getLastArgValue(OPT_log_channels);
+ multi_client = Args.hasArg(OPT_server);
+ debug = Args.hasArg(OPT_debug);
+ verbose = Args.hasArg(OPT_verbose);
+
+ if (Args.hasArg(OPT_socket_file)) {
+ socket_file.SetFile(Args.getLastArgValue(OPT_socket_file),
+ FileSpec::Style::native);
+ }
- case 'f': // Socket file
- if (optarg && optarg[0])
- socket_file.SetFile(optarg, FileSpec::Style::native);
- break;
+ if (Args.hasArg(OPT_gdbserver_port)) {
+ if (!llvm::to_integer(Args.getLastArgValue(OPT_gdbserver_port),
+ gdbserver_port)) {
+ WithColor::error() << "invalid --gdbserver-port value\n";
+ return 1;
+ }
+ }
- case 'P':
- case 'm':
- case 'M': {
- uint16_t portnum;
- if (!llvm::to_integer(optarg, portnum)) {
- WithColor::error() << "invalid port number string " << optarg << "\n";
- option_error = 2;
- break;
- }
- // Note the condition gdbserver_port > HIGH_PORT is valid in case of using
- // --child-platform-fd. Check gdbserver_port later.
- if (ch == 'P')
- gdbserver_port = portnum;
- else if (gdbserver_port == 0)
- gdbserver_port = portnum;
- } break;
-
- case 2: {
- uint64_t _fd;
- if (!llvm::to_integer(optarg, _fd)) {
- WithColor::error() << "invalid fd " << optarg << "\n";
- option_error = 6;
- } else
- fd = (shared_fd_t)_fd;
- } break;
-
- case 'h': /* fall-through is intentional */
- case '?':
- show_usage = true;
- break;
+ if (Args.hasArg(OPT_child_platform_fd)) {
+ uint64_t _fd;
+ if (!llvm::to_integer(Args.getLastArgValue(OPT_child_platform_fd), _fd)) {
+ WithColor::error() << "invalid --child-platform-fd value\n";
+ return 1;
}
+ fd = (shared_fd_t)_fd;
}
if (!LLDBServerUtilities::SetupLogging(log_file, log_channels, 0))
return -1;
// Print usage and exit if no listening port is specified.
- if (listen_host_port.empty() && fd == SharedSocket::kInvalidFD)
- show_usage = true;
+ if (listen_host_port.empty() && fd == SharedSocket::kInvalidFD) {
+ WithColor::error() << "either --listen or --child-platform-fd is required\n"
+ << HelpText;
+ return 1;
+ }
- if (show_usage || option_error) {
- display_usage(progname, subcommand);
- exit(option_error);
+ // Get remaining arguments for inferior
+ std::vector<llvm::StringRef> Inputs;
+ for (opt::Arg *Arg : Args.filtered(OPT_INPUT))
+ Inputs.push_back(Arg->getValue());
+ if (opt::Arg *Arg = Args.getLastArg(OPT_REM)) {
+ for (const char *Val : Arg->getValues())
+ Inputs.push_back(Val);
}
- // Skip any options we consumed with getopt_long_only.
- argc -= optind;
- argv += optind;
lldb_private::Args inferior_arguments;
- inferior_arguments.SetArguments(argc, const_cast<const char **>(argv));
+ if (!Inputs.empty()) {
+ std::vector<const char *> args_ptrs;
+ for (const auto &Input : Inputs)
+ args_ptrs.push_back(Input.data());
+ inferior_arguments.SetArguments(args_ptrs.size(), args_ptrs.data());
+ }
FileSpec debugserver_path = GetDebugserverPath();
if (!debugserver_path) {
@@ -577,22 +630,22 @@ int main_platform(int argc, char *argv[]) {
llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>> platform_handles =
platform_sock->Accept(
main_loop, [progname, gdbserver_port, &inferior_arguments, log_file,
- log_channels, &main_loop,
+ log_channels, &main_loop, multi_client,
&platform_handles](std::unique_ptr<Socket> sock_up) {
printf("Connection established.\n");
Status error = spawn_process(
progname, HostInfo::GetProgramFileSpec(), sock_up.get(),
gdbserver_port, inferior_arguments, log_file, log_channels,
- main_loop);
+ main_loop, multi_client);
if (error.Fail()) {
Log *log = GetLog(LLDBLog::Platform);
LLDB_LOGF(log, "spawn_process failed: %s", error.AsCString());
WithColor::error()
<< "spawn_process failed: " << error.AsCString() << "\n";
- if (!g_server)
+ if (!multi_client)
main_loop.RequestTermination();
}
- if (!g_server)
+ if (!multi_client)
platform_handles->clear();
});
if (!platform_handles) {
|
Yay for TableGen! If a test like this doesn't exist already, can you add something like |
17d6190
to
6d6cabb
Compare
Sounds good, I'll take a look |
98e4f98
to
eab11a4
Compare
Added a few tests, one for help text, one for error messages, and one for successful startups. Thank you for the suggestion!
I also renamed lldb-server/TestErrorMessages.test to TestGdbserverErrorMessages.test to disambiguate it from lldb-server platform tests. |
Nice! Thanks for the change! (I will let Jonas accept.) |
eab11a4
to
e0cf097
Compare
Updated per your feedback @JDevlieghere The one question I had was regarding
and whether I should replace the -1 with |
Btw, I plan to make a few more pull requests to improve the API of lldb-server and the messages it prints. One for lldb-server's help itself. Another for lldb-server p's messages that it logs to stderr (it's completely silent when it starts and is unclear if it's working or hung). And one more to hide the |
Personally I would keep the current value, as it seems to have been done on purpose, and the risk of breaking someone's workflow outweighs the benefit of slightly improving consistency. |
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.
LGTM. I added some other active contributors to lldb-server for visibility.
e0cf097
to
7673bc3
Compare
The lldb-server platform help text is inconsistent with lldb-server gdbserver help text. This PR modernizes the platform server to use LLVM's TableGen-based option parsing (matching the existing gdbserver implementation), which auto-generates option parsing code and help text.
The changes improve documentation quality by adding comprehensive option descriptions, fixing incorrect option names (e.g.,
--port-file
→--socket-file
), adding support for-h
/--help
flags, and organizing help output with DESCRIPTION and EXAMPLES sections. Internal-only options (--child-platform-fd
) and unused legacy options (--debug
,--verbose
) are now hidden from help while maintaining backward compatibility. All functional behavior remains unchanged—this is purely a documentation and code modernization improvement.before
after
For comparison, here is the gdbserver help text: