[PATCH 0/3] Improve debug output support in gdbserver

[PATCH 0/3] Improve debug output support in gdbserver

[Andrew Burgess]

For another patch I'm working on I wanted to add some additional debug
output to gdbserver.

Right now there appears to be two broad approaches I could take, these are:

  1. Just use threads_debug_printf.  The comments in debug.h even
  claim that this is really the "general" gdbserver debug output and
  that the "threads" here is just historic, but that feels like a bit
  of a cop out, we do have separate remote and event-loop debug
  control.  I think what really happened is folk just started reusing
  the threads_debug_printf rather than adding a new debug category,
  and over time we just resigned ourselves to threads actually being
  general output...  I think we can do better than that, so

  2. We can add a whole new debug category, with a new *_debug_printf
  function.  That I think would be better, but to control this new
  debug flag we then need to add a new command line flag (we already
  have --debug, --remote-debug, and --event-loop-debug), and then we
  need to add new monitor commands to control this new debug
  setting... this begins to feel not very scalable.

So, in this series I try to reimagine debug control in gdbserver, but
(hopefully) retain backwards compatibility.

The gdbserver command line flag now takes an optional list of
components, so we can do:

  gdbserver --debug=remote,threads

to enable 'remote' and 'threads' debug.  The default if no components
are listed is 'threads', which retains the backwards compatibility.
I've also retained the two additional flags '--remote-debug' and
'--event-loop-debug'.

QUESTION: How would folk feel if I was super aggressive and removed
these older flags?  In theory these flags are only used for debugging
gdbserver itself, but who knows, right?

And on the monitor command side, we now support:

  (gdb) monitor set debug COMPONENT 1
  (gdb) monitor set debug COMPONENT 0

to enable and disable debug for COMPONENT.  Where COMPONENT is again,
'threads', 'remote', and 'event-loop'.  Again, I've retained:

  (gdb) set debug 1
  (gdb) set debug 0

which is equivalent to 'set debug threads 1' or 'set debug threads 0'.
And I've also retained the legacy:

  (gdb) set remote-debug 1
  (gdb) set remote-debug 0
  (gdb) set event-loop-debug 1
  (gdb) set event-loop-debug 0
  
QUESTION: As with the command line, how would people feel if I ripped
out all of the legacy support and required folks to move to the newer
command set?

---

Andrew Burgess (3):
  gdbserver: cleanup monitor_show_help
  gdbserver: allow the --debug command line option to take a value
  gdbserver: allow for general 'monitor set debug COMPONENT VALUE' use

 gdb/NEWS                                |   7 +
 gdb/doc/gdb.texinfo                     |  78 ++++++++--
 gdb/testsuite/gdb.server/server-mon.exp |   6 +
 gdbserver/server.cc                     | 183 ++++++++++++++++++++++--
 4 files changed, 252 insertions(+), 22 deletions(-)


base-commit: 2029e13917d53d2289d3ebb390c4f40bd2112d21
-- 
2.25.4

[PATCH 1/3] gdbserver: cleanup monitor_show_help

[Andrew Burgess]

After this commit:

  commit 0b04e52316079981b2b77124198a405d826a05cd
  Date:   Sun Jan 19 14:33:37 2014 -0700

      link gdbserver against libiberty

we can cleanup how the help text is generated in monitor_show_help.
This doesn't change the output that the user will see -- it just folds
multiple monitor_output calls into one.

There should be no user visible change after this commit.
---
 gdbserver/server.cc | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 5f2032c37c1..5451d43df18 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1032,9 +1032,7 @@ monitor_show_help (void)
   monitor_output ("    Enable event loop debugging messages\n");
   monitor_output ("  set debug-format option1[,option2,...]\n");
   monitor_output ("    Add additional information to debugging messages\n");
-  monitor_output ("    Options: all, none");
-  monitor_output (", timestamp");
-  monitor_output ("\n");
+  monitor_output ("    Options: all, none, timestamp\n");
   monitor_output ("  exit\n");
   monitor_output ("    Quit GDBserver\n");
 }
-- 
2.25.4

[PATCH 2/3] gdbserver: allow the --debug command line option to take a value

[Andrew Burgess]

Currently, gdbserver has the following command line options related to
debugging output:

  --debug
  --remote-debug
  --event-loop-debug

This doesn't scale well.  If I want an extra debug component we need
to add another command line flag.

This commit changes --debug to take a list of components, I've then
deprecated the --remote-debug and --event-loop-debug options by
removing them from the --help output and man page, and by indicating
that these options are deprecated in the GDB manual.

The currently supported components are: all, threads, remote, and
event-loop.  The 'threads' component represents the debug we currently
get from --debug.  And if --debug is used without a component list
then the threads component is assumed as the default.

Currently the threads component actually includes a lot of output that
is not really threads related.  In the future I'd like to see this
split up and additional components added.

The special component 'all' does what you'd expect: enables debug
output for all supported components.

The component list is parsed left to write, and you can prefix a
component with '-' to disable that component, so I can write:

  target> gdbserver --debug=all,-event-loop

to get debug for all components except the event-loop component.

In this commit I've only update the command line options, in the next
commit I'll update the monitor commands to support a similar
interface.
---
 gdb/NEWS            |   7 ++++
 gdb/doc/gdb.texinfo |  62 +++++++++++++++++++++++----
 gdbserver/server.cc | 100 ++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 156 insertions(+), 13 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 6022def1037..13cf962dfcf 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -6,6 +6,13 @@
 * GDB index now contains information about the main function.  This speeds up
   startup when it is being used for some large binaries.
 
+* New features in the GDB remote stub, GDBserver
+
+  ** The `--debug` option now takes an (optional) list of components
+     to emit debug for.  The currently supported components are all,
+     threads, event-loop, and remote.  This allows the
+     --event-loop-debug and --remote-debug options to be deprecated.
+
 * Python API
 
   ** New function gdb.notify_mi(NAME, DATA), that emits custom
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index db1a82ec838..d3f088f6505 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -23664,11 +23664,47 @@
 @pxref{--multi Option in Types of Remote Connnections}.
 
 @cindex @option{--debug}, @code{gdbserver} option
-The @option{--debug} option tells @code{gdbserver} to display extra
-status information about the debugging process.
+The @option{--debug[=option1,option2,...]} option tells
+@code{gdbserver} to display extra diagnostic information about the
+debugging process.  The @var{option1}, @var{option2}, etc control for
+which areas of @code{gdbserver} additional information will be
+displayed, possible values are:
+
+@table @code
+@item all
+This enables all available diagnostic output.
+@item threads
+This enables diagnostic output related to threading.  Currently other
+general diagnostic output is included in this category, but this could
+change in future releases of @code{gdbserver}.
+@item event-loop
+This enables event-loop specific diagnostic output.
+@item remote
+This enables diagnostic output relating to the transfer of remote
+protocol packets too and from the debugger.
+@end table
+
+@noindent
+If no options are passed to @option{--debug} then this is treated as
+equivalent to @option{--debug=threads}.  This could change in future
+releases of @code{gdbserver}.  The options passed to @option{--debug}
+are processed left to right, and individual options can be prefixed
+with the @kbd{-} (minus) character to disable diagnostic output from
+this area, so it is possible to use:
+
+@smallexample
+  target> gdbserver --debug=all,-event-loop
+@end smallexample
+
+@noindent
+In order to enable all diagnostic output except that for the
+event-loop.
+
 @cindex @option{--remote-debug}, @code{gdbserver} option
-The @option{--remote-debug} option tells @code{gdbserver} to display
-remote protocol debug output.
+The @option{--remote-debug} option is deprecated, but retained for
+backward compatibility.  This is equivalent to
+@option{--debug=remote}.
+
 @cindex @option{--debug-file}, @code{gdbserver} option
 @cindex @code{gdbserver}, send all debug output to a single file
 The @option{--debug-file=@var{filename}} option tells @code{gdbserver} to
@@ -50531,16 +50567,24 @@
 target> gdbserver --multi @var{comm}
 @end smallexample
 
-@item --debug
+@item --debug@r{[}=option1,option2,...@r{]}
 Instruct @code{gdbserver} to display extra status information about the debugging
 process.
 This option is intended for @code{gdbserver} development and for bug reports to
 the developers.
 
