diff mbox

parisc: Improve ptrace support for gdb single-step

Message ID BLU0-SMTP34ED8A5FC4451C5BECF0F6973F0@phx.gbl (mailing list archive)
State Accepted
Headers show

Commit Message

John David Anglin Dec. 28, 2012, 11:18 p.m. UTC
Various GCC tests use gdb to simulate a multithreaded application.  Many
of these tests have been failing on parisc linux.

GCC does this by using gdb to single-step the application, then gdb is  
used to
call other test specific code.  Where this fails is when the  
application is stepped
into the delay slot of a taken branch.  This sets the PSW B bit.  When  
the test
specific code is executed, this usually clears the PSW B bit.   
Currently, gdb is
not allowed to set the B bit.  So, the code falls through what should  
be a taken
branch.

The attached patch adds the PSW B bit to the set of bits that gdb is  
allowed to
set.  In order to set the B bit, the trace system call must return  
using an interrupt
restore.  The patch also modifies this code to use the saved IAOQ  
values when
they are saved by a ptrace syscall or interruption.

Signed-off-by: John David Anglin <dave.anglin@bell.net>
---

--
John David Anglin	dave.anglin@bell.net

Comments

John David Anglin Dec. 29, 2012, 4:42 p.m. UTC | #1
On 29-Dec-12, at 8:31 AM, Carlos O'Donell wrote:

>
> On Dec 28, 2012 6:18 PM, "John David Anglin" <dave.anglin@bell.net>  
> wrote:
> >
> > Various GCC tests use gdb to simulate a multithreaded  
> application.  Many
> > of these tests have been failing on parisc linux.
> >
> > GCC does this by using gdb to single-step the application, then  
> gdb is used to
> > call other test specific code.  Where this fails is when the  
> application is stepped
> > into the delay slot of a taken branch.  This sets the PSW B bit.   
> When the test
> > specific code is executed, this usually clears the PSW B bit.   
> Currently, gdb is
> > not allowed to set the B bit.  So, the code falls through what  
> should be a taken
> > branch.
>
> How exactly does this trigger? Step into delay slot, then what, call  
> an inferior function, then resume?
>
Yes.

I have attached a test case and gdb script which show bug.

Compile testcase with something like the following:
Executing on host: /home/dave/gnu/gcc/objdir/gcc/xgcc -B/home/dave/gnu/ 
gcc/objdir/gcc/ /home/dave/gnu/gcc/gcc/gcc/testsuite/gcc.dg/simulate- 
thread/speculative-store-4.c  -fno-diagnostics-show-caret   -O0 -g  -- 
param allow-store-data-races=0  -lm   -o speculative-store-4.exe

Execute with something like:
gdb -nx -nw -x /home/dave/gnu/gcc/gcc/gcc/testsuite/gcc.dg/simulate- 
thread/simulate-thread.gdb ./speculative-store-4.exe

You can see that the gdb script executes inferior functions to  
simulate thread behavior and to determine
whether the test is done.

I debugged this by adding extra print and disp statements to the  
kernel, gdb and the gdb script.

According to the PA 2.0 arch, a non sequential iaoq queue without the  
B bit being set invokes undefined
behavior.  Possibly, this should be checked for when the return is  
setup after pt_regs_ok.  However, I
think it is ok to let gdb play with the B bit.  See the definition of  
USER_PSW_MASK in psw.h:

#define USER_PSW_MASK (WIDE_PSW | PSW_T | PSW_N | PSW_X | PSW_B |  
PSW_V | PSW_CB)

This defines the PSW bits that the user can change using exception  
support.  Probably, the two sets
should be the same.

> > The attached patch adds the PSW B bit to the set of bits that gdb  
> is allowed to
> > set.  In order to set the B bit, the trace system call must return  
> using an interrupt
> > restore.  The patch also modifies this code to use the saved IAOQ  
> values when
> > they are saved by a ptrace syscall or interruption.
> >
> > Signed-off-by: John David Anglin <dave.anglin@bell.net>
>
> What testing did you do with patch?
>
I have been running the change with 3.7.x kernels for a couple of  
weeks.  I checked that strace still works
and GCC test results for tests using gdb are now similar to the  
results on hpux.

I believe gdb still has problems regarding delay slots, PSW bits and  
inferior function calls.  This mainly shows
up in the GCC guality tests.  Of course, there are hundreds of FAILs  
in the gdb testsuite...

Dave
--
John David Anglin	dave.anglin@bell.net
/* { dg-do link } */
/* { dg-options "--param allow-store-data-races=0" } */
/* { dg-final { simulate-thread } } */

