[RFA- v2] Remove CANNOT_STEP_HW_WATCHPOINTS related code (was fix for bug 11531)

["Pierre Muller" <pierre dot muller at ics-cnrs dot unistra dot fr>]

> -----Message d'origine-----
> De?: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé?: Sunday, April 25, 2010 10:10 PM
> À?: Pierre Muller
> Cc?: gdb-patches@sourceware.org; 'Joel Brobecker'
> Objet?: Re: [RFA- v2] Testcase for bug report 11531 and fix for Solaris
> 
> On Saturday 24 April 2010 16:13:06, Pierre Muller wrote:
> >     Work around the problem by removing hardware watchpoints if a
> step is
> >     requested, GDB will check for a hardware watchpoint trigger after
> the
> >     step anyway.  */
> > -#define CANNOT_STEP_HW_WATCHPOINTS
> > +/* The code related to this macro does not work
> > +   anymore. Thus we remove this macro definition.  */
> > +/* #define CANNOT_STEP_HW_WATCHPOINTS */
> 
> The new comment isn't correct.  I'd like to clarify this, at least
> for the archives.

  Thanks!
 
> It's not that the code does not work anymore.  The _main_ issue the
> workaround handles, is the fact that on older Solaris kernels, when
> stepping around a page that has a watchpoint installed, the kernel
> would forget the step, and hence, the inferior would run free.  The
> workaround should still be preventing that as is.

  OK, that is also what I understood.
 
> What no longer works since the workaround was written, is that GDB
> used to check if there was any watchpoint that triggered after
> all single-steps, regardless of whether the target indicating common
> code a watchpoint triggered or not, hence PR 11531.
> 
> Obviously, having the inferior randomly running free when the user
> does "step" or "next" is worse than missing a watchpoint when
> stepping.
> 
> If we still wanted to support Solaris <= 2.7, we'd
> still want the workaround in some form.  So, it's not that
> it "doesn't work anymore" -- it's that the current workaround
> implementation is incomplete because it breaks something else.
> For example, we'd need to somehow make GDB core check for
> watchpoints after every single-step on this target.  And, we
> would indeed want to make this conditional on the currently
> running kernel version, because although it should be relatively
> simple to workaround PR11531 (regular watchpoints), by always
> checking for watched value changes after all single-steps,
> read and write watchpoints, would be harder to unbreak.
> (yes, procfs.c doesn't report the stopped data address today,
> so read and write watchpoints don't work anyway, but it could
> report it, as Solaris' procfs does support that, IIRC).
  I have a patch ready for that, but 
didn't want to send it before this was resolved.


> As nobody said they're still interested in those older Solaris'
> kernel, let's indeed just go the simple route of removing the whole
> workaround, and not leave that code commented out (as Joel suggested),
> but please let's keep finally removing the nm file and the
> associated glue for a separate patch.


  In that case, the configure.tgt script should probably report 
that Solaris versions <= 2.7 are not supported anymore.
This is what I added to configure.tgt below.


  Is this patch OK?

  Eli, is the simple removal from gdbint.texinfo OK, or should we
state in the internal docs that this macro was removed?

Pierre

PS: As agreed, I will wait until that part is removed
before resending the patch that removes nm-i386-sol2.h


  
ChangeLog entry:
2010-04-26  Pierre Muller  <muller@ics.u-strasbg.fr>

	PR breakpoints/11531.
	* configure.tgt (Solaris target): Restrict support
	to versions >= 2.8.
	* infrun.c (CANNOT_STEP_HW_WATCHPOINTS): Remove macro.
	(resume): Remove code and comment related to this macro.

doc ChangeLog entry:

2010-04-26  Pierre Muller  <muller@ics.u-strasbg.fr>

	* gdbint.texinfo (CANNOT_STEP_HW_WATCHPOINTS): Remove explanation
	of macro deleted from GDB code.

Index: src/gdb/configure.tgt
===================================================================
RCS file: /cvs/src/src/gdb/configure.tgt,v
retrieving revision 1.231
diff -u -p -r1.231 configure.tgt
--- src/gdb/configure.tgt	20 Apr 2010 00:21:33 -0000	1.231
+++ src/gdb/configure.tgt	26 Apr 2010 07:47:51 -0000
@@ -197,8 +197,10 @@ i[34567]86-*-solaris2.1[0-9]* | x86_64-*
 			i386-sol2-tdep.o sol2-tdep.o \
 			corelow.o solib.o solib-svr4.o"
 	;;
-i[34567]86-*-solaris*)
-	# Target: Solaris x86
+i[34567]86-*-solaris2.[8-9] )
+	# Target: Solaris x86, only versions 2.8 and 2.9
+	# Versions <= 2.7 suffer a bug that was handled in older versions of