-@item --remote-debug
-Instruct @code{gdbserver} to display remote protocol debug output.
-This option is intended for @code{gdbserver} development and for bug reports to
-the developers.
+Each @var{option} is the name of a component for which debugging
+should be enabled.  The list of possible options is @option{all},
+@option{threads}, @option{event-loop}, @option{remote}.  The special
+option @option{all} enables all components.  The option list is
+processed left to right, and an option can be prefixed with the
+@kbd{-} character to disable output for that component, so you could write:
+
+@smallexample
+target> gdbserver --debug=all,-event-loop
+@end smallexample
+
+@noindent to turn on debug output for all components except @option{event-loop}.
 
 @item --debug-file=@var{filename}
 Instruct @code{gdbserver} to send any debug output to the given @var{filename}.
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 5451d43df18..b954507dd6b 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1335,6 +1335,66 @@ parse_debug_format_options (const char *arg, int is_monitor)
   return std::string ();
 }
 
+/* Parse the options to --debug=...
+
+   OPTIONS is the string of debug components which should be enabled (or
+   disabled), and must not be nullptr.  An empty OPTIONS string is valid,
+   in which case a default set of debug components will be enabled.
+
+   An unknown, or otherwise invalid debug component will result in an
+   exception being thrown.
+
+   OPTIONS can consist of multiple debug component names separated by a
+   comma.  Debugging for each component will be turned on.  The special
+   component 'all' can be used to enable debugging for all components.
+
+   A component can also be prefixed with '-' to disable debugging of that
+   component, so a user might use: '--debug=all,-remote', to enable all
+   debugging, except for the remote (protocol) component.  Components are
+   processed left to write in the OPTIONS list.  */
+
+static void
+parse_debug_options (const char *options)
+{
+  gdb_assert (options != nullptr);
+
+  /* Empty options means the "default" set.  This exists mostly for
+     backwards compatibility with gdbserver's legacy behaviour.  */
+  if (*options == '\0')
+    {
+      debug_threads = true;
+      return;
+    }
+
+  while (*options != '\0')
+    {
+      const char *end = strchrnul (options, ',');
+
+      bool set_value = *options != '-';
+      if (*options == '-' || *options == '+')
+	++options;
+
+      gdb::string_view opt (options, end - options);
+
+      if (opt.size () == 0)
+	error ("invalid empty debug option");
+
+      bool is_opt_all = opt == "all";
+
+      if (is_opt_all || opt == "threads")
+	debug_threads = set_value;
+      else if (is_opt_all || opt == "event-loop")
+	debug_event_loop = (set_value ? debug_event_loop_kind::ALL
+			    : debug_event_loop_kind::OFF);
+      else if (is_opt_all || opt == "remote")
+	remote_debug = set_value;
+      else
+	error ("unknown debug option '%s'", to_string (opt).c_str ());
+
+      options = (*end == ',') ? end + 1 : end;
+    }
+}
+
 /* Handle monitor commands not handled by target-specific handlers.  */
 
 static void
@@ -3449,15 +3509,19 @@ gdbserver_usage (FILE *stream)
 	   "\n"
 	   "Debug options:\n"
 	   "\n"
-	   "  --debug               Enable general debugging output.\n"
+	   "  --debug[=OPT1,OPT2,...]\n"
+	   "                        Enable debugging output.\n"
+	   "                          Options:\n"
+	   "                            all, threads, event-loop, remote\n"
+	   "                          With no options, 'threads' is assumed.\n"
+	   "                          Prefix an option with '-' to disable\n"
+	   "                          debugging of that component.\n"
 	   "  --debug-format=OPT1[,OPT2,...]\n"
 	   "                        Specify extra content in debugging output.\n"
 	   "                          Options:\n"
 	   "                            all\n"
 	   "                            none\n"
 	   "                            timestamp\n"
-	   "  --remote-debug        Enable remote protocol debugging output.\n"
-	   "  --event-loop-debug    Enable event loop debugging output.\n"
 	   "  --disable-packet=OPT1[,OPT2,...]\n"
 	   "                        Disable support for RSP packets or features.\n"
 	   "                          Options:\n"
@@ -3736,8 +3800,32 @@ captured_main (int argc, char *argv[])
 	  /* 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)
-	debug_threads = true;
+	{
+	  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
@@ -3750,8 +3838,12 @@ captured_main (int argc, char *argv[])
 	      exit (1);
 	    }
 	}
+      /* This option is retained for backwards compatibility.  It is
+	 considered deprecated, and might be removed one day.  */
       else if (strcmp (*next_arg, "--remote-debug") == 0)
 	remote_debug = true;
+      /* This option is retained for backwards compatibility.  It is
+	 considered deprecated, and might be removed one day.  */
       else if (strcmp (*next_arg, "--event-loop-debug") == 0)
 	debug_event_loop = debug_event_loop_kind::ALL;
       else if (startswith (*next_arg, "--debug-file="))
-- 
2.25.4

[PATCH 3/3] gdbserver: allow for general 'monitor set debug COMPONENT VALUE' use

[Andrew Burgess]

Building on the last commit, which added a general --debug=COMPONENT
option to the gdbserver command line, this commit extends the monitor
command to allow for general:

  (gdb) monitor set debug COMPONENT 0|1

style commands.  Just like with the previous commit, the COMPONENT can
be any one of threads, remote, event-loop, and correspond to the same
set of global debug flags.

While on the command line it is possible to do:

  --debug=remote,event-loop,threads

the components have to be entered one at a time with the monitor
command, and also, the 'all' component (see previous commit) is not
supported.  This makes the monitor command usage similar to how GDB's
normal 'set debug ...'  commands work.

I've retained the existing monitor command for backwards
compatibility, though I have removed them from the monitor help
output.
---
 gdb/doc/gdb.texinfo                     | 16 ++++-
 gdb/testsuite/gdb.server/server-mon.exp |  6 ++
 gdbserver/server.cc                     | 79 +++++++++++++++++++++++--
 3 files changed, 95 insertions(+), 6 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index d3f088f6505..87993ac924a 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -23804,11 +23804,23 @@
 @itemx monitor set debug 1
 Disable or enable general debugging messages.
 
-@item monitor set remote-debug 0
-@itemx monitor set remote-debug 1
+@item monitor set debug threads 0
+@itemx monitor set debug threads 1
+Disable or enable specific debugging messages associated thread
+handling in gdbserver.  Currently this category also includes
+additional debug output not specifically related to thread handling,
+this could change in future releases of gdbserver.
+
+@item monitor set debug remote 0
+@itemx monitor set debug remote 1
 Disable or enable specific debugging messages associated with the remote
 protocol (@pxref{Remote Protocol}).
 
+@item monitor set debug event-loop 0
+@itemx monitor set debug event-loop 1
+Disable or enable specific debugging messages associated with
+gdbserver's event-loop.
+
 @item monitor set debug-file filename
 @itemx monitor set debug-file
 Send any debug output to the given file, or to stderr.
diff --git a/gdb/testsuite/gdb.server/server-mon.exp b/gdb/testsuite/gdb.server/server-mon.exp
index 728cc84a9e0..b2ad07254be 100644
--- a/gdb/testsuite/gdb.server/server-mon.exp
+++ b/gdb/testsuite/gdb.server/server-mon.exp
@@ -46,6 +46,12 @@ gdb_test "monitor" "Unknown monitor command.*Protocol error.*"
 
 gdb_test "monitor set debug 1" "Debug output enabled\\."
 gdb_test "monitor set debug 0" "Debug output disabled\\."
+gdb_test "monitor set debug remote 1" "Debug output for 'remote' enabled\\."
+gdb_test "monitor set debug remote 0" "Debug output for 'remote' disabled\\."
+gdb_test "monitor set debug event-loop 1" "Debug output for 'event-loop' enabled\\."
+gdb_test "monitor set debug event-loop 0" "Debug output for 'event-loop' disabled\\."
+gdb_test "monitor set debug threads 1" "Debug output for 'threads' enabled\\."
+gdb_test "monitor set debug threads 0" "Debug output for 'threads' disabled\\."
 gdb_test "monitor set remote-debug 1" "Protocol debug output enabled\\."
 gdb_test "monitor set remote-debug 0" "Protocol debug output disabled\\."
 gdb_test "monitor set debug-format all" \
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index b954507dd6b..46625ce5556 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1024,12 +1024,11 @@ monitor_show_help (void)
   monitor_output ("The following monitor commands are supported:\n");
   monitor_output ("  set debug <0|1>\n");
   monitor_output ("    Enable general debugging messages\n");