#include <stdio.h>
#include <stdlib.h>

#include "simulate-thread.h"

/* PR 54139 */
/* Test that speculative stores do not happen for --param
   allow-store-data-races=0.  */

int g_13=1, insns=1;

__attribute__((noinline))
void simulate_thread_main()
{
  int l_245;

  /* Since g_13 is unilaterally set positive above, there should be
     no store to g_13 below.  */
  for (l_245 = 0; l_245 <= 1; l_245 += 1)
    for (; g_13 <= 0; g_13 = 1)
      ;
}

int main()
{
  simulate_thread_main ();
  simulate_thread_done ();
  return 0;
}

void simulate_thread_other_threads ()
{
  ++g_13;
  ++insns;
}

int simulate_thread_step_verify ()
{
  return 0;
}

int simulate_thread_final_verify ()
{
  if (g_13 != insns)
    {
      printf("FAIL: g_13 was incorrectly cached\n");
      return 1;
    }
  return 0;
}
set height 0
break simulate_thread_main
# disp/i $pc
run

set $ret = 0
while (simulate_thread_fini != 1) && (! $ret)
  set $ret |= simulate_thread_wrapper_other_threads()
  stepi
  set $ret |= simulate_thread_step_verify()
end

if (! $ret)
  set $ret |= simulate_thread_wrapper_final_verify()
end
continue
quit $ret
John David Anglin Dec. 29, 2012, 4:56 p.m. UTC | #2
On 29-Dec-12, at 11:42 AM, John David Anglin wrote:

> I have attached a test case and gdb script which show bug.


Forgot .h file.

Dave
--
John David Anglin	dave.anglin@bell.net
int simulate_thread_fini = 0;

void __attribute__((noinline))
simulate_thread_done ()
{
  simulate_thread_fini = 1;
}

/* A hostile thread is one which changes a memory location so quickly
   that another thread may never see the same value again.  This is
   simulated when simulate_thread_other_thread() is defined to modify
   a memory location every cycle.

   A process implementing a dependency on this value can run into
   difficulties with such a hostile thread.  For instance,
   implementing an add with a compare_and_swap loop goes something
   like:

     expected = *mem;
   loop:
     new = expected += value;
     if (!succeed (expected = compare_and_swap (mem, expected, new)))
       goto loop;

   If the content of 'mem' are changed every cycle by
   simulate_thread_other_thread () this will become an infinite loop
   since the value *mem will never be 'expected' by the time the
   compare_and_swap is executed.

   HOSTILE_THREAD_THRESHOLD defines the number of intructions which a
   program will execute before triggering the hostile thread
   pause. The pause will last for HOSTILE_THREAD_PAUSE instructions,
   and then the counter will reset and begin again.  During the pause
   period, simulate_thread_other_thread will not be called.

   This provides a chance for forward progress to be made and the
   infinite loop to be avoided.

   If the testcase defines HOSTILE_PAUSE_ERROR, then it will be
   considered a RUNTIME FAILURE if the hostile pause is triggered.
   This will allow to test for guaranteed forward progress routines.

   If the default values for HOSTILE_THREAD_THRESHOLD or
   HOSTILE_THREAD_PAUSE are insufficient, then the testcase may
   override these by defining the values before including this file.

   Most testcase are intended to run for very short periods of time,
   so these defaults are considered to be high enough to not trigger
   on a typical case, but not drag the test time out too much if a
   hostile condition is interferring.  */

  
/* Define the threshold instruction count to start pausing the hostile 
   thread.  To avoid huge potential log files when things are not going well,
   set this number very low.  If a test specifically requires that the forward
   progress guarantee is made, this number should be raised by the testcase. */
#if !defined (HOSTILE_THREAD_THRESHOLD)
#define HOSTILE_THREAD_THRESHOLD 	50
#endif

/* Define the length of pause in cycles for the hostile thread to pause to
   allow forward progress to be made.  If this number is too low, a 
   compare_and_swap loop may not have time to finish, especially on a
   128 bit operation. */
#if !defined (HOSTILE_THREAD_PAUSE)
#define HOSTILE_THREAD_PAUSE	20
#endif

/* Define the number of instructions which are allowed to be executed before
   the testcase is deemed to fail.  This is primarily to avoid huge log files
   when a testcase goes into an infinte loop.  */
#if !defined (INSN_COUNT_THRESHOLD)
#define INSN_COUNT_THRESHOLD	10000
#endif

