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

["Pierre Muller" <muller@ics.u-strasbg.fr>]

Thanks for the feedback.

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Ulrich Weigand
> Envoyé : Tuesday, May 12, 2009 3:01 PM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org; 'Pedro Alves'; 'Eli Zaretskii'
> Objet : Re: [RFC] Remove i386 low level debug register function from
> nm- header file.
> 
> Pierre Muller wrote:
> 
> > Following my first RFC
> > http://sourceware.org/ml/gdb-patches/2009-03/msg00197.html
> >
> > and the following answers, I resubmit
> > a new version where all i386 low level debug register
> > is contained in a single struct defined in the
> > new i386-nat.h header (to comply with DOS file name restrictions
> > as asked for by Eli).
> 
> This looks good to me.  However, now that you've moved the
> prototypes to i386-nat.h, there is no longer any need for
> the config/i386/nm-i386.h file -- please get rid of that file
> (by removing the places where it is included into other nm-
> files).

  OK, but how do I commit a file deletion?
I never did this before!

> Also, it seems to me that now there is no need any more for
> the I386_USE_GENERIC_WATCHPOINTS define -- targets that use
> watchpoints call the i386_use_watchpoints routines, those
> that don't, do not.

  Seems reasonable indeed!
 
> I think you should provide everything in i386-nat.c (and
> the new i386-nat.h) unconditionally, and eliminate the
> various definitions of I386_USE_GENERIC_WATCHPOINTS in the
> nm- header files.

  The only drawback is that "maint show-debug--regs" command
will then also appear on target that do not support
debug registers... Anyhow, it will just be a no-op in that case.
Is that a problem?

> Note that in combination with removing nm-i386.h, this
> actually makes several nm- files completely empty, so they
> should be removed as well.
> 
> > 2009-05-10  Pierre Muller  <muller.u-strasbg.fr>
> >
> > 	Remove all i386 debug register low level macros in config tm
> files.
> 
> Should read "nm" files, not tm.

Yes of course, thanks for catching this!

> > +/* Targets should define this to use the generic x86 watchpoint
> support.
> > */
> > +#ifdef I386_USE_GENERIC_WATCHPOINTS
> 
> As mentioned above, please get rid of this conditional ...
> 
> > +
> > +/* Add watchpoint methods to the provided target_ops.
> > +   Targets defining I386_USE_GENERIC_WATCHPOINTS should call
> > +   this.  */
> 
> ... as well as the second sentence of the comment.
> 
> >  /* Support for 8-byte wide hw watchpoints.  */
> >  #ifndef TARGET_HAS_DR_LEN_8
> > -#define TARGET_HAS_DR_LEN_8	0
> > +#define TARGET_HAS_DR_LEN_8 (i386_dr_low.debug_register_length == 8)
> >  #endif
> 
> Nobody should be defining this any more, so please eliminate
> the #ifndef ... conditional.

  OK, will do.
 
> > Index: src/gdb/i386fbsd-nat.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/i386fbsd-nat.c,v
> > retrieving revision 1.18
> > @@ -126,7 +127,19 @@ _initialize_i386fbsd_nat (void)
> >
> >    /* Add some extra features to the common *BSD/i386 target.  */
> >    t = i386bsd_target ();
> > +
> > +#ifdef I386_USE_GENERIC_WATCHPOINTS
> > +
> >    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;
> > +  i386_set_debug_register_length (4);
> > +#endif /* I386_USE_GENERIC_WATCHPOINTS */
> 
> 
> In this case, because the header file currently conditionally defines
> I386_USE_GENERIC_WATCHPOINTS:
> 
> #ifdef HAVE_PT_GETDBREGS
> #define I386_USE_GENERIC_WATCHPOINTS
> #endif
> 
> you should move that condition to the i386fbsd-nat.c file:
> 
> #ifdef HAVE_PT_GETDBREGS
>    i386_use_watchpoints (t);
> 
>    ...
> #endif

  So freebsd with old kernels (not supporting PT_GETDBREGS), 
will typically have the non-operational "maint show-debug-reg" command.
 
> > diff -u -p -r1.189 windows-nat.c
> > --- windows-nat.c	17 Apr 2009 15:44:28 -0000	1.189
> > +++ windows-nat.c	8 May 2009 14:02:25 -0000
> > @@ -2166,8 +2171,22 @@ init_windows_ops (void)
> >    windows_ops.to_has_execution = 1;
> >    windows_ops.to_pid_to_exec_file = windows_pid_to_exec_file;
> >    windows_ops.to_get_ada_task_ptid = windows_get_ada_task_ptid;
> > +
> > +#ifdef I386_USE_GENERIC_WATCHPOINTS
> > +
> >    i386_use_watchpoints (&windows_ops);
> >
> > +  i386_dr_low.set_control = cygwin_set_dr7;
> > +  i386_dr_low.set_addr = cygwin_set_dr;
> > +  i386_dr_low.reset_addr = NULL;
> > +  i386_dr_low.get_status = cygwin_get_dr6;
> 
> Here, it seems every user of windows-nat.c unconditionally
> defines I386_USE_GENERIC_WATCHPOINTS, so there's no point in
> adding the #ifdef's to this file ...

  I don't know here, the problem is that windows-nat is full of i386
specific code that should be moved to i386-windows-nat and/or
amd64-windows-nat
so that supporting other processors will be easier.

Thanks Ulrich,
I will try to submit a new version promptly.

Pierre