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


   Date: Wed, 7 Mar 2001 19:21:23 +0200 (IST)
   From: Eli Zaretskii <eliz@is.elta.co.il>

   > 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?

Yep.  But instead of multi-arching all those functions we should
probably use an approach similar to what has been done for shared
library support (`struct target_so_ops').

   > 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.

Considering it as SEP.  But as long as it hasn't been address the
could shouldn't be put in i386-tdep.c.

   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?

Simply because I'd like i386-tdep.c to contain only code that's
multi-arch ready.  GDB already contains too many hacks that never got
cleaned up.  You're code is very useful for native targets, but simply
isn't multi-arch ready.

   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.

If someone really wants to use the code in non-native targets he/she
should address the multi-arch problems.

   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.

Yes, but the actual implementation of the I386_DR_LOW_* will live in
an entirely different file.  This stuff is totally unrelevant for the
implementation of the generic code.

   > > +/* 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.

See my previous comment.

   > > +/* 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?

It's much easier to maintain one single copy of these declarations
than having them scattered all around the place.

   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.

I think you are.

Mark


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