[RFC-v2] Remove i386 low level debug register function from nm- header file.
Ulrich Weigand
uweigand@de.ibm.com
Wed May 13 15:33:00 GMT 2009
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
More information about the Gdb-patches
mailing list