This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

Re: [RFA] Unified watchpoints for x86 platforms



On 7 Mar 2001, Mark Kettenis wrote:

> Sorry for not responding sooner.  I've been suffering from a bad flu
> :-(.  But I do have some comcerns though.

Thanks for the feedback.

> That addresses one of my concerns :-).  However, why are you
> conditionalizing the code both on I386_USE_GENERIC_WATCHPOINTS and
> TARGET_HAS_HARDWARE_WATCHPOINTS?

TARGET_HAS_HARDWARE_WATCHPOINTS is widely used elsewehere in GDB.  I 
didn't want to replace it with I386_USE_GENERIC_WATCHPOINTS that I 
invented.  On the other hand, I didn't want to make it possible for
some port to define the latter without defining the former, because I 
think the rest of GDB code will become confused, or the port will not 
work as intended.  So I opted for a compile-time detection of such 
problems.

> I'm still not convinced that this stuff belongs in i386-tdep.c.  While
> the code defenitely is an improvement over what's currently in GDB, it
> has some limitations.  The fact that you rely on the I386_DR_LOW_*
> macro's means that adding these functions to i386-tdep.c is moving us
> farther away from multi-arching the i386 stuff.

I admit I don't know enough about multi-arch to be sure I don't say 
something stupid.  However, at least in principle, I386_DR_LOW_* macros 
can be themselves multi-arch'ed, can't they?

> Another limitation is the use of global varaibles to keep reference
> counts and debug register mirrors.  This means that the current
> implementation can only work for a single target with a single thread
> of control.

The non-multi-arch nature of the code was a specific limitation of the 
design I presented.  Someone, perhaps even you, told me that 
multi-arching this stuff would be SEP (Someone Else's Problem).  I don't 
know enough to do that myself, and even if I did, I don't have any 
practical way of testing it.

As for multithreading, the discussion we held back in November concluded 
that only the status register should be thread specific; the other debug 
registers are global.  The code I wrote supports that (or at least I 
think it does; if you see some fragment that doesn't please identify it).

IMHO, since currently only native debugging uses watchpoints, it 
shouldn't matter one way or the other.  In the long run, when the code is 
multi-arched, Someone(tm) will have to figure out how to do that so that
non-native targets would be able to benefit from this code.  If and when 
they do, they will want to move the code back to i386-tdep.c.  So why not 
leave it there in the first place?

As written, the code is definitely good for any x86 target, including 
non-native targets, as long as that target doesn't try to use multiple 
architectures.  I don't think it's right to prevent such targets from 
using the code just because it is not general enough at this time.

> > +#ifdef HAVE_PTRACE_H
> > +#include <ptrace.h>
> > +#else
> > +#ifdef HAVE_SYS_PTRACE_H
> > +#include <sys/ptrace.h>
> > +#endif
> > +#endif
> > +
> > +#ifdef HAVE_SYS_USER
> > +#include <sys/user.h>
> > +#endif
> > +
> > +/* FIXME: The following should be just "#include <sys/debugreg.h>",
> > +   but the the Linux 2.1.x kernel and glibc 2.0.x are not in sync;
> > +   including <sys/debugreg.h> will result in an error.  With luck,
> > +   these losers will get their act together and we can trash this hack
> > +   in the near future.
> > +
> > +   --jsm 1998-10-21; modified by eliz 2001-02-24.  */
> > +
> > +#ifdef HAVE_ASM_DEBUGREG_H
> > +#include <asm/debugreg.h>
> > +#else  /* !HAVE_ASM_DEBUGREG_H */
> > +
> > +#ifdef HAVE_SYS_DEBUGREG_H
> > +#include <sys/debugreg.h>
> > +#else  /* !HAVE_SYS_DEBUGREG_H */
> 
> Please get rid of this crap.  You're completely free to define these
> things yourself.  There is absolutely no need to rely on constants
> defined in those possibly broken headers.  Simply assume there are
> four address registers and the bits are laid out as in the Intel
> documentation.

I will remove debugreg.h if no one objects.  As for ptrace.h, is it wise 
to remove that as well?  I'd imagine that just about every target will 
want to include it anyway.

> > +/* This is in i386v-nat.c, so let's have it here, just in case.  */
> > +#if !defined (offsetof)
> > +#define offsetof(TYPE, MEMBER) ((unsigned long) &((TYPE *)0)->MEMBER)
> > +#endif
> 
> Remove this crap.  You don't use offsetof() in your code.

I left it there because at least one implementation of the I386_DR_LOW_* 
macros will want it.

> > +/* Remove a hardware-assisted breakpoint at address ADDR.  SHADOW is
> > +   unused.  Return 0 on success, -1 on failure.  */
> > +int  i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow);
> > +
> 
> If these are exported they should live in a public header
> (i386-nat.h?), and you shouldn't provide them here.

Why is it bad to have them here?  If nothing else, they document the code 
in one place.

As for the header, the same prototypes are in config/i386/tm-i386.h.  Is 
that okay?

> > +/* Insert a hardware-assisted breakpoint at address ADDR.  SHADOW is
> > +   unused.  Return 0 on success, EBUSY on failure.  */
> > +int
> > +i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow)
> > +{
> > +  unsigned len_rw = i386_length_and_rw_bits (1, hw_execute);
> > +  int retval = i386_insert_aligned_watchpoint (addr, len_rw) ? EBUSY : 0;
> > +
> > +  if (maint_show_dr)
> > +    i386_show_dr ("insert_hwbp", addr, 1, hw_execute);
> > +
> > +  return retval;
> > +}
> 
> What's this crap about EBUSY?

It was in the go32-nat.c code which served as a prototype.  IIRC, EBUSY 
is used in an error message printed by GDB when a breakpoint cannot be 
inserted.  Perhaps I'm mistaken.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]