+	# GDB, but that code was removed.
 	gdb_target_obs="i386-tdep.o i387-tdep.o i386-sol2-tdep.o sol2-tdep.o
\
 			corelow.o solib.o solib-svr4.o"
 	;;
Index: src/gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.435
diff -u -p -r1.435 infrun.c
--- src/gdb/infrun.c	25 Mar 2010 20:48:53 -0000	1.435
+++ src/gdb/infrun.c	26 Apr 2010 07:35:07 -0000
@@ -179,16 +179,6 @@ show_debug_infrun (struct ui_file *file,
 #endif
 
 
-/* Convert the #defines into values.  This is temporary until wfi control
-   flow is completely sorted out.  */
-
-#ifndef CANNOT_STEP_HW_WATCHPOINTS
-#define CANNOT_STEP_HW_WATCHPOINTS 0
-#else
-#undef  CANNOT_STEP_HW_WATCHPOINTS
-#define CANNOT_STEP_HW_WATCHPOINTS 1
-#endif
-
 /* Tables of how to react to signals; the user sets them.  */
 
 static unsigned char *signal_stop;
@@ -1484,18 +1474,6 @@ resume (int step, enum target_signal sig
 			"trap_expected=%d\n",
  			step, sig, tp->trap_expected);
 
-  /* Some targets (e.g. Solaris x86) have a kernel bug when stepping
-     over an instruction that causes a page fault without triggering
-     a hardware watchpoint. The kernel properly notices that it shouldn't
-     stop, because the hardware watchpoint is not triggered, but it forgets
-     the step request and continues the program normally.
-     Work around the problem by removing hardware watchpoints if a step is
-     requested, GDB will check for a hardware watchpoint trigger after the
-     step anyway.  */
-  if (CANNOT_STEP_HW_WATCHPOINTS && step)
-    remove_hw_watchpoints ();
-
-
   /* Normally, by the time we reach `resume', the breakpoints are either
      removed or inserted, as appropriate.  The exception is if we're
sitting
      at a permanent breakpoint; we need to step over it, but permanent
Index: src/gdb/doc/gdbint.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdbint.texinfo,v
retrieving revision 1.321
diff -u -p -r1.321 gdbint.texinfo
--- src/gdb/doc/gdbint.texinfo	10 Mar 2010 18:20:07 -0000	1.321
+++ src/gdb/doc/gdbint.texinfo	26 Apr 2010 07:40:23 -0000
@@ -781,11 +781,6 @@ inferior after a watchpoint has been hit
 when watchpoints trigger at the instruction following an interesting
 read or write.
 
-@findex CANNOT_STEP_HW_WATCHPOINTS
-@item CANNOT_STEP_HW_WATCHPOINTS
-If this is defined to a non-zero value, @value{GDBN} will remove all
-watchpoints before stepping the inferior.
-
 @findex STOPPED_BY_WATCHPOINT
 @item STOPPED_BY_WATCHPOINT (@var{wait_status})
 Return non-zero if stopped by a watchpoint.  @var{wait_status} is of