Re: [RFA] Unified watchpoints for x86 platforms

[Mark Kettenis <kettenis at science dot uva dot nl>]

   Date: Sun, 11 Mar 2001 13:16:19 +0200 (IST)
   From: Eli Zaretskii <eliz@is.elta.co.il>

   On Fri, 9 Mar 2001, Mark Kettenis wrote:

   > Simply because I'd like i386-tdep.c to contain only code that's
   > multi-arch ready.

   Why is this goal so important that it justifies preventing
   non-multiarched targets from using watchpoints, and creating a
   separate file on top of that?

Long term maintainability of the code.

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

   IMHO, this means we are being too harsh to target maintainers.

   Anyway, since you insist on moving the code to a separate file, I'll
   do that.  I just wish I understood the motivation for that better than
   I do now.

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

   That's one possibility.  What I had in mind was something different;
   for example:

     #define I386_DR_LOW_SET_ADDR(dr,addr) \
	ptrace (6, inferior_pid, offsetof (struct user, u_debugreg[dr]),(addr))

     #define I386_DR_LOW_SET_CONTROL(val) \
	ptrace (6, inferior_pid, offsetof (struct user, u_debugreg[DR_CONTROL]),(val))

We certainly don't want to encourage that kind of macro's.  Again this
style of coding has caused many problems in the past.  We've been
converting these kind of macro's into proper functions all over the
place over the last couple of years.

   I thought that when the macros are defined like this, ptrace.h would
   be most useful.

   While writing the code, I tried to make it very easy for the targets
   to start using it.  As written, all they need to do is define a small
   number of macros in tm-*.h header and say "./configure; make".

I don't think that demanding people to provide proper functions is
making the implementation that much harder.

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

   Here's the relevant snippet from breakpoint.c:insert_breakpoints (with 
   some of the code removed for brevity):

	   if (b->type == bp_hardware_breakpoint)
	     val = target_insert_hw_breakpoint (b->address, b->shadow_contents);
	   else
	     {
	      ...
		 val = target_insert_breakpoint (b->address, b->shadow_contents);
	     }
	   if (val)
	     {
	       /* Can't set the breakpoint.  */
   #if defined (DISABLE_UNSETTABLE_BREAK)
	      ...
   #endif
		 {
		   target_terminal_ours_for_output ();
		   warning ("Cannot insert breakpoint %d:", b->number);
   #ifdef ONE_PROCESS_WRITETEXT
		   warning ("The same program may be running in another process.");
   #endif
		   memory_error (val, b->address);	   /* which bombs us out */
		 }
	     }

   As you see, it calls memory_error with the value returned by
   target_insert_hw_breakpoint.  memory_error then interprets this arg
   as a value of errno and prints the text returned by safe_strerror for
   it.

Ah, I didn't really look at the hardware breakpoint stuff, only at the
watchpoints.  memory_error isn't really appropriate here of course.

   So the question is: what do we want GDB to print when it fails to
   insert a breakpoint, hardware or otherwise?

I'm not sure.  We should really fix the stuff in breakpoint.c to not
assume that all breakpoints are implemented by doing some sort of
memory access.  Meanwhile, returning EBUSY for hardware watchpoints is
probably fine.  But I think that the case where you propose to return
EINVAL is really a GDB internal error.

Mark