+  monitor_output ("  set debug COMPONENT <0|1>\n");
+  monitor_output ("    Enable debugging messages for COMPONENT, which is\n");
+  monitor_output ("    one of: threads, remote, event-loop.\n");
   monitor_output ("  set debug-hw-points <0|1>\n");
   monitor_output ("    Enable h/w breakpoint/watchpoint debugging messages\n");
-  monitor_output ("  set remote-debug <0|1>\n");
-  monitor_output ("    Enable remote protocol debugging messages\n");
-  monitor_output ("  set event-loop-debug <0|1>\n");
-  monitor_output ("    Enable event loop debugging messages\n");
   monitor_output ("  set debug-format option1[,option2,...]\n");
   monitor_output ("    Add additional information to debugging messages\n");
   monitor_output ("    Options: all, none, timestamp\n");
@@ -1395,6 +1394,66 @@ parse_debug_options (const char *options)
     }
 }
 
+/* Called from the 'monitor' command handler, to handle general 'set debug'
+   commands with the format: 'set debug COMPONENT VALUE' where VALUE is
+   either '0' or '1', and COMPONENT is the name of a supported debug
+   component, see parse_debug_options for the supported components.  MON
+   should point to the 'COMPONENT VALUE' part of the string, the 'set
+   debug' should already have been stripped off.
+
+   Return a string containing an error message if something goes wrong,
+   this error can be returned as part of the monitor command output.  If
+   everything goes correctly then the debug global will have been updated,
+   and an empty string is returned.  */
+
+static std::string
+handle_general_monitor_debug (const char *mon)
+{
+  mon = skip_spaces (mon);
+
+  if (*mon == '\0')
+    return "No debug component name found.\n";
+
+  /* Skip to next space.  */
+  const char *end = skip_to_space (mon);
+  std::string component (mon, end - mon);
+  if (component == "all")
+    return "Component 'all' is not supported.\n";
+  if (component.find (',') != component.npos || component[0] == '-'
+      || component[0] == '+')
+    return "Invalid character found in debug component name.\n";
+
+  /* Skip any spaces.  */
+  const char *value = skip_spaces (end);
+
+  /* Check next character is either '0' or '1'.  */
+  if (*value != '0' && *value != '1')
+    return "Invalid value for set debug.\n";
+  bool enable = *value == '1';
+  const char *after_value = skip_spaces (value + 1);
+  if (*after_value != '\0')
+    return "junk found at end of set debug command";
+
+  /* Build a string with either a '+' or '-' prefix and pass this to
+     parse_debug_options.  */
+  std::string action_str = std::string (enable ? "+" : "-") + component;
+
+  try
+    {
+      parse_debug_options (action_str.c_str ());
+      std::string msg = string_printf ("Debug output for '%s' %s.\n",
+				       component.c_str (),
+				       enable ? "enabled" : "disabled");
+      monitor_output (msg.c_str ());
+    }
+  catch (const gdb_exception_error &exception)
+    {
+      return string_printf ("Error: %s\n", exception.what ());
+    }
+
+  return {};
+}
+
 /* Handle monitor commands not handled by target-specific handlers.  */
 
 static void
@@ -1410,6 +1469,18 @@ handle_monitor_command (char *mon, char *own_buf)
       debug_threads = false;
       monitor_output ("Debug output disabled.\n");
     }
+  else if (startswith (mon, "set debug "))
+    {
+      std::string error_msg
+	= handle_general_monitor_debug (mon + sizeof ("set debug ") - 1);
+
+      if (!error_msg.empty ())
+	{
+	  monitor_output (error_msg.c_str ());
+	  monitor_show_help ();
+	  write_enn (own_buf);
+	}
+    }
   else if (strcmp (mon, "set debug-hw-points 1") == 0)
     {
       show_debug_regs = 1;
-- 
2.25.4

Re: [PATCH 2/3] gdbserver: allow the --debug command line option to take a value

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Tue,  7 Nov 2023 18:03:22 +0000
> 
>  gdb/NEWS            |   7 ++++
>  gdb/doc/gdb.texinfo |  62 +++++++++++++++++++++++----
>  gdbserver/server.cc | 100 ++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 156 insertions(+), 13 deletions(-)

Thanks.

> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -6,6 +6,13 @@
>  * GDB index now contains information about the main function.  This speeds up
>    startup when it is being used for some large binaries.
>  
> +* New features in the GDB remote stub, GDBserver
> +
> +  ** The `--debug` option now takes an (optional) list of components
            ^^^^^^^^^
That's not how we quote in plain-ASCII documentation.  (And I think we
don't need quoting here anyway.)

>  @cindex @option{--debug}, @code{gdbserver} option
> -The @option{--debug} option tells @code{gdbserver} to display extra
> -status information about the debugging process.
> +The @option{--debug[=option1,option2,...]} option tells
                                        ^^^
Please use @dots{} instead of literally 3 dots.

> +@code{gdbserver} to display extra diagnostic information about the
> +debugging process.  The @var{option1}, @var{option2}, etc control for
                                                         ^^^
"etc.@:"

> +@item remote
> +This enables diagnostic output relating to the transfer of remote
                                  ^^^^^^^^
"related"

> -@item --debug
> +@item --debug@r{[}=option1,option2,...@r{]}
                                      ^^^
@dots{}

> +Each @var{option} is the name of a component for which debugging
> +should be enabled.  The list of possible options is @option{all},
> +@option{threads}, @option{event-loop}, @option{remote}.  The special
> +option @option{all} enables all components.  The option list is
> +processed left to right, and an option can be prefixed with the
> +@kbd{-} character to disable output for that component, so you could write:
> +
> +@smallexample
> +target> gdbserver --debug=all,-event-loop
> +@end smallexample
> +
> +@noindent to turn on debug output for all components except @option{event-loop}.

Why do we need this duplicate description?  I'd prefer to have a short
sentence with a cross-reference to the previous description, so that
we could have only one of them, not two.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 0/3] Improve debug output support in gdbserver

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> QUESTION: How would folk feel if I was super aggressive and removed
Andrew> these older flags?  In theory these flags are only used for debugging
Andrew> gdbserver itself, but who knows, right?

I think it would be completely fine.  I think it would be a bad idea for
us to promise that debug flags won't change, and the current approach in
gdbserver isn't really all that great.

Andrew> And on the monitor command side, we now support:

Andrew>   (gdb) monitor set debug COMPONENT 1
Andrew>   (gdb) monitor set debug COMPONENT 0

Andrew> to enable and disable debug for COMPONENT.  Where COMPONENT is again,
Andrew> 'threads', 'remote', and 'event-loop'.  Again, I've retained:

Andrew>   (gdb) set debug 1
Andrew>   (gdb) set debug 0

Andrew> which is equivalent to 'set debug threads 1' or 'set debug threads 0'.
Andrew> And I've also retained the legacy:

Andrew>   (gdb) set remote-debug 1
Andrew>   (gdb) set remote-debug 0
Andrew>   (gdb) set event-loop-debug 1
Andrew>   (gdb) set event-loop-debug 0
  
Andrew> QUESTION: As with the command line, how would people feel if I ripped
Andrew> out all of the legacy support and required folks to move to the newer
Andrew> command set?

I think this would also be fine.

Tom

Re: [PATCH 0/3] Improve debug output support in gdbserver

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> For another patch I'm working on I wanted to add some additional debug
Andrew> output to gdbserver.

Andrew> Right now there appears to be two broad approaches I could take, these are:

[...]

I read through these and they look fine to me.

Tom

Re: [PATCH 1/3] gdbserver: cleanup monitor_show_help

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

> After this commit:
>
>   commit 0b04e52316079981b2b77124198a405d826a05cd
>   Date:   Sun Jan 19 14:33:37 2014 -0700
>
>       link gdbserver against libiberty
>
> we can cleanup how the help text is generated in monitor_show_help.
> This doesn't change the output that the user will see -- it just folds
> multiple monitor_output calls into one.
>
> There should be no user visible change after this commit.

I've pushed this one patch from this series.

Thanks,
Andrew


> ---
>  gdbserver/server.cc | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index 5f2032c37c1..5451d43df18 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -1032,9 +1032,7 @@ monitor_show_help (void)
>    monitor_output ("    Enable event loop debugging messages\n");
>    monitor_output ("  set debug-format option1[,option2,...]\n");
>    monitor_output ("    Add additional information to debugging messages\n");
> -  monitor_output ("    Options: all, none");
> -  monitor_output (", timestamp");
> -  monitor_output ("\n");
> +  monitor_output ("    Options: all, none, timestamp\n");
>    monitor_output ("  exit\n");
>    monitor_output ("    Quit GDBserver\n");
>  }
> -- 
> 2.25.4

