[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGfa5fa63fd140: [lldb] Port lldb gdb-server to libOption
(authored by labath).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89477/new/
https://reviews.llvm.org/D89477
Files:
lldb/include/lldb/Utility/Args.h
lldb/source/Utility/Args.cpp
lldb/test/Shell/lldb-server/TestErrorMessages.test
lldb/tools/lldb-server/CMakeLists.txt
lldb/tools/lldb-server/LLGSOptions.td
lldb/tools/lldb-server/lldb-gdbserver.cpp
Index: lldb/tools/lldb-server/lldb-gdbserver.cpp
===
--- lldb/tools/lldb-server/lldb-gdbserver.cpp
+++ lldb/tools/lldb-server/lldb-gdbserver.cpp
@@ -17,7 +17,6 @@
#include
#endif
-
#include "Acceptor.h"
#include "LLDBServerUtilities.h"
#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h"
@@ -25,8 +24,6 @@
#include "lldb/Host/Config.h"
#include "lldb/Host/ConnectionFileDescriptor.h"
#include "lldb/Host/FileSystem.h"
-#include "lldb/Host/HostGetOpt.h"
-#include "lldb/Host/OptionParser.h"
#include "lldb/Host/Pipe.h"
#include "lldb/Host/Socket.h"
#include "lldb/Host/StringConvert.h"
@@ -34,7 +31,11 @@
#include "lldb/Target/Process.h"
#include "lldb/Utility/Status.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Option/ArgList.h"
+#include "llvm/Option/OptTable.h"
+#include "llvm/Option/Option.h"
#include "llvm/Support/Errno.h"
+#include "llvm/Support/WithColor.h"
#if defined(__linux__)
#include "Plugins/Process/Linux/NativeProcessLinux.h"
@@ -88,31 +89,6 @@
#endif
}
-// option descriptors for getopt_long_only()
-
-static int g_debug = 0;
-static int g_verbose = 0;
-
-static struct option g_long_options[] = {
-{"debug", no_argument, _debug, 1},
-{"verbose", no_argument, _verbose, 1},
-{"log-file", required_argument, nullptr, 'l'},
-{"log-channels", required_argument, nullptr, 'c'},
-{"attach", required_argument, nullptr, 'a'},
-{"named-pipe", required_argument, nullptr, 'N'},
-{"pipe", required_argument, nullptr, 'U'},
-{"native-regs", no_argument, nullptr,
- 'r'}, // Specify to use the native registers instead of the gdb defaults
- // for the architecture. NOTE: this is a do-nothing arg as it's
- // behavior is default now. FIXME remove call from lldb-platform.
-{"reverse-connect", no_argument, nullptr,
- 'R'}, // Specifies that llgs attaches to the client address:port rather
- // than llgs listening for a connection from address on port.
-{"setsid", no_argument, nullptr,
- 'S'}, // Call setsid() to make llgs run in its own session.
-{"fd", required_argument, nullptr, 'F'},
-{nullptr, 0, nullptr, 0}};
-
#ifndef _WIN32
// Watch for signals
static int g_sighup_received_count = 0;
@@ -129,20 +105,6 @@
}
#endif // #ifndef _WIN32
-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] "
- "[--setsid] "
- "[--fd file-descriptor]"
- "[--named-pipe named-pipe-path] "
- "[--native-regs] "
- "[--attach pid] "
- "[[HOST]:PORT] "
- "[-- PROGRAM ARG1 ARG2 ...]\n",
- progname, subcommand);
-}
-
void handle_attach_to_pid(GDBRemoteCommunicationServerLLGS _server,
lldb::pid_t pid) {
Status error = gdb_server.AttachToProcess(pid);
@@ -176,12 +138,12 @@
handle_attach_to_process_name(gdb_server, attach_target);
}
-void handle_launch(GDBRemoteCommunicationServerLLGS _server, int argc,
- const char *const argv[]) {
+void handle_launch(GDBRemoteCommunicationServerLLGS _server,
+ llvm::ArrayRef Arguments) {
ProcessLaunchInfo info;
info.GetFlags().Set(eLaunchFlagStopAtEntry | eLaunchFlagDebug |
eLaunchFlagDisableASLR);
- info.SetArguments(const_cast(argv), true);
+ info.SetArguments(Args(Arguments), true);
llvm::SmallString<64> cwd;
if (std::error_code ec = llvm::sys::fs::current_path(cwd)) {
@@ -198,7 +160,7 @@
Status error = gdb_server.LaunchProcess();
if (error.Fail()) {
llvm::errs() << llvm::formatv("error: failed to launch '{0}': {1}\n",
- argv[0], error);
+ Arguments[0], error);
exit(1);
}
}
@@ -229,7 +191,7 @@
void ConnectToRemote(MainLoop ,
GDBRemoteCommunicationServerLLGS _server,
- bool reverse_connect, const char *const host_and_port,
+ bool reverse_connect, llvm::StringRef host_and_port,
const char *const progname, const char *const subcommand,
[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption
JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.
LGTM
Comment at: lldb/source/Utility/Args.cpp:178
+Args::Args(llvm::ArrayRef args) : Args() {
+ for (llvm::StringRef arg : args)
labath wrote:
> JDevlieghere wrote:
> > Maybe StringList should have a ctor that takes an `ArrayRef`,
> > but probably not worth the extra copies here.
> I think it can have that constructor if it's needed (though I'd rather delete
> that class altogether), but I think this constructor would be good to have
> regardless.
Agreed on both accounts
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89477/new/
https://reviews.llvm.org/D89477
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption
labath updated this revision to Diff 298606.
labath marked an inline comment as done.
labath added a comment.
address review feedback
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89477/new/
https://reviews.llvm.org/D89477
Files:
lldb/include/lldb/Utility/Args.h
lldb/source/Utility/Args.cpp
lldb/test/Shell/lldb-server/TestErrorMessages.test
lldb/tools/lldb-server/CMakeLists.txt
lldb/tools/lldb-server/LLGSOptions.td
lldb/tools/lldb-server/lldb-gdbserver.cpp
Index: lldb/tools/lldb-server/lldb-gdbserver.cpp
===
--- lldb/tools/lldb-server/lldb-gdbserver.cpp
+++ lldb/tools/lldb-server/lldb-gdbserver.cpp
@@ -17,7 +17,6 @@
#include
#endif
-
#include "Acceptor.h"
#include "LLDBServerUtilities.h"
#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h"
@@ -25,8 +24,6 @@
#include "lldb/Host/Config.h"
#include "lldb/Host/ConnectionFileDescriptor.h"
#include "lldb/Host/FileSystem.h"
-#include "lldb/Host/HostGetOpt.h"
-#include "lldb/Host/OptionParser.h"
#include "lldb/Host/Pipe.h"
#include "lldb/Host/Socket.h"
#include "lldb/Host/StringConvert.h"
@@ -34,7 +31,11 @@
#include "lldb/Target/Process.h"
#include "lldb/Utility/Status.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Option/ArgList.h"
+#include "llvm/Option/OptTable.h"
+#include "llvm/Option/Option.h"
#include "llvm/Support/Errno.h"
+#include "llvm/Support/WithColor.h"
#if defined(__linux__)
#include "Plugins/Process/Linux/NativeProcessLinux.h"
@@ -88,31 +89,6 @@
#endif
}
-// option descriptors for getopt_long_only()
-
-static int g_debug = 0;
-static int g_verbose = 0;
-
-static struct option g_long_options[] = {
-{"debug", no_argument, _debug, 1},
-{"verbose", no_argument, _verbose, 1},
-{"log-file", required_argument, nullptr, 'l'},
-{"log-channels", required_argument, nullptr, 'c'},
-{"attach", required_argument, nullptr, 'a'},
-{"named-pipe", required_argument, nullptr, 'N'},
-{"pipe", required_argument, nullptr, 'U'},
-{"native-regs", no_argument, nullptr,
- 'r'}, // Specify to use the native registers instead of the gdb defaults
- // for the architecture. NOTE: this is a do-nothing arg as it's
- // behavior is default now. FIXME remove call from lldb-platform.
-{"reverse-connect", no_argument, nullptr,
- 'R'}, // Specifies that llgs attaches to the client address:port rather
- // than llgs listening for a connection from address on port.
-{"setsid", no_argument, nullptr,
- 'S'}, // Call setsid() to make llgs run in its own session.
-{"fd", required_argument, nullptr, 'F'},
-{nullptr, 0, nullptr, 0}};
-
#ifndef _WIN32
// Watch for signals
static int g_sighup_received_count = 0;
@@ -129,20 +105,6 @@
}
#endif // #ifndef _WIN32
-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] "
- "[--setsid] "
- "[--fd file-descriptor]"
- "[--named-pipe named-pipe-path] "
- "[--native-regs] "
- "[--attach pid] "
- "[[HOST]:PORT] "
- "[-- PROGRAM ARG1 ARG2 ...]\n",
- progname, subcommand);
-}
-
void handle_attach_to_pid(GDBRemoteCommunicationServerLLGS _server,
lldb::pid_t pid) {
Status error = gdb_server.AttachToProcess(pid);
@@ -176,12 +138,12 @@
handle_attach_to_process_name(gdb_server, attach_target);
}
-void handle_launch(GDBRemoteCommunicationServerLLGS _server, int argc,
- const char *const argv[]) {
+void handle_launch(GDBRemoteCommunicationServerLLGS _server,
+ llvm::ArrayRef Arguments) {
ProcessLaunchInfo info;
info.GetFlags().Set(eLaunchFlagStopAtEntry | eLaunchFlagDebug |
eLaunchFlagDisableASLR);
- info.SetArguments(const_cast(argv), true);
+ info.SetArguments(Args(Arguments), true);
llvm::SmallString<64> cwd;
if (std::error_code ec = llvm::sys::fs::current_path(cwd)) {
@@ -198,7 +160,7 @@
Status error = gdb_server.LaunchProcess();
if (error.Fail()) {
llvm::errs() << llvm::formatv("error: failed to launch '{0}': {1}\n",
- argv[0], error);
+ Arguments[0], error);
exit(1);
}
}
@@ -229,7 +191,7 @@
void ConnectToRemote(MainLoop ,
GDBRemoteCommunicationServerLLGS _server,
- bool reverse_connect, const char *const host_and_port,
+ bool reverse_connect, llvm::StringRef host_and_port,
const char *const progname, const char *const subcommand,
const char *const named_pipe_path, pipe_t unnamed_pipe,
[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption
labath marked 2 inline comments as done.
labath added inline comments.
Comment at: lldb/source/Utility/Args.cpp:178
+Args::Args(llvm::ArrayRef args) : Args() {
+ for (llvm::StringRef arg : args)
JDevlieghere wrote:
> Maybe StringList should have a ctor that takes an `ArrayRef`, but
> probably not worth the extra copies here.
I think it can have that constructor if it's needed (though I'd rather delete
that class altogether), but I think this constructor would be good to have
regardless.
Comment at: lldb/tools/lldb-server/CMakeLists.txt:61
LINK_COMPONENTS
Support
)
MaskRay wrote:
> Otherwise it fails in a BUILD_SHARED_LIBS=on build because the -Wl,-z,defs
> linker option requires a module to have fully specified dependencies
Thanks.
Comment at: lldb/tools/lldb-server/lldb-gdbserver.cpp:414
+ std::string HelpText =
+ "Use '" + Name + " --help' for a complete list of options.\n";
JDevlieghere wrote:
> Should we extract this code into a utility that we can use here and in the
> lldb driver?
I started implementing that but then I realized we the two are on the opposite
sides of the SB API so we don't have a very good place to put it. I don't think
this line of text is worth the trouble of figuring that out.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89477/new/
https://reviews.llvm.org/D89477
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption
DavidSpickett added inline comments. Comment at: lldb/tools/lldb-server/lldb-gdbserver.cpp:370 + If no target is selected a startup, the lldb-server can be directed to launch + or attach a process by the LLDB client. + `attach a process` -> `attach to a process` Also maybe it reads better as: `the lldb-server can be directed by the LLDB client to launch or attach to a process.` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89477/new/ https://reviews.llvm.org/D89477 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption
MaskRay added inline comments. Comment at: lldb/tools/lldb-server/lldb-gdbserver.cpp:369 + + If no target is selected a startup, the lldb-server can be directed to launch + or attach a process by the LLDB client. `a startup` -> `at startup` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89477/new/ https://reviews.llvm.org/D89477 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption
MaskRay added inline comments. Comment at: lldb/tools/lldb-server/lldb-gdbserver.cpp:404 + unsigned MissingArgCount; + opt::InputArgList Args = Opts.ParseArgs(makeArrayRef(argv + 2, argc - 2), + MissingArgIndex, MissingArgCount); Consider using `parseArgs` I added in D83639 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89477/new/ https://reviews.llvm.org/D89477 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption
MaskRay added inline comments. Comment at: lldb/tools/lldb-server/CMakeLists.txt:61 LINK_COMPONENTS Support ) Otherwise it fails in a BUILD_SHARED_LIBS=on build because the -Wl,-z,defs linker option requires a module to have fully specified dependencies Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89477/new/ https://reviews.llvm.org/D89477 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D89477: [lldb] Port lldb gdb-server to libOption
JDevlieghere added inline comments.
Comment at: lldb/source/Utility/Args.cpp:178
+Args::Args(llvm::ArrayRef args) : Args() {
+ for (llvm::StringRef arg : args)
Maybe StringList should have a ctor that takes an `ArrayRef`, but
probably not worth the extra copies here.
Comment at: lldb/tools/lldb-server/lldb-gdbserver.cpp:414
+ std::string HelpText =
+ "Use '" + Name + " --help' for a complete list of options.\n";
Should we extract this code into a utility that we can use here and in the lldb
driver?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89477/new/
https://reviews.llvm.org/D89477
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
