Re: [RFC-v2] Remove i386 low level debug register function from nm- header file.

["Ulrich Weigand" <uweigand@de.ibm.com>]

Pierre Muller wrote:

> This is a new version of the patch.
> 
> I tried to take into account all remarks from Ulrich.

Thanks!  There's one additional thing I noticed: several of
the prototypes you've moved to the new i386-nat.h file
actually need no longer be exported at all; instead, they
can be made static functions in i386-nat.c.

Likewise, in most cases the callbacks used to implement
the i386_dr_low_type members can now be made static
functions to the file that defines them.

> I also tested it on a amd64 linux machine,
> and had to change two functions in amd64-linux-nat.c
> to be able to compile it on that target.

Hmm ... it seems there's a disconnect between using "unsigned"
and "unsigned long" as type for the DR6/7 contents.  This affects
not only amd64-linux-nat.c, but apparently some other files as well:

i386bsd-nat.c:i386bsd_dr_set_control (unsigned long control)
i386-linux-nat.c:i386_linux_dr_set_control (unsigned long control)
amd64-linux-nat.c:amd64_linux_dr_set_control (unsigned long control)

... but ...

win32-nat.c:cygwin_set_dr7 (unsigned val)
go32-nat.c:go32_set_dr7 (unsigned val)

When you build on a 32-bit system, this probably won't result in
an error, even though it's strictly speaking still invalid C ...

I think these need to be fixed so they all agree.  As far as I know,
those values are in fact 32-bit, so I guess "unsigned" (or preferably,
"unsigned int") should be OK to use.

> Index: src/gdb/i386-nat.h
> ===================================================================
> RCS file: i386-nat.h
> diff -N i386-nat.h

> +#include "config.h"
> +#include "defs.h"
> +#include "breakpoint.h"
> +#include "command.h"
> +#include "gdbcmd.h"

IMHO these shouldn't be in a common header file, but included as needed
by the source files ...   The old config/i386/nm-i386.h didn't have such
includes either.


> +/* Insert a watchpoint to watch a memory region which starts at
> +   address ADDR and whose length is LEN bytes.  Watch memory accesses
> +   of the type TYPE.  Return 0 on success, -1 on failure.  */
> +extern int i386_insert_watchpoint (CORE_ADDR addr, int len, int type);
> +
> +/* Remove a watchpoint that watched the memory region which starts at
> +   address ADDR, whose length is LEN bytes, and for accesses of the
> +   type TYPE.  Return 0 on success, -1 on failure.  */
> +extern int i386_remove_watchpoint (CORE_ADDR addr, int len, int type);
> +
> +/* Return non-zero if we can watch a memory region that starts at
> +   address ADDR and whose length is LEN bytes.  */
> +extern int i386_region_ok_for_watchpoint (CORE_ADDR addr, int len);
> +
> +/* Return non-zero if the inferior has some break/watchpoint that
> +   triggered.  */
> +extern int i386_stopped_by_hwbp (void);
> +
> +/* If the inferior has some break/watchpoint that triggered, set
> +   the address associated with that break/watchpoint and return
> +   true.  Otherwise, return false.  */
> +extern int i386_stopped_data_address (struct target_ops *, CORE_ADDR *);
> +
> +/* Insert a hardware-assisted breakpoint at BP_TGT->placed_address.
> +   Return 0 on success, EBUSY on failure.  */
> +struct bp_target_info;
> +extern int i386_insert_hw_breakpoint (struct bp_target_info *bp_tgt);
> +
> +/* Remove a hardware-assisted breakpoint at BP_TGT->placed_address.
> +   Return 0 on success, -1 on failure.  */
> +extern int i386_remove_hw_breakpoint (struct bp_target_info *bp_tgt);
> +
> +extern int i386_stopped_by_watchpoint (void);

All these shouldn't be there, and the functions made static to
i386-nat.c.

> +struct i386_dr_low_type i386_dr_low;

You should not have a variable *definition* in a header file.  Instead,
have an "extern" declaration in the header, and move the definition 
into i386-nat.c.

> +  i386_dr_low.set_control = amd64_linux_dr_set_control;
> +  i386_dr_low.set_addr = amd64_linux_dr_set_addr;
> +  i386_dr_low.reset_addr = amd64_linux_dr_reset_addr;
> +  i386_dr_low.get_status = amd64_linux_dr_get_status;

These amd64_linux_... functions can now be made static.

> +  i386_dr_low.set_control = go32_set_dr7;
> +  i386_dr_low.set_addr = go32_set_dr;
> +  i386_dr_low.reset_addr = NULL;
> +  i386_dr_low.get_status = go32_get_dr6;

Likewise those go32_... functions.

> +  i386_dr_low.set_control = i386_linux_dr_set_control;
> +  i386_dr_low.set_addr = i386_linux_dr_set_addr;
> +  i386_dr_low.reset_addr = i386_linux_dr_reset_addr;
> +  i386_dr_low.get_status = i386_linux_dr_get_status;

And those i386_linux_... functions.

> +static int
> +show_debug_regs_command_added = 0;
> +
> +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,
> +			   &maint_show_dr, _("\
> +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);
> +  show_debug_regs_command_added = 1;
> +}

