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

Pierre Muller
2009-05-12T14:37:21Z

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