Re: [PATCH 2/3] gdbserver: allow the --debug command line option to take a value

[Andrew Burgess]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: Andrew Burgess <aburgess@redhat.com>
>> Date: Tue,  7 Nov 2023 18:03:22 +0000
>> 
>>  gdb/NEWS            |   7 ++++
>>  gdb/doc/gdb.texinfo |  62 +++++++++++++++++++++++----
>>  gdbserver/server.cc | 100 ++++++++++++++++++++++++++++++++++++++++++--
>>  3 files changed, 156 insertions(+), 13 deletions(-)
>
> Thanks.
>
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -6,6 +6,13 @@
>>  * GDB index now contains information about the main function.  This speeds up
>>    startup when it is being used for some large binaries.
>>  
>> +* New features in the GDB remote stub, GDBserver
>> +
>> +  ** The `--debug` option now takes an (optional) list of components
>             ^^^^^^^^^
> That's not how we quote in plain-ASCII documentation.  (And I think we
> don't need quoting here anyway.)
>
>>  @cindex @option{--debug}, @code{gdbserver} option
>> -The @option{--debug} option tells @code{gdbserver} to display extra
>> -status information about the debugging process.
>> +The @option{--debug[=option1,option2,...]} option tells
>                                         ^^^
> Please use @dots{} instead of literally 3 dots.
>
>> +@code{gdbserver} to display extra diagnostic information about the
>> +debugging process.  The @var{option1}, @var{option2}, etc control for
>                                                          ^^^
> "etc.@:"
>
>> +@item remote
>> +This enables diagnostic output relating to the transfer of remote
>                                   ^^^^^^^^
> "related"
>
>> -@item --debug
>> +@item --debug@r{[}=option1,option2,...@r{]}
>                                       ^^^
> @dots{}
>
>> +Each @var{option} is the name of a component for which debugging
>> +should be enabled.  The list of possible options is @option{all},
>> +@option{threads}, @option{event-loop}, @option{remote}.  The special
>> +option @option{all} enables all components.  The option list is
>> +processed left to right, and an option can be prefixed with the
>> +@kbd{-} character to disable output for that component, so you could write:
>> +
>> +@smallexample
>> +target> gdbserver --debug=all,-event-loop
>> +@end smallexample
>> +
>> +@noindent to turn on debug output for all components except @option{event-loop}.
>
> Why do we need this duplicate description?  I'd prefer to have a short
> sentence with a cross-reference to the previous description, so that
> we could have only one of them, not two.

This is unfortunate.  One of these instances is the general gdb info
manual, while the second of these is the gdbserver man page.

Both of these are (maybe unfortunately?) included within the info
manual, so there is indeed duplication, but the pure man page only
includes one of these blocks.

I guess we could restructure things somehow so that the man page part is
used in some way such that the duplication is removed?  But that seems
like a bigger refactoring task; there is already lots of duplication
between the man page and the info manual, so I think trying to solve the
problem just for this one option doesn't make much sense.

Thanks,
Andrew

[PATCHv2 0/3] Improve debug output support in gdbserver

[Andrew Burgess]

Changes since v1:

  - I've now dropped the --remote-debug and --event-loop-debug command
    line options,

  - I've now dropped the 'monitor set remote-debug' and 'monitor set
    event-loop-debug' monitor commands,

  - I've fixed some of Eli's doc feedback.  We still have the
    duplication between the man page and info manual content.  I'm not
    sure if that can easily be fixed as part of this commit, or if
    that should be a separate piece of work,

  - Now accept on/off instead of 1/0 in the monitor commands,

  - Now allow 'all' as a component in the monitor commands.
    Preventing this before was pretty arbitrary, and on reflection
    seemed pointless,

  - Fixed a bug where the 'all' component would only actually enable
    the 'threads' component,

  - The first patch of the prevoius series has been pushed.  But I
    have a new first patch which fixes another gdbserver debug related
    issue.

---

For another patch I'm working on I wanted to add some additional debug
output to gdbserver.

Right now there appears to be two broad approaches I could take, these are:

  1. Just use threads_debug_printf.  The comments in debug.h even
  claim that this is really the "general" gdbserver debug output and
  that the "threads" here is just historic, but that feels like a bit
  of a cop out, we do have separate remote and event-loop debug
  control.  I think what really happened is folk just started reusing
  the threads_debug_printf rather than adding a new debug category,
  and over time we just resigned ourselves to threads actually being
  general output...  I think we can do better than that, so

  2. We can add a whole new debug category, with a new *_debug_printf
  function.  That I think would be better, but to control this new
  debug flag we then need to add a new command line flag (we already
  have --debug, --remote-debug, and --event-loop-debug), and then we
  need to add new monitor commands to control this new debug
  setting... this begins to feel not very scalable.

So, in this series I try to reimagine debug control in gdbserver.

The gdbserver command line flag now takes an optional list of
components, so we can do:

  gdbserver --debug=remote,threads

to enable 'remote' and 'threads' debug.  The default if no components
are listed is 'threads', which retains backwards compatibility with
the current behaviour, thought we might want to change this in the
future.

I've also removed the two existing flags '--remote-debug' and
'--event-loop-debug', instead we should now do '--debug=remote',
'--debug=event-loop', or '--debug=remote,event-loop' as needed.

And on the monitor command side, we now support:

  (gdb) monitor set debug COMPONENT on
  (gdb) monitor set debug COMPONENT off

to enable and disable debug for COMPONENT.  Where COMPONENT is again,
'all', 'threads', 'remote', and 'event-loop'.  Again, I've retained:

  (gdb) monitor set debug on

As a synonym for 'monitor set debug threads on', but I have changed:

  (gdb) monitor set debug off

Which now disables all debugging output from gdbserver.  Notice also
that I'm not using 'on' and 'off' in the monitor commands rather than
'1' and '0'.  The old 1/0 still works, but on/off is what the docs now
suggest; this seemed to better match GDB's internal settings.

I have removed the monitor command:

  (gdb) monitor set remote-debug 0|1
  (gdb) monitor set event-loop-debug 0|1

These commands are replaces by using the 'remote' or 'event-loop'
components as needed.

---

Andrew Burgess (3):
  gdb: fix GDB_DEBUG and GDBSERVER_DEBUG Makefile variables
  gdbserver: allow the --debug command line option to take a value
  gdbserver: allow for general 'monitor set debug COMPONENT VALUE' use

 gdb/Makefile.in                         |   5 +-
 gdb/NEWS                                |  18 ++
 gdb/doc/gdb.texinfo                     |  98 ++++++--
 gdb/testsuite/gdb.server/server-mon.exp |  25 +-
 gdbserver/server.cc                     | 315 +++++++++++++++++++++---
 5 files changed, 397 insertions(+), 64 deletions(-)


base-commit: a393b155174d20d3d120b5012b87c5438ab9e3d4
-- 
2.25.4

[PATCHv2 1/3] gdb: fix GDB_DEBUG and GDBSERVER_DEBUG Makefile variables

[Andrew Burgess]

The gdb/testsuite/README file documents GDB_DEBUG and GDBSERVER_DEBUG
flags, which can be passed to make in order to enable debugging within
GDB or gdbserver respectively.

However, when I do:

  make check-gdb GDB_DEBUG=infrun

I don't see the corresponding debug feature within GDB being enabled.
Nor does:

  make check-gdb GDBSERVER_DEBUG=debug  \
       RUNTESTFLAGS="--target_board=native-extended-gdbserver"

Appear to enable gdbserver debugging.

I tracked this down to the GDB_DEBUG and GDBSERVER_DEBUG flags being
missing from the TARGET_FLAGS_TO_PASS variable in gdb/Makefile.  This
variable already contains lots of testing related flags, like
RUNTESTFLAGS and TESTS, so I think it makes sense to add GDB_DEBUG and
GDBSERVER_DEBUG here too.

With this done, this debug feature is now working as expected.
---
 gdb/Makefile.in | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 3510577f090..0886c0e8495 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -993,7 +993,10 @@ TARGET_FLAGS_TO_PASS = \
 	"RUNTEST=$(RUNTEST)" \
 	"RUNTESTFLAGS=$(RUNTESTFLAGS)" \
 	"FORCE_PARALLEL=$(FORCE_PARALLEL)" \
-	"TESTS=$(TESTS)"
+	"TESTS=$(TESTS)" \
+	"GDB_DEBUG=$(GDB_DEBUG)" \
+	"GDBSERVER_DEBUG=$(GDBSERVER_DEBUG)" \
+
 
 # All source files that go into linking GDB.
 
