Re: [RFA] Unified watchpoints for x86 platforms

[Eli Zaretskii <eliz@is.elta.co.il>]

On Fri, 9 Mar 2001, Mark Kettenis wrote:

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

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?

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

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

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

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