gdbserver: switch to using getopt_long for argument processingHEADmaster

Update gdbserver to use getopt_long for argument processing.  This
turned out to be easier than I expected.

Interesting parts of this patch are:

I pass '+' as the OPTSTRING to getopt_long, this prevents getopt_long
from reordering the contents of ARGV.  This is needed so that things
like this will work:

  gdbserver :54321 program --arg1 --arg2

Without the '+', getopt_long will reorder ARGV, moving '--arg1' and
'--arg2' forward and handling them as arguments to gdbserver.

Because of this (not reordering) and to maintain backward
compatibility, we retain a special case to deal with '--attach' which
can appear after the PORT, like this:

  gdbserver :54321 --attach PID

I did consider adding a warning to this special case, informing the
user that placing --attach after the PORT was deprecated, but in the
end I didn't want to really change the behaviour as part of this
commit, so I've left that as an optional change for the future.

The getopt_long function supports two value passing forms, there is
'--option=value', and also '--option value'.  Traditionally, gdbserver
only supports the first of these.  To maintain this behaviour, after
the call to getopt_long, I spot if '--option value' was used, and if
so, modify the state so that it seems that no value was passed, and
that 'value' is the next ARGV entry to be parsed.  We could, possibly,
remove this code in the future, but that would be a functional change,
which is not something I want in this commit.

Handling of "-" for stdio connection has now moved out of the argument
processing loop as '-' isn't considered a valid option by getopt_long,
this is an improvement as all PORT handling is now in one place.

I've tried as much as possible to leave user visible functionality
unchanged after this commit, and as far as I can tell from testing,
nothing has changed.

Approved-By: Tom Tromey <tom@tromey.com>

1 files changed, 247 insertions, 153 deletions

diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index c772080..4037b1c 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -53,6 +53,8 @@
 #include "gdbsupport/gdb_argv_vec.h"
 #include "gdbsupport/remote-args.h"
 
+#include <getopt.h>
+
 /* PBUFSIZ must also be at least as big as IPA_CMD_BUF_SIZE, because
    the client state data is passed directly to some agent
    functions.  */
@@ -4104,9 +4106,6 @@ static void test_registers_raw_compare_zero_length ()
 captured_main (int argc, char *argv[])
 {
   int pid;
-  char *arg_end;
-  const char *port = NULL;
-  char **next_arg = &argv[1];
   volatile bool multi_mode = false;
   volatile bool attach = false;
   bool selftest = false;
@@ -4128,184 +4127,278 @@ captured_main (int argc, char *argv[])
 	     safe_strerror (errno));
     }
 