-- 
2.25.4

[PATCHv2 2/3] gdbserver: allow the --debug command line option to take a value

[Andrew Burgess]

Currently, gdbserver has the following command line options related to
debugging output:

  --debug
  --remote-debug
  --event-loop-debug

This doesn't scale well.  If I want an extra debug component I need to
add another command line flag.

This commit changes --debug to take a list of components.

The currently supported components are: all, threads, remote, and
event-loop.  The 'threads' component represents the debug we currently
get from the --debug option.  And if --debug is used without a
component list then the threads component is assumed as the default.

Currently the threads component actually includes a lot of output that
is not really threads related.  In the future I'd like to split this
up into some new, separate components.  But that is not part of this
commit, or even this series.

The special component 'all' does what you'd expect: enables debug
output from all supported components.

The component list is parsed left to write, and you can prefix a
component with '-' to disable that component, so I can write:

  target> gdbserver --debug=all,-event-loop

to get debug for all components except the event-loop component.

I've removed the existing --remote-debug and --event-loop-debug
command line options, these are equivalent to --debug=remote and
--debug=event-loop respectively, or --debug=remote,event-loop to
enable both components.

In this commit I've only update the command line options, in the next
commit I'll update the monitor commands to support a similar
interface.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
---
 gdb/NEWS            |  10 +++
 gdb/doc/gdb.texinfo |  70 ++++++++++++++++----
 gdbserver/server.cc | 158 +++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 216 insertions(+), 22 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 1073e38dfc6..94d1fd8ea8d 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -38,6 +38,16 @@ set remote thread-options-packet
 show remote thread-options-packet
   Set/show the use of the thread options packet.
 
+* New features in the GDB remote stub, GDBserver
+
+  ** The --remote-debug and --event-loop-debug command line options
+     have been removed.
+
+  ** The --debug command line option now takes an optional comma
+     separated list of components to emit debug for.  The currently
+     supported components are: all, threads, event-loop, and remote.
+     If no components are given then threads is assumed.
+
 * Python API
 
   ** New function gdb.notify_mi(NAME, DATA), that emits custom
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index e4c00143fd1..9e46eba4ac9 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -23674,11 +23674,42 @@
 @pxref{--multi Option in Types of Remote Connnections}.
 
 @cindex @option{--debug}, @code{gdbserver} option
-The @option{--debug} option tells @code{gdbserver} to display extra
-status information about the debugging process.
-@cindex @option{--remote-debug}, @code{gdbserver} option
-The @option{--remote-debug} option tells @code{gdbserver} to display
-remote protocol debug output.
+The @option{--debug[=option1,option2,@dots{}]} option tells
+@code{gdbserver} to display extra diagnostic information about the
+debugging process.  The options (@var{option1}, @var{option2}, etc)
+control for which areas of @code{gdbserver} additional information
+will be displayed, possible values are:
+
+@table @code
+@item all
+This enables all available diagnostic output.
+@item threads
+This enables diagnostic output related to threading.  Currently other
+general diagnostic output is included in this category, but this could
+change in future releases of @code{gdbserver}.
+@item event-loop
+This enables event-loop specific diagnostic output.
+@item remote
+This enables diagnostic output related to the transfer of remote
+protocol packets too and from the debugger.
+@end table
+
+@noindent
+If no options are passed to @option{--debug} then this is treated as
+equivalent to @option{--debug=threads}.  This could change in future
+releases of @code{gdbserver}.  The options passed to @option{--debug}
+are processed left to right, and individual options can be prefixed
+with the @kbd{-} (minus) character to disable diagnostic output from
+this area, so it is possible to use:
+
+@smallexample
+  target> gdbserver --debug=all,-event-loop
+@end smallexample
+
+@noindent
+In order to enable all diagnostic output except that for the
+event-loop.
+
 @cindex @option{--debug-file}, @code{gdbserver} option
 @cindex @code{gdbserver}, send all debug output to a single file
 The @option{--debug-file=@var{filename}} option tells @code{gdbserver} to
@@ -50667,16 +50698,27 @@
 target> gdbserver --multi @var{comm}
 @end smallexample
 
-@item --debug
-Instruct @code{gdbserver} to display extra status information about the debugging
-process.
-This option is intended for @code{gdbserver} development and for bug reports to
-the developers.
+@item --debug@r{[}=option1,option2,@dots{}@r{]}
+Instruct @code{gdbserver} to display extra status information about
+the debugging process.  This option is intended for @code{gdbserver}
+development and for bug reports to the developers.
 
-@item --remote-debug
-Instruct @code{gdbserver} to display remote protocol debug output.
-This option is intended for @code{gdbserver} development and for bug reports to
-the developers.
+Each @var{option} is the name of a component for which debugging
+should be enabled.  The list of possible options is @option{all},
+@option{threads}, @option{event-loop}, @option{remote}.  The special
+option @option{all} enables all components.  The option list is
+processed left to right, and an option can be prefixed with the
+@kbd{-} character to disable output for that component, so you could write:
+
+@smallexample
+target> gdbserver --debug=all,-event-loop
+@end smallexample
+
+@noindent
+to turn on debug output for all components except @option{event-loop}.
+If no options are passed to @option{--debug} then this is treated as
+equivalent to @option{--debug=threads}.  This could change in future
+releases of @code{gdbserver}.
 
 @item --debug-file=@var{filename}
 Instruct @code{gdbserver} to send any debug output to the given @var{filename}.
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index f91f70d2aa9..3cb71a861d5 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1457,6 +1457,124 @@ parse_debug_format_options (const char *arg, int is_monitor)
   return std::string ();
 }
 
+/* A wrapper to enable, or disable a debug flag.  These are debug flags
+   that control the debug output from gdbserver, that developers might
+   want, this is not something most end users will need.  */
+
+struct debug_opt
+{
+  /* NAME is the name of this debug option, this should be a simple string
+     containing no whitespace, starting with a letter from isalpha(), and
+     contain only isalnum() characters and '_' underscore and '-' hyphen.
+
+     SETTER is a callback function used to set the debug variable.  This
+     callback will be passed true to enable the debug setting, or false to
+     disable the debug setting.  */
+  debug_opt (const char *name, std::function<void (bool)> setter)
+    : m_name (name),
+      m_setter (setter)
+  {
+    gdb_assert (isalpha (*name));
+  }
+
+  /* Called to enable or disable the debug setting.  */
+  void set (bool enable) const
+  {
+    m_setter (enable);
+  }
+
+  /* Return the name of this debug option.  */
+  const char *name () const
+  { return m_name; }
+
+private:
+  /* The name of this debug option.  */
+  const char *m_name;
+
+  /* The callback to update the debug setting.  */
+  std::function<void (bool)> m_setter;
+};
+
+/* The set of all debug options that gdbserver supports.  These are the
+   options that can be passed to the command line '--debug=...' flag, or to
+   the monitor command 'monitor set debug ...'.  */
+
+static std::vector<debug_opt> all_debug_opt {
+  {"threads", [] (bool enable)
+  {
+    debug_threads = enable;
+  }},
+  {"remote", [] (bool enable)
+  {
+    remote_debug = enable;
+  }},
+  {"event-loop", [] (bool enable)
+  {
+    debug_event_loop = (enable ? debug_event_loop_kind::ALL
+			: debug_event_loop_kind::OFF);
+  }}
+};
+
+/* Parse the options to --debug=...
+
+   OPTIONS is the string of debug components which should be enabled (or
+   disabled), and must not be nullptr.  An empty OPTIONS string is valid,
+   in which case a default set of debug components will be enabled.
+
+   An unknown, or otherwise invalid debug component will result in an
+   exception being thrown.
+
+   OPTIONS can consist of multiple debug component names separated by a
+   comma.  Debugging for each component will be turned on.  The special
+   component 'all' can be used to enable debugging for all components.
+
+   A component can also be prefixed with '-' to disable debugging of that
+   component, so a user might use: '--debug=all,-remote', to enable all
+   debugging, except for the remote (protocol) component.  Components are
+   processed left to write in the OPTIONS list.  */
+
+static void
+parse_debug_options (const char *options)
+{
+  gdb_assert (options != nullptr);
+
+  /* Empty options means the "default" set.  This exists mostly for
+     backwards compatibility with gdbserver's legacy behaviour.  */
+  if (*options == '\0')
+    options = "+threads";
+
+  while (*options != '\0')
+    {
+      const char *end = strchrnul (options, ',');
+
+      bool enable = *options != '-';
+      if (*options == '-' || *options == '+')
+	++options;
+
+      std::string opt (options, end - options);
+
+      if (opt.size () == 0)
+	error ("invalid empty debug option");
+
+      bool is_opt_all = opt == "all";
+
+      bool found = false;
+      for (const auto &debug_opt : all_debug_opt)
+	if (is_opt_all || opt == debug_opt.name ())
+	  {
+	    debug_opt.set (enable);
+	    found = true;
+	    if (!is_opt_all)
+	      break;
+	  }
+
+      if (!found)
+	error ("unknown debug option '%s'", opt.c_str ());
+
+      options = (*end == ',') ? end + 1 : end;
+    }
+}
+
 /* Handle monitor commands not handled by target-specific handlers.  */
 
 static void