This is just a minor nit, but I'd prefer to have the guard against
multiple calls be static inside the function, and do the test there.

> +i386_set_debug_register_length (int len)
>  {
> +  /* This function should be called only once for each native target.  */
> +  gdb_assert (i386_dr_low.debug_register_length == 0);
> +  i386_dr_low.debug_register_length = len;
>  }

In the alternative, you might actually move the call to *this* function,
which is already guarded to be called only once ...

> Index: src/gdb/i386fbsd-nat.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/i386fbsd-nat.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 i386fbsd-nat.c
> --- src/gdb/i386fbsd-nat.c	23 Feb 2009 00:03:49 -0000	1.18
> +++ src/gdb/i386fbsd-nat.c	12 May 2009 14:07:01 -0000
> @@ -29,6 +29,7 @@
>  
>  #include "fbsd-nat.h"
>  #include "i386-tdep.h"
> +#include "i386-nat.h"
>  #include "i386bsd-nat.h"
>  
>  /* Resume execution of the inferior process.  If STEP is nonzero,
> @@ -126,7 +127,20 @@ _initialize_i386fbsd_nat (void)
>  
>    /* Add some extra features to the common *BSD/i386 target.  */
>    t = i386bsd_target ();
> +
> +#ifdef HAVE_PT_GETDBREGS
> +
>    i386_use_watchpoints (t);
> +
> +  i386_dr_low.set_control = i386bsd_dr_set_control;
> +  i386_dr_low.set_addr = i386bsd_dr_set_addr;
> +  i386_dr_low.reset_addr = i386bsd_dr_reset_addr;
> +  i386_dr_low.get_status = i386bsd_dr_get_status;

Have you tried building this?  It seems you should be getting
warnings here as there's now no prototype for the functions;
note that they are defined in a *different* file.

I think the prototypes for those should move to i386bsd-nat.h.

> +void cygwin_set_dr (int i, CORE_ADDR addr);
> +void cygwin_set_dr7 (unsigned val);
> +unsigned cygwin_get_dr6 (void);

These should again be static.



As a side note, I think we should be able to get completely rid of
the remaining nm- header files you touched.  (Note, I'm not suggesting
this needs to be done as part of this patch, but it's something that
could be done by follow-up patches ...)

> Index: src/gdb/config/i386/nm-cygwin.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/config/i386/nm-cygwin.h,v
> retrieving revision 1.10
> diff -u -p -r1.10 nm-cygwin.h
> --- src/gdb/config/i386/nm-cygwin.h	26 Mar 2009 00:18:46 -0000	1.10
> +++ src/gdb/config/i386/nm-cygwin.h	12 May 2009 15:20:18 -0000
> @@ -18,20 +18,3 @@
>  
>  #define ADD_SHARED_SYMBOL_FILES dll_symbol_command
>  void dll_symbol_command (char *, int);

> Index: src/gdb/config/i386/nm-cygwin64.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/config/i386/nm-cygwin64.h,v
> retrieving revision 1.3
> diff -u -p -r1.3 nm-cygwin64.h
> --- src/gdb/config/i386/nm-cygwin64.h	26 Mar 2009 00:18:46 -0000	1.3
> +++ src/gdb/config/i386/nm-cygwin64.h	12 May 2009 15:20:18 -0000
> @@ -17,20 +17,3 @@
>  
>  #define ADD_SHARED_SYMBOL_FILES dll_symbol_command
>  void dll_symbol_command (char *, int);

This hook simply registers an additional GDB command
"add-shared-symbol-files".  Maybe registering this command
can simply move to windows-nat.c instead?

Or maybe this is the wrong place anyway: shouldn't the availability
of the command depend on whether the *target* is Windows, not the host?


> Index: src/gdb/config/i386/nm-fbsd.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/config/i386/nm-fbsd.h,v
> retrieving revision 1.20
> diff -u -p -r1.20 nm-fbsd.h
> --- src/gdb/config/i386/nm-fbsd.h	26 Mar 2009 00:18:46 -0000	1.20
> +++ src/gdb/config/i386/nm-fbsd.h	12 May 2009 15:20:18 -0000
> @@ -21,33 +21,8 @@
>  #ifdef HAVE_SYS_PARAM_H
>  #include <sys/param.h>
>  #endif

This include was added in 2002 by David O'Brien:
http://sourceware.org/ml/gdb-patches/2002-06/msg00573.html

who in a follow-up message stated:

  Some code I added to GDB 5.2 in the FreeBSD source tree needs it.  That
  code isn't ready to submit back yet.  Since the include is benign but I
  wanted to reduce the diffs where easy.  I actually don't need it in
  nm-fbsd.h any more.  

As per the last sentence, I think this can go away ...


> Index: src/gdb/config/i386/nm-linux.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/config/i386/nm-linux.h,v
> retrieving revision 1.30
> diff -u -p -r1.30 nm-linux.h
>  #ifdef HAVE_PTRACE_GETFPXREGS
>  /* Include register set support for the SSE registers.  */
>  #define FILL_FPXREGSET

This is dead code now; the only use of FILL_FPXREGSET is in a conditional
section in gregset.h, but everything that's defined/declared there is no
longer used anyway ... This should simply go away.


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com