-  while (*next_arg != NULL && **next_arg == '-')
-    {
-      if (strcmp (*next_arg, "--version") == 0)
+  enum opts { OPT_VERSION = 1, OPT_HELP, OPT_ATTACH, OPT_MULTI, OPT_WRAPPER,
+    OPT_DEBUG, OPT_DEBUG_FILE, OPT_DEBUG_FORMAT, OPT_DISABLE_PACKET,
+    OPT_DISABLE_RANDOMIZATION, OPT_NO_DISABLE_RANDOMIZATION,
+    OPT_STARTUP_WITH_SHELL, OPT_NO_STARTUP_WITH_SHELL, OPT_ONCE,
+    OPT_SELFTEST,
+  };
+
+  static struct option longopts[] =
+    {
+      {"version", no_argument, nullptr, OPT_VERSION},
+      {"help", no_argument, nullptr, OPT_HELP},
+      {"attach", no_argument, nullptr, OPT_ATTACH},
+      {"multi", no_argument, nullptr, OPT_MULTI},
+      {"wrapper", no_argument, nullptr, OPT_WRAPPER},
+      {"debug", optional_argument, nullptr, OPT_DEBUG},
+      {"debug-file", required_argument, nullptr, OPT_DEBUG_FILE},
+      {"debug-format", required_argument, nullptr, OPT_DEBUG_FORMAT},
+      /* --disable-packet is optional_argument only so that we can print a
+	 better help list when the argument is missing.  */
+      {"disable-packet", optional_argument, nullptr, OPT_DISABLE_PACKET},
+      {"disable-randomization", no_argument, nullptr,
+       OPT_DISABLE_RANDOMIZATION},
+      {"no-disable-randomization", no_argument, nullptr,
+       OPT_NO_DISABLE_RANDOMIZATION},
+      {"startup-with-shell", no_argument, nullptr, OPT_STARTUP_WITH_SHELL},
+      {"no-startup-with-shell", no_argument, nullptr,
+       OPT_NO_STARTUP_WITH_SHELL},
+      {"once", no_argument, nullptr, OPT_ONCE},
+      {"selftest", optional_argument, nullptr, OPT_SELFTEST},
+      {nullptr, no_argument, nullptr, 0}
+    };
+
+  /* Ask getopt_long not to print error messages, we'll do that ourselves.
+     Look for handling of '?' from getopt_long.  */
+  opterr = 0;
+
+  int optc, longindex;
+
+  /* The '+' passed to getopt_long here stops ARGV being reordered.  In a
+     command line like: 'gdbserver PORT program --arg1 --arg2', the
+     '--arg1' and '--arg2' are arguments to 'program', not to gdbserver.
+     If getopt_long is free to reorder ARGV then it will try to steal those
+     arguments for itself.  */
+  while ((longindex = -1,
+	  optc = getopt_long (argc, argv, "+", longopts, &longindex)) != -1)
+    {
+      /* We only support '--option=value' form, not '--option value'.  To
+	 achieve this, if global OPTARG points to the start of the previous
+	 ARGV entry, then we must have used the second (unsupported) form,
+	 so set OPTARG to NULL and decrement OPTIND to make it appear that
+	 there was no value passed.  If the option requires an argument,
+	 then this means we should convert OPTC to '?' to indicate an
+	 error.  */
+      if (longindex != -1
+	  && longopts[longindex].has_arg != no_argument)
 	{
-	  gdbserver_version ();
-	  exit (0);
+	  if (optarg == argv[optind - 1])
+	    {
+	      optarg = nullptr;
+	      --optind;
+	    }
+
+	  if (longopts[longindex].has_arg == required_argument
+	      && optarg == nullptr)
+	    optc = '?';
 	}
-      else if (strcmp (*next_arg, "--help") == 0)
+
+      switch (optc)
 	{
+	case OPT_VERSION:
+	  gdbserver_version ();
+	  exit (0);
+
+	case OPT_HELP:
 	  gdbserver_usage (stdout);
 	  exit (0);
-	}
-      else if (strcmp (*next_arg, "--attach") == 0)
-	attach = true;
-      else if (strcmp (*next_arg, "--multi") == 0)
-	multi_mode = true;
-      else if (strcmp (*next_arg, "--wrapper") == 0)
-	{
-	  char **tmp;
 
-	  next_arg++;
+	case OPT_ATTACH:
+	  attach = true;
+	  break;
 
-	  tmp = next_arg;
-	  while (*next_arg != NULL && strcmp (*next_arg, "--") != 0)
-	    {
-	      wrapper_argv += *next_arg;
-	      wrapper_argv += ' ';
-	      next_arg++;
-	    }
+	case OPT_MULTI:
+	  multi_mode = true;
+	  break;
 
-	  if (!wrapper_argv.empty ())
-	    {
-	      /* Erase the last whitespace.  */
-	      wrapper_argv.erase (wrapper_argv.end () - 1);
-	    }
+	case OPT_WRAPPER:
+	  {
+	    int original_optind = optind;
 
-	  if (next_arg == tmp || *next_arg == NULL)
-	    {
-	      gdbserver_usage (stderr);
-	      exit (1);
-	    }
+	    while (argv[optind] != nullptr
+		   && strcmp (argv[optind], "--") != 0)
+	      {
+		wrapper_argv += argv[optind];
+		wrapper_argv += ' ';
+		++optind;
+	      }
 
-	  /* Consume the "--".  */
-	  *next_arg = NULL;
-	}
-      else if (startswith (*next_arg, "--debug="))
-	{
-	  try
-	    {
-	      parse_debug_options ((*next_arg) + sizeof ("--debug=") - 1);
-	    }
-	  catch (const gdb_exception_error &exception)
-	    {
-	      fflush (stdout);
-	      fprintf (stderr, "gdbserver: %s\n", exception.what ());
-	      exit (1);
-	    }
-	}
-      else if (strcmp (*next_arg, "--debug") == 0)
-	{
-	  try
-	    {
-	      parse_debug_options ("");
-	    }
-	  catch (const gdb_exception_error &exception)
-	    {
-	      fflush (stdout);
-	      fprintf (stderr, "gdbserver: %s\n", exception.what ());
-	      exit (1);
-	    }
-	}
-      else if (startswith (*next_arg, "--debug-format="))
-	{
-	  std::string error_msg
-	    = parse_debug_format_options ((*next_arg)
-					  + sizeof ("--debug-format=") - 1, 0);
+	    if (!wrapper_argv.empty ())
+	      {
+		/* Erase the last whitespace.  */
+		wrapper_argv.erase (wrapper_argv.end () - 1);
+	      }
 
-	  if (!error_msg.empty ())
-	    {
-	      fprintf (stderr, "%s", error_msg.c_str ());
-	      exit (1);
-	    }
-	}
-      else if (startswith (*next_arg, "--debug-file="))
-	debug_set_output ((*next_arg) + sizeof ("--debug-file=") -1);
-      else if (strcmp (*next_arg, "--disable-packet") == 0)
-	{
-	  gdbserver_show_disableable (stdout);
-	  exit (1);
-	}
-      else if (startswith (*next_arg, "--disable-packet="))
-	{
-	  char *packets = *next_arg += sizeof ("--disable-packet=") - 1;
-	  char *saveptr;
-	  for (char *tok = strtok_r (packets, ",", &saveptr);
-	       tok != NULL;
-	       tok = strtok_r (NULL, ",", &saveptr))
-	    {
-	      if (strcmp ("vCont", tok) == 0)
-		disable_packet_vCont = true;
-	      else if (strcmp ("Tthread", tok) == 0)
-		disable_packet_Tthread = true;
-	      else if (strcmp ("qC", tok) == 0)
-		disable_packet_qC = true;
-	      else if (strcmp ("qfThreadInfo", tok) == 0)
-		disable_packet_qfThreadInfo = true;
-	      else if (strcmp ("T", tok) == 0)
-		disable_packet_T = true;
-	      else if (strcmp ("threads", tok) == 0)
-		{
+	    if (original_optind == optind || argv[optind] == nullptr)
+	      {
+		gdbserver_usage (stderr);
+		exit (1);
+	      }
+
+	    /* Consume the "--".  */
+	    ++optind;
+	  }
+	  break;
+
+	case OPT_DEBUG:
+	  {
+	    const char *debug_opt = (optarg == nullptr) ? "" : optarg;
+	    try
+	      {
+		parse_debug_options (debug_opt);
+	      }
+	    catch (const gdb_exception_error &exception)
+	      {
+		fflush (stdout);
+		fprintf (stderr, "gdbserver: %s\n", exception.what ());
+		exit (1);
+	      }
+	  }
+	  break;
+
+	case OPT_DEBUG_FILE:
+	  {
+	    gdb_assert (optarg != nullptr);
+	    debug_set_output (optarg);
+	  }
+	  break;
+
+	case OPT_DEBUG_FORMAT:
+	  {
+	    gdb_assert (optarg != nullptr);
+	    std::string error_msg
+	      = parse_debug_format_options (optarg, 0);
+
+	    if (!error_msg.empty ())
+	      {
+		fprintf (stderr, "%s", error_msg.c_str ());
+		exit (1);
+	      }
+	  }
+	  break;
+
+	case OPT_DISABLE_PACKET:
+	  {
+	    char *packets = optarg;
+	    if (packets == nullptr)
+	      {
+		gdbserver_show_disableable (stdout);
+		exit (1);
+	      }
+	    char *saveptr;
+	    for (char *tok = strtok_r (packets, ",", &saveptr);
+		 tok != nullptr;
+		 tok = strtok_r (nullptr, ",", &saveptr))
+	      {
+		if (strcmp ("vCont", tok) == 0)
 		  disable_packet_vCont = true;
+		else if (strcmp ("Tthread", tok) == 0)
 		  disable_packet_Tthread = true;
+		else if (strcmp ("qC", tok) == 0)
 		  disable_packet_qC = true;
+		else if (strcmp ("qfThreadInfo", tok) == 0)
 		  disable_packet_qfThreadInfo = true;
-		}
-	      else
-		{
-		  fprintf (stderr, "Don't know how to disable \"%s\".\n\n",
-			   tok);
-		  gdbserver_show_disableable (stderr);
-		  exit (1);
-		}
-	    }
-	}
-      else if (strcmp (*next_arg, "-") == 0)
-	{
-	  /* "-" specifies a stdio connection and is a form of port
-	     specification.  */
-	  port = STDIO_CONNECTION_NAME;
+		else if (strcmp ("T", tok) == 0)
+		  disable_packet_T = true;
+		else if (strcmp ("threads", tok) == 0)
+		  {
+		    disable_packet_vCont = true;
+		    disable_packet_Tthread = true;
+		    disable_packet_qC = true;
+		    disable_packet_qfThreadInfo = true;
+		  }
+		else
+		  {
+		    fprintf (stderr, "Don't know how to disable \"%s\".\n\n",
+			     tok);
+		    gdbserver_show_disableable (stderr);
+		    exit (1);
+		  }
+	      }
+	  }
+	  break;
 
-	  /* Implying --once here prevents a hang after stdin has been closed.  */
-	  run_once = true;
+	case OPT_DISABLE_RANDOMIZATION:
+	  cs.disable_randomization = 1;
+	  break;
 
-	  next_arg++;
+	case OPT_NO_DISABLE_RANDOMIZATION:
+	  cs.disable_randomization = 0;
+	  break;
+
+	case OPT_STARTUP_WITH_SHELL:
+	  startup_with_shell = true;
+	  break;
+
+	case OPT_NO_STARTUP_WITH_SHELL:
+	  startup_with_shell = false;
+	  break;
+
+	case OPT_ONCE:
+	  run_once = true;
 	  break;
-	}
-      else if (strcmp (*next_arg, "--disable-randomization") == 0)
-	cs.disable_randomization = 1;
-      else if (strcmp (*next_arg, "--no-disable-randomization") == 0)
-	cs.disable_randomization = 0;
-      else if (strcmp (*next_arg, "--startup-with-shell") == 0)
-	startup_with_shell = true;
-      else if (strcmp (*next_arg, "--no-startup-with-shell") == 0)
-	startup_with_shell = false;
-      else if (strcmp (*next_arg, "--once") == 0)
-	run_once = true;
-      else if (strcmp (*next_arg, "--selftest") == 0)
-	selftest = true;
-      else if (startswith (*next_arg, "--selftest="))
-	{
-	  selftest = true;
 
+	case OPT_SELFTEST:
+	  {
+	    selftest = true;
+	    if (optarg != nullptr)
+	      {
 #if GDB_SELF_TEST
-	  const char *filter = *next_arg + strlen ("--selftest=");
-	  if (*filter == '\0')
-	    {
-	      fprintf (stderr, _("Error: selftest filter is empty.\n"));
-	      exit (1);
-	    }
+		if (*optarg == '\0')
+		  {
+		    fprintf (stderr, _("Error: selftest filter is empty.\n"));
+		    exit (1);
+		  }
 
-	  selftest_filters.push_back (filter);
+		selftest_filters.push_back (optarg);
 #endif
-	}
-      else
-	{
-	  fprintf (stderr, "Unknown argument: %s\n", *next_arg);
+	      }
+	  }
+	  break;
+
+	case '?':
+	  /* Figuring out which element of ARGV contained the invalid
+	     argument is not simple.  There are a couple of cases we need
+	     to consider.
+
+	     (1) Something like '-x'.  gdbserver doesn't support single
+		 character options, so a '-' followed by a character is
+		 always invalid.  In this case global OPTOPT will be set to
+		 'x', and global OPTIND will point to the next ARGV entry.
+
+	     (2) Something like '-xyz'.  gdbserver doesn't support single
+		 dash arguments for its command line options.  The
+		 getopt_long call treats this like '-x -y -z', in which
+		 case global OPTOPT is set to 'x' and global OPTIND will
+		 point to this ARGV entry.
+
+	     (3) Something like '--unknown'.  This is just an unknown
+		 double dash argument.  Global OPTOPT is set to '\0', and
+		 global OPTIND points to the next ARGV entry.  */
+	  std::string bad_arg;
+	  if (optopt == '\0' || argv[optind] == nullptr
+	      || argv[optind][0] != '-' || argv[optind][1] != optopt)
+	    bad_arg = argv[optind - 1];
+	  else
+	    bad_arg = argv[optind];
+
+	  fprintf (stderr, "Unknown argument: %s\n", bad_arg.c_str ());
 	  exit (1);
 	}
-
-      next_arg++;
-      continue;
     }
 
-  if (port == NULL)
+  const char *port = argv[optind];
+  ++optind;
+  if (port != nullptr && strcmp (port, "-") == 0)
     {
-      port = *next_arg;
-      next_arg++;
+      port = STDIO_CONNECTION_NAME;
+
+      /* Implying --once here prevents a hang after stdin has been closed.  */
+      run_once = true;
     }
+
+  char **next_arg = &argv[optind];
   if ((port == NULL || (!attach && !multi_mode && *next_arg == NULL))
        && !selftest)
     {
@@ -4337,6 +4430,7 @@ captured_main (int argc, char *argv[])
       next_arg++;
     }
 
+  char *arg_end;
   if (attach
       && (*next_arg == NULL
 	  || (*next_arg)[0] == '\0'