@@ -3583,15 +3701,19 @@ gdbserver_usage (FILE *stream)
 	   "\n"
 	   "Debug options:\n"
 	   "\n"
-	   "  --debug               Enable general debugging output.\n"
+	   "  --debug[=OPT1,OPT2,...]\n"
+	   "                        Enable debugging output.\n"
+	   "                          Options:\n"
+	   "                            all, threads, event-loop, remote\n"
+	   "                          With no options, 'threads' is assumed.\n"
+	   "                          Prefix an option with '-' to disable\n"
+	   "                          debugging of that component.\n"
 	   "  --debug-format=OPT1[,OPT2,...]\n"
 	   "                        Specify extra content in debugging output.\n"
 	   "                          Options:\n"
 	   "                            all\n"
 	   "                            none\n"
 	   "                            timestamp\n"
-	   "  --remote-debug        Enable remote protocol debugging output.\n"
-	   "  --event-loop-debug    Enable event loop debugging output.\n"
 	   "  --disable-packet=OPT1[,OPT2,...]\n"
 	   "                        Disable support for RSP packets or features.\n"
 	   "                          Options:\n"
@@ -3870,8 +3992,32 @@ captured_main (int argc, char *argv[])
 	  /* 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)
-	debug_threads = true;
+	{
+	  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
@@ -3884,10 +4030,6 @@ captured_main (int argc, char *argv[])
 	      exit (1);
 	    }
 	}
-      else if (strcmp (*next_arg, "--remote-debug") == 0)
-	remote_debug = true;
-      else if (strcmp (*next_arg, "--event-loop-debug") == 0)
-	debug_event_loop = debug_event_loop_kind::ALL;
       else if (startswith (*next_arg, "--debug-file="))
 	debug_set_output ((*next_arg) + sizeof ("--debug-file=") -1);
       else if (strcmp (*next_arg, "--disable-packet") == 0)
-- 
2.25.4

[PATCHv2 3/3] gdbserver: allow for general 'monitor set debug COMPONENT VALUE' use

[Andrew Burgess]

Building on the last commit, which added a general --debug=COMPONENT
option to the gdbserver command line, this commit updates the monitor
command to allow for general:

  (gdb) monitor set debug COMPONENT off|on

style commands.  Just like with the previous commit, the COMPONENT can
be any one of all, threads, remote, event-loop, and correspond to the
same set of global debug flags.

While on the command line it is possible to do:

  --debug=remote,event-loop,threads

the components have to be entered one at a time with the monitor
command.  I guess there's no reason why we couldn't allow component
grouping within the monitor command, but (to me) what I have here
seemed more in the spirit of GDB's existing 'set debug ...' commands.
If people want it then we can always add component grouping later.

Notice in the above that I use 'off' and 'on' instead of '0' and '1',
which is what the 'monitor set debug' command used to use.  The 0/1
can still be used, but I now advertise off/on in all the docs and help
text, again, this feels more inline with GDB's existing boolean
settings.

I have removed the two existing monitor commands:

  monitor set remote-debug 0|1
  monitor set event-loop-debug 0|1

These are replaced by:

  monitor set debug remote off|on
  monitor set debug event-loop off|on

respectively.
---
 gdb/NEWS                                |   8 ++
 gdb/doc/gdb.texinfo                     |  30 +++--
 gdb/testsuite/gdb.server/server-mon.exp |  25 +++-
 gdbserver/server.cc                     | 159 +++++++++++++++++++-----
 4 files changed, 179 insertions(+), 43 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 94d1fd8ea8d..c8166f08218 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -48,6 +48,14 @@ show remote thread-options-packet
      supported components are: all, threads, event-loop, and remote.
      If no components are given then threads is assumed.
 
+  ** The 'monitor set remote-debug' and 'monitor set event-loop-debug'
+     command have been removed.
+
+  ** The 'monitor set debug 0|1' command has been extended to take a
+     component name, e.g.: 'monitor set debug COMPONENT off|on'.
+     Possible component names are: all, threads, event-loop, and
+     remote.
+
 * Python API
 
   ** New function gdb.notify_mi(NAME, DATA), that emits custom
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9e46eba4ac9..20621a0f7ee 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -23805,15 +23805,31 @@
 @item monitor help
 List the available monitor commands.
 
-@item monitor set debug 0
-@itemx monitor set debug 1
-Disable or enable general debugging messages.
-
-@item monitor set remote-debug 0
-@itemx monitor set remote-debug 1
-Disable or enable specific debugging messages associated with the remote
+@item monitor set debug off
+Disable all internal logging from gdbserver.
+
+@item monitor set debug on
+Enable some general logging from within gdbserver.  Currently this is
+equivalent to @kbd{monitor set debug threads on}, but this might
+change in future releases of gdbserver.
+
+@item monitor set debug threads off
+@itemx monitor set debug threads on
+Disable or enable specific logging messages associated thread handling
+in gdbserver.  Currently this category also includes additional output
+not specifically related to thread handling, this could change in
+future releases of gdbserver.
+
+@item monitor set debug remote off
+@itemx monitor set debug remote on
+Disable or enable specific logging messages associated with the remote
 protocol (@pxref{Remote Protocol}).
 
+@item monitor set debug event-loop off
+@itemx monitor set debug event-loop on
+Disable or enable specific logging messages associated with
+gdbserver's event-loop.
+
 @item monitor set debug-file filename
 @itemx monitor set debug-file
 Send any debug output to the given file, or to stderr.
diff --git a/gdb/testsuite/gdb.server/server-mon.exp b/gdb/testsuite/gdb.server/server-mon.exp
index 728cc84a9e0..65f5961ca8a 100644
--- a/gdb/testsuite/gdb.server/server-mon.exp
+++ b/gdb/testsuite/gdb.server/server-mon.exp
@@ -44,10 +44,27 @@ gdb_test_multiple "monitor help" "monitor help" {
 
 gdb_test "monitor" "Unknown monitor command.*Protocol error.*"
 
-gdb_test "monitor set debug 1" "Debug output enabled\\."
-gdb_test "monitor set debug 0" "Debug output disabled\\."
-gdb_test "monitor set remote-debug 1" "Protocol debug output enabled\\."
-gdb_test "monitor set remote-debug 0" "Protocol debug output disabled\\."
+gdb_test "monitor set debug 1" "General debug output enabled\\."
+gdb_test "monitor set debug 0" "All debug output disabled\\."
+gdb_test "monitor set debug on" "General debug output enabled\\."
+gdb_test "monitor set debug off" "All debug output disabled\\."
+gdb_test "monitor set debug remote 1" "Debug output for 'remote' enabled\\."
+gdb_test "monitor set debug remote 0" "Debug output for 'remote' disabled\\."
+gdb_test "monitor set debug remote on" "Debug output for 'remote' enabled\\."
+gdb_test "monitor set debug remote off" "Debug output for 'remote' disabled\\."
+gdb_test "monitor set debug event-loop 1" "Debug output for 'event-loop' enabled\\."
+gdb_test "monitor set debug event-loop 0" "Debug output for 'event-loop' disabled\\."
+gdb_test "monitor set debug event-loop on" "Debug output for 'event-loop' enabled\\."
+gdb_test "monitor set debug event-loop off" "Debug output for 'event-loop' disabled\\."
+gdb_test "monitor set debug threads 1" "Debug output for 'threads' enabled\\."
+gdb_test "monitor set debug threads 0" "Debug output for 'threads' disabled\\."
+gdb_test "monitor set debug threads on" "Debug output for 'threads' enabled\\."
+gdb_test "monitor set debug threads off" "Debug output for 'threads' disabled\\."
+gdb_test "monitor set debug all 1" "Debug output for 'all' enabled\\."
+gdb_test "monitor set debug all 0" "Debug output for 'all' disabled\\."
+gdb_test "monitor set debug all on" "Debug output for 'all' enabled\\."
+gdb_test "monitor set debug all off" "Debug output for 'all' disabled\\."
+
 gdb_test "monitor set debug-format all" \
     "All extra debug format options enabled\\."
 gdb_test "monitor set debug-format none" \
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 3cb71a861d5..c4dcb83a79b 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1143,14 +1143,15 @@ static void
 monitor_show_help (void)
 {
   monitor_output ("The following monitor commands are supported:\n");
-  monitor_output ("  set debug <0|1>\n");
+  monitor_output ("  set debug on\n");
   monitor_output ("    Enable general debugging messages\n");
+  monitor_output ("  set debug off\n");
+  monitor_output ("    Disable all debugging messages\n");
+  monitor_output ("  set debug COMPONENT <off|on>\n");
+  monitor_output ("    Enable debugging messages for COMPONENT, which is\n");
+  monitor_output ("    one of: all, threads, remote, event-loop.\n");
   monitor_output ("  set debug-hw-points <0|1>\n");
   monitor_output ("    Enable h/w breakpoint/watchpoint debugging messages\n");
-  monitor_output ("  set remote-debug <0|1>\n");
-  monitor_output ("    Enable remote protocol debugging messages\n");
-  monitor_output ("  set event-loop-debug <0|1>\n");
-  monitor_output ("    Enable event loop debugging messages\n");
   monitor_output ("  set debug-format option1[,option2,...]\n");
   monitor_output ("    Add additional information to debugging messages\n");
   monitor_output ("    Options: all, none, timestamp\n");
@@ -1575,50 +1576,144 @@ parse_debug_options (const char *options)
     }
 }
 
-/* Handle monitor commands not handled by target-specific handlers.  */
+/* Called from the 'monitor' command handler, to handle general 'set debug'
+   monitor commands with one of the formats:
 
-static void
-handle_monitor_command (char *mon, char *own_buf)
+     set debug COMPONENT VALUE
+     set debug VALUE
+
+   In both of these command formats VALUE can be 'on', 'off', '1', or '0'
+   with 1/0 being equivalent to on/off respectively.
+
+   In the no-COMPONENT version of the command, if VALUE is 'on' (or '1')
+   then the component 'threads' is assumed, this is for backward
+   compatibility, but maybe in the future we might find a better "default"
+   set of debug flags to enable.
+
+   In the no-COMPONENT version of the command, if VALUE is 'off' (or '0')
+   then all debugging is turned off.
+
+   Otherwise, COMPONENT must be one of the known debug components, and that
+   component is either enabled or disabled as appropriate.
+
+   The string MON contains either 'COMPONENT VALUE' or just the 'VALUE' for
+   the second command format, the 'set debug ' has been stripped off
+   already.
+
+   Return a string containing an error message if something goes wrong,
+   this error can be returned as part of the monitor command output.  If
+   everything goes correctly then the debug global will have been updated,
+   and an empty string is returned.  */
+
+static std::string
+handle_general_monitor_debug (const char *mon)
 {
-  if (strcmp (mon, "set debug 1") == 0)
+  mon = skip_spaces (mon);
+
+  if (*mon == '\0')
+    return "No debug component name found.\n";
+
+  /* Find the first word within MON.  This is either the component name,
+     or the value if no component has been given.  */
+  const char *end = skip_to_space (mon);
+  std::string component (mon, end - mon);
+  if (component.find (',') != component.npos || component[0] == '-'
+      || component[0] == '+')
+    return "Invalid character found in debug component name.\n";
+
+  /* In ACTION_STR we create a string that will be passed to the
+     parse_debug_options string.  This will be either '+COMPONENT' or
+     '-COMPONENT' depending on whether we want to enable or disable
+     COMPONENT.  */
+  std::string action_str;
+
+  /* If parse_debug_options succeeds, then MSG will be returned to the user
+     as the output of the monitor command.  */
+  std::string msg;
+
+  /* Check for 'set debug off', this disables all debug output.  */
+  if (component == "0" || component == "off")
     {
-      debug_threads = true;
-      monitor_output ("Debug output enabled.\n");
+      action_str = "-all";
+      msg = "All debug output disabled.\n";
     }
-  else if (strcmp (mon, "set debug 0") == 0)
+  /* Check for 'set debug on', this disables a general set of debug.  */
+  else if (component == "1" || component == "on")
     {
-      debug_threads = false;
-      monitor_output ("Debug output disabled.\n");
+      action_str = "+threads";
+      msg = "General debug output enabled.\n";
     }
-  else if (strcmp (mon, "set debug-hw-points 1") == 0)
+  /* Otherwise we should have 'set debug COMPONENT VALUE'.  Extract the two
+     parts and validate.  */
+  else
     {
-      show_debug_regs = 1;
-      monitor_output ("H/W point debugging output enabled.\n");
+      /* Figure out the value the user passed.  */
+      const char *value_start = skip_spaces (end);
+      if (*value_start == '\0')
+	return "Missing value for 'set debug' command.\n";
+
+      const char *after_value = skip_to_space (value_start);
+      if (*skip_spaces (after_value) != '\0')
+	return "junk found at end of 'set debug' command value";
+
+      std::string value (value_start, after_value - value_start);
+
+      /* Check VALUE to see if we are enabling, or disabling.  */
+      bool enable;
+      if (value == "0" || value == "off")
+	enable = false;
+      else if (value == "1" || value == "on")
+	enable = true;
+      else
+	string_printf ("Invalid value '%s' for set debug.\n", value.c_str ());
+
+      action_str = std::string (enable ? "+" : "-") + component;
+      msg = string_printf ("Debug output for '%s' %s.\n", component.c_str (),
+			   enable ? "enabled" : "disabled");
     }
-  else if (strcmp (mon, "set debug-hw-points 0") == 0)
+
+  gdb_assert (!msg.empty ());
+  gdb_assert (!action_str.empty ());
+
+  try
     {
-      show_debug_regs = 0;
-      monitor_output ("H/W point debugging output disabled.\n");
+      parse_debug_options (action_str.c_str ());
+      monitor_output (msg.c_str ());
     }
-  else if (strcmp (mon, "set remote-debug 1") == 0)
+  catch (const gdb_exception_error &exception)
     {
-      remote_debug = true;
-      monitor_output ("Protocol debug output enabled.\n");
+      return string_printf ("Error: %s\n", exception.what ());
     }
-  else if (strcmp (mon, "set remote-debug 0") == 0)
+
+  return {};
+}
+
+/* Handle monitor commands not handled by target-specific handlers.  */
+
+static void
+handle_monitor_command (char *mon, char *own_buf)
+{
+  if (startswith (mon, "set debug "))
     {
-      remote_debug = false;
-      monitor_output ("Protocol debug output disabled.\n");
+      std::string error_msg
+	= handle_general_monitor_debug (mon + sizeof ("set debug ") - 1);
+
+      if (!error_msg.empty ())
+	{
+	  monitor_output (error_msg.c_str ());
+	  monitor_show_help ();
+	  write_enn (own_buf);
+	}
     }
-  else if (strcmp (mon, "set event-loop-debug 1") == 0)
+  else if (strcmp (mon, "set debug-hw-points 1") == 0)
     {
-      debug_event_loop = debug_event_loop_kind::ALL;
-      monitor_output ("Event loop debug output enabled.\n");
+      show_debug_regs = 1;
+      monitor_output ("H/W point debugging output enabled.\n");
     }
-  else if (strcmp (mon, "set event-loop-debug 0") == 0)
+  else if (strcmp (mon, "set debug-hw-points 0") == 0)
     {
-      debug_event_loop = debug_event_loop_kind::OFF;
-      monitor_output ("Event loop debug output disabled.\n");
+      show_debug_regs = 0;
+      monitor_output ("H/W point debugging output disabled.\n");
     }
   else if (startswith (mon, "set debug-format "))
     {
-- 
2.25.4

Re: [PATCH 2/3] gdbserver: allow the --debug command line option to take a value

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Thu, 30 Nov 2023 18:31:24 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> +Each @var{option} is the name of a component for which debugging
> >> +should be enabled.  The list of possible options is @option{all},
> >> +@option{threads}, @option{event-loop}, @option{remote}.  The special
> >> +option @option{all} enables all components.  The option list is
> >> +processed left to right, and an option can be prefixed with the
> >> +@kbd{-} character to disable output for that component, so you could write:
> >> +
> >> +@smallexample
> >> +target> gdbserver --debug=all,-event-loop
> >> +@end smallexample
> >> +
> >> +@noindent to turn on debug output for all components except @option{event-loop}.
> >
> > Why do we need this duplicate description?  I'd prefer to have a short
> > sentence with a cross-reference to the previous description, so that
> > we could have only one of them, not two.
> 
> This is unfortunate.  One of these instances is the general gdb info
> manual, while the second of these is the gdbserver man page.

The above is from the manual, and in Texinfo, not from the man page.
How is the man page relevant?

> I guess we could restructure things somehow so that the man page part is
> used in some way such that the duplication is removed?  But that seems
> like a bigger refactoring task; there is already lots of duplication
> between the man page and the info manual, so I think trying to solve the
> problem just for this one option doesn't make much sense.

I meant duplication in the manual, not between the manual and the man
page.

Re: [PATCHv2 3/3] gdbserver: allow for general 'monitor set debug COMPONENT VALUE' use

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Thu, 30 Nov 2023 18:44:18 +0000
> 
>  gdb/NEWS                                |   8 ++
>  gdb/doc/gdb.texinfo                     |  30 +++--
>  gdb/testsuite/gdb.server/server-mon.exp |  25 +++-
>  gdbserver/server.cc                     | 159 +++++++++++++++++++-----
>  4 files changed, 179 insertions(+), 43 deletions(-)

Thanks.

> diff --git a/gdb/NEWS b/gdb/NEWS
> index 94d1fd8ea8d..c8166f08218 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -48,6 +48,14 @@ show remote thread-options-packet
>       supported components are: all, threads, event-loop, and remote.
>       If no components are given then threads is assumed.
>  
> +  ** The 'monitor set remote-debug' and 'monitor set event-loop-debug'
> +     command have been removed.
> +
> +  ** The 'monitor set debug 0|1' command has been extended to take a
> +     component name, e.g.: 'monitor set debug COMPONENT off|on'.
> +     Possible component names are: all, threads, event-loop, and
> +     remote.
> +
>  * Python API

This part is OK.

> +@item monitor set debug threads off
> +@itemx monitor set debug threads on
> +Disable or enable specific logging messages associated thread handling
                                                         ^
"with" is missing there.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCHv2 0/3] Improve debug output support in gdbserver

[Tom Tromey]

>>>>> Andrew Burgess <aburgess@redhat.com> writes:

> Changes since v1:
>   - I've now dropped the --remote-debug and --event-loop-debug command
>     line options,
[...]

Thank you.  This looks good to me.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 2/3] gdbserver: allow the --debug command line option to take a value

