From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28897 invoked by alias); 13 May 2009 15:33:52 -0000 Received: (qmail 28873 invoked by uid 22791); 13 May 2009 15:33:50 -0000 X-SWARE-Spam-Status: No, hits=-0.7 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,MSGID_FROM_MTA_HEADER,SPF_SOFTFAIL X-Spam-Check-By: sourceware.org Received: from mtagate8.de.ibm.com (HELO mtagate8.de.ibm.com) (195.212.29.157) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 13 May 2009 15:33:43 +0000 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate8.de.ibm.com (8.14.3/8.13.8) with ESMTP id n4DFXeZe141582 for ; Wed, 13 May 2009 15:33:40 GMT Received: from d12av02.megacenter.de.ibm.com (d12av02.megacenter.de.ibm.com [9.149.165.228]) by d12nrmr1607.megacenter.de.ibm.com (8.13.8/8.13.8/NCO v9.2) with ESMTP id n4DFXe1L2351132 for ; Wed, 13 May 2009 17:33:40 +0200 Received: from d12av02.megacenter.de.ibm.com (loopback [127.0.0.1]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id n4DFXdHa019704 for ; Wed, 13 May 2009 17:33:40 +0200 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.12.11) with SMTP id n4DFXcvm019686; Wed, 13 May 2009 17:33:38 +0200 Message-Id: <200905131533.n4DFXcvm019686@d12av02.megacenter.de.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Wed, 13 May 2009 17:33:38 +0200 Subject: Re: [RFC-v2] Remove i386 low level debug register function from nm- header file. To: muller@ics.u-strasbg.fr (Pierre Muller) Date: Wed, 13 May 2009 15:33:00 -0000 From: "Ulrich Weigand" Cc: muller@ics.u-strasbg.fr ('Pierre Muller'), gdb-patches@sourceware.org, pedro@codesourcery.com ('Pedro Alves'), eliz@gnu.org ('Eli Zaretskii') In-Reply-To: <000601c9d348$92580d80$b7082880$@u-strasbg.fr> from "Pierre Muller" at May 12, 2009 11:28:18 PM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-05/txt/msg00273.txt.bz2 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 > #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