void simulate_thread_other_threads (void);
int simulate_thread_final_verify (void);

static int simulate_thread_hostile_pause = 0;

/* This function wraps simulate_thread_other_threads an monitors for
   an infinite loop.  If the threshold value HOSTILE_THREAD_THRESHOLD
   is reached, the other_thread process is paused for
   HOSTILE_THREAD_PAUSE cycles before resuming, and the counters start
   again.  */
int
simulate_thread_wrapper_other_threads()
{
  static int insn_count = 0;
  static int hostile_count = 0;
  static int hostile_pause = 0;

  if (++insn_count >= INSN_COUNT_THRESHOLD)
    {
      printf ("FAIL: Testcase exceeded maximum instruction count threshold\n");
      return 1;
    }

  if (++hostile_count >= HOSTILE_THREAD_THRESHOLD)
    {
      if (!simulate_thread_hostile_pause)
        simulate_thread_hostile_pause = 1;

      /* Count cycles before calling the hostile thread again.  */
      if (hostile_pause++ < HOSTILE_THREAD_PAUSE)
	return 0;

      /* Reset the pause counter, as well as the thread counter.  */
      hostile_pause = 0;
      hostile_count = 0;
    }
  simulate_thread_other_threads ();
  return 0;
}


/* If the test case defines HOSTILE_PAUSE_ERROR, then the test case
   will fail execution if it had a hostile pause.  */
int
simulate_thread_wrapper_final_verify ()
{
  int ret = simulate_thread_final_verify ();
#if defined (HOSTILE_PAUSE_ERROR)
  if (simulate_thread_hostile_pause)
    {
      printf ("FAIL: Forward progress made only by pausing hostile thread\n");
      ret = ret | 1;    /* 0 indicates proper comnpletion.  */
    }
#endif
  return ret;
}
diff mbox

Patch

diff --git a/arch/parisc/kernel/entry.S b/arch/parisc/kernel/entry.S
index 18670a0..c19b2b0 100644
--- a/arch/parisc/kernel/entry.S
+++ b/arch/parisc/kernel/entry.S
@@ -2064,7 +2057,7 @@  syscall_restore:
 
 	/* Are we being ptraced? */
 	ldw	TASK_FLAGS(%r1),%r19
-	ldi	(_TIF_SINGLESTEP|_TIF_BLOCKSTEP),%r2
+	ldi	_TIF_SYSCALL_TRACE_MASK,%r2
 	and,COND(=)	%r19,%r2,%r0
 	b,n	syscall_restore_rfi
 
@@ -2177,15 +2170,23 @@  syscall_restore_rfi:
 	/* sr2 should be set to zero for userspace syscalls */
 	STREG	%r0,TASK_PT_SR2(%r1)
 
-pt_regs_ok:
 	LDREG	TASK_PT_GR31(%r1),%r2
-	depi	3,31,2,%r2			   /* ensure return to user mode. */
-	STREG	%r2,TASK_PT_IAOQ0(%r1)
+	depi	3,31,2,%r2		   /* ensure return to user mode. */
+	STREG   %r2,TASK_PT_IAOQ0(%r1)
 	ldo	4(%r2),%r2
 	STREG	%r2,TASK_PT_IAOQ1(%r1)
+	b	intr_restore
 	copy	%r25,%r16
+
+pt_regs_ok:
+	LDREG	TASK_PT_IAOQ0(%r1),%r2
+	depi	3,31,2,%r2		   /* ensure return to user mode. */
+	STREG	%r2,TASK_PT_IAOQ0(%r1)
+	LDREG	TASK_PT_IAOQ1(%r1),%r2
+	depi	3,31,2,%r2
+	STREG	%r2,TASK_PT_IAOQ1(%r1)
 	b	intr_restore
-	nop
+	copy	%r25,%r16
 
 	.import schedule,code
 syscall_do_resched:
diff --git a/arch/parisc/kernel/ptrace.c b/arch/parisc/kernel/ptrace.c
index 857c2f5..534abd4 100644
--- a/arch/parisc/kernel/ptrace.c
+++ b/arch/parisc/kernel/ptrace.c
@@ -26,7 +26,7 @@ 
 #include <asm/asm-offsets.h>
 
 /* PSW bits we allow the debugger to modify */
-#define USER_PSW_BITS	(PSW_N | PSW_V | PSW_CB)
+#define USER_PSW_BITS	(PSW_N | PSW_B | PSW_V | PSW_CB)
 
 /*
  * Called by kernel/ptrace.c when detaching..