[Andrew Burgess]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: gdb-patches@sourceware.org
>> Date: Thu, 30 Nov 2023 18:31:24 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> +Each @var{option} is the name of a component for which debugging
>> >> +should be enabled.  The list of possible options is @option{all},
>> >> +@option{threads}, @option{event-loop}, @option{remote}.  The special
>> >> +option @option{all} enables all components.  The option list is
>> >> +processed left to right, and an option can be prefixed with the
>> >> +@kbd{-} character to disable output for that component, so you could write:
>> >> +
>> >> +@smallexample
>> >> +target> gdbserver --debug=all,-event-loop
>> >> +@end smallexample
>> >> +
>> >> +@noindent to turn on debug output for all components except @option{event-loop}.
>> >
>> > Why do we need this duplicate description?  I'd prefer to have a short
>> > sentence with a cross-reference to the previous description, so that
>> > we could have only one of them, not two.
>> 
>> This is unfortunate.  One of these instances is the general gdb info
>> manual, while the second of these is the gdbserver man page.
>
> The above is from the manual, and in Texinfo, not from the man page.
> How is the man page relevant?

The man page is generated from gdb.texinfo using this magic in
gdb/doc/Makefile.in:

  gdbserver.1: $(GDB_DOC_FILES)
  	touch $@
  	-$(TEXI2POD) $(MANCONF) -Dgdbserver < $(srcdir)/gdb.texinfo > gdbserver.pod
  	-($(POD2MAN1) gdbserver.pod | sed -e '/^.if n .na/d' > $@.T$$$$ && \
  		mv -f $@.T$$$$ $@) || (rm -f $@.T$$$$ && exit 1)
  	rm -f gdbserver.pod

