Re: [PATCH 4/4 v6] Introduce common-debug.h

[Doug Evans <dje at google dot com>]

Gary Benson writes:
 > This introduces common-debug.h.  This holds the flag debug_hw_points
 > (which can be set to enable debugging of the hardware breakpoint/
 > watchpoint support code) and debug_printf and debug_vprintf, two
 > functions that the common code can use to print debugging messages.
 > Clients of the common code are expected to implement debug_vprintf;
 > a debug_vprintf function is written from scratch for GDB, and
 > gdbserver's existing debug_printf is repurposed as debug_vprintf.
 > 
 > common/agent.c is changed to use debug_vprintf rather than
 > defining the macro DEBUG_AGENT depending on GDBSERVER.
 > 
 > nat/i386-dregs.c is changed to use the externally-implemented
 > debug_printf, rather than defining it itself, and a now-unnecessary
 > declaration of debug_hw_points is removed.
 > 
 > Various other files are changed to remove declarations of
 > debug_hw_points.
 > 
 > gdb/
 > 2014-08-11  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* common/common-debug.h: New file.
 > 	* common/common-debug.c: Likewise.
 > 	* debug.c: Likewise.
 > 	* Makefile.in (SFILES): Add common/common-debug.c.
 > 	(HFILES_NO_SRCDIR): Add common/common-debug.h.
 > 	(COMMON_OBS): Add common-debug.o and debug.o.
 > 	(common-debug.o): New rule.
 > 	* common/common-defs.h: Include common-debug.h.
 > 	* common/agent.c (debug_agent_printf): New function.
 > 	(DEBUG_AGENT): Redefine.
 > 	* nat/i386-dregs.c (debug_printf): Undefine.
 > 	(debug_hw_points): Remove.
 > 	* aarch64-linux-nat.c (debug_hw_points): Likewise.
 > 	* i386-nat.c (debug_hw_points): Likewise.
 > 
 > gdb/gdbserver/
 > 2014-08-11  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* Makefile.in (SFILES): Add common/common-debug.c.
 > 	(OBS): Add common-debug.o.
 > 	(common-debug.o): New rule.
 > 	* debug.h (debug_printf): Don't declare.
 > 	* debug.c (debug_printf): Renamed and rewritten as...
 > 	(debug_vprintf): New function.
 > 	* server.h (debug_hw_points): Remove.
 > 	* server.c (debug_hw_points): Likewise.
 > 	* linux-aarch64-low.c (debug_hw_points): Likewise.

Hi.

I think there's at least still one TODO here.
I don't have a strong preference on how it's solved,
even keeping the current situation (though it feels less preferable),
but I haven't seen it discussed so I want to make sure that the
issue has at least been considered.

i386-nat.c has this:

static void
add_show_debug_regs_command (void)
{
  /* A maintenance command to enable printing the internal DRi mirror
     variables.  */
  add_setshow_boolean_cmd ("show-debug-regs", class_maintenance,
			   &debug_hw_points, _("\
Set whether to show variables that mirror the x86 debug registers."), _("\
Show whether to show variables that mirror the x86 debug registers."), _("\
Use \"on\" to enable, \"off\" to disable.\n\
If enabled, the debug registers values are shown when GDB inserts\n\
or removes a hardware breakpoint or watchpoint, and when the inferior\n\
triggers a breakpoint or watchpoint."),
			   NULL,
			   NULL,
			   &maintenance_set_cmdlist,
			   &maintenance_show_cmdlist);
}

and similarly aarch64-linux-nat.c has this:

static void
add_show_debug_regs_command (void)
{
  /* A maintenance command to enable printing the internal DRi mirror
     variables.  */
  add_setshow_boolean_cmd ("show-debug-regs", class_maintenance,
			   &debug_hw_points, _("\
Set whether to show variables that mirror the AArch64 debug registers."), _("\
Show whether to show variables that mirror the AArch64 debug registers."), _("\
Use \"on\" to enable, \"off\" to disable.\n\
If enabled, the debug registers values are shown when GDB inserts\n\
or removes a hardware breakpoint or watchpoint, and when the inferior\n\
triggers a breakpoint or watchpoint."),
			   NULL,
			   NULL,
			   &maintenance_set_cmdlist,
			   &maintenance_show_cmdlist);
}

If debug_hw_points is now a "common" variable, it seems like
non-architecture-specific code should define the parameter to set/show it.
OTOH, it is nice that the parameter only get defined if/when it's useful.
[I realize being "nat" files these files would never get compiled together
in the same binary.]
OTOOH, it's a debug parameter, I'm less inclined to rigid cleanliness here.
Plus being a "common" parameter means other arches can use it and would be
less inclined to unnecessarily invent a different parameter.

---

Hmmm, I just noticed something: The aarch64 version also uses
add_setshow_boolean_cmd, but there is code like this in aarch64-linux-nat.c:

  if (debug_hw_points > 1)

If we make this a non-arch-specific parameter, we should probably make it
an integer parameter.

---

btw, it's confusing that the variable is named "debug_hw_points"
but the command to set it is "maint set show-debug-regs".  Bleah.
The intuitive naming is to base the variable name off of the parameter name,
but I'm also ok with changing the parameter name.
"set debug hw-points <n>" ?
I don't have a strong opinion, other than if we're making changes
in this area IWBN to clean up the naming while we're at it.
Plus "set debug ..." is more consistent with other such parameters
than "maint set ...".