If from a top-level binutils-gdb build directory you do:

  make -C gdb/doc man

you'll then be able to do:

 man gdb/doc/gdbserver.1

and in that man-page you'll find that one of the blocks of my change are
present.  Note that my patch doesn't touch any other files in gdb/doc,
so we can be sure this is all coming from gdb.texinfo.

Similarly, if you:

 make -C gdb/doc info

and then:

 info gdb/doc/gdb.info

and then search for "Man Pages", you'll see that the man-pages for each
tool are included in the info manual.

Thanks,
Andrew

>
>> I guess we could restructure things somehow so that the man page part is
>> used in some way such that the duplication is removed?  But that seems
>> like a bigger refactoring task; there is already lots of duplication
>> between the man page and the info manual, so I think trying to solve the
>> problem just for this one option doesn't make much sense.
>
> I meant duplication in the manual, not between the manual and the man
> page.

Re: [PATCH 2/3] gdbserver: allow the --debug command line option to take a value

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Mon, 04 Dec 2023 15:57:14 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > Why do we need this duplicate description?  I'd prefer to have a short
> >> > sentence with a cross-reference to the previous description, so that
> >> > we could have only one of them, not two.
> >> 
> >> This is unfortunate.  One of these instances is the general gdb info
> >> manual, while the second of these is the gdbserver man page.
> >
> > The above is from the manual, and in Texinfo, not from the man page.
> > How is the man page relevant?
> 
> The man page is generated from gdb.texinfo using this magic in
> gdb/doc/Makefile.in:

And we must have two identical passages in the manual to do that?  We
cannot generate these man pages using a single copy of the text we are
talking about?

Re: [PATCH 2/3] gdbserver: allow the --debug command line option to take a value

[Andrew Burgess]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: gdb-patches@sourceware.org
>> Date: Mon, 04 Dec 2023 15:57:14 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> > Why do we need this duplicate description?  I'd prefer to have a short
>> >> > sentence with a cross-reference to the previous description, so that
>> >> > we could have only one of them, not two.
>> >> 
>> >> This is unfortunate.  One of these instances is the general gdb info
>> >> manual, while the second of these is the gdbserver man page.
>> >
>> > The above is from the manual, and in Texinfo, not from the man page.
>> > How is the man page relevant?
>> 
>> The man page is generated from gdb.texinfo using this magic in
>> gdb/doc/Makefile.in:
>
> And we must have two identical passages in the manual to do that?  We
> cannot generate these man pages using a single copy of the text we are
> talking about?

I 100% agree with you that it would be better if we could somehow
structure things so that we could remove the duplication.  But I don't
know what the right way to do that would be, and I don't think it's
going to be a small task.

Hopefully you're not suggesting that I need to do this in order to
document a new gdbserver command line option?

I guess my question would be: Are the doc parts of this patch good
enough given the known idiosyncrasies of how the manual is structured?

Thanks,
Andrew

Re: [PATCH 2/3] gdbserver: allow the --debug command line option to take a value

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Tue, 05 Dec 2023 10:17:33 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> The man page is generated from gdb.texinfo using this magic in
> >> gdb/doc/Makefile.in:
> >
> > And we must have two identical passages in the manual to do that?  We
> > cannot generate these man pages using a single copy of the text we are
> > talking about?
> 
> I 100% agree with you that it would be better if we could somehow
> structure things so that we could remove the duplication.  But I don't
> know what the right way to do that would be, and I don't think it's
> going to be a small task.
> 
> Hopefully you're not suggesting that I need to do this in order to
> document a new gdbserver command line option?

No, if this is not trivial to fix, we might as well keep living with
it.

> I guess my question would be: Are the doc parts of this patch good
> enough given the known idiosyncrasies of how the manual is structured?

Yes, thanks.

Re: [PATCHv2 0/3] Improve debug output support in gdbserver

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> Andrew Burgess <aburgess@redhat.com> writes:
>
>> Changes since v1:
>>   - I've now dropped the --remote-debug and --event-loop-debug command
>>     line options,
> [...]
>
> Thank you.  This looks good to me.
> Approved-By: Tom Tromey <tom@tromey.com>

Pushed.

Thanks,
Andrew