Committed: fix sim/cris/pipe2.c

[Hans-Peter Nilsson <hans-peter dot nilsson at axis dot com>]

It's odd that the pipe2 test ever ran successfully.  The test-case had
both bugs and wrong assumptions.  On the host, the pipe was always closed
when the second write came about.  In the simulator, the test-case ran
into a sanity check in the pipe machinery triggered by the (large) amount
of buffered, not yet consumed, pipe data.  The assumptions for the
return-value from write weren't really sane.  When fixed, I ran into a bug
in the CRIS pipe-empty hook which had been somewhat cloaked in the
test-case by expecting the second pipe-write to return zero.  Fixed
test-case also runs on host as well as actual target HW, heh.

sim:
	* cris/traps.c (TARGET_PIPE_BUF): New macro.
	(cris_pipe_empty): Correct initialization of "remaining".  Only
	adjust the "write" return value if more than TARGET_PIPE_BUF bytes
	are written.

sim/testsuite:
	* sim/cris/c/pipe2.c: Adjust expected output.
	(process): Don't write as much to the pipe as to trig the
	inordinate-amount test in the sim pipe machinery.  Correct test of
	write return-value; check only that pipemax bytes were
	successfully written.  For error-case, emit strerror as well.
	(main): Add a second read.

Index: cris/traps.c
===================================================================
RCS file: /cvs/src/src/sim/cris/traps.c,v
retrieving revision 1.8
diff -p -u -r1.8 traps.c
--- cris/traps.c	29 Sep 2006 02:45:48 -0000	1.8
+++ cris/traps.c	30 Sep 2006 01:27:00 -0000
@@ -240,6 +240,9 @@ with this program; if not, write to the 
 #define TARGET___WALL 0x40000000
 #define TARGET___WCLONE 0x80000000
 
+/* From linux/limits.h. */
+#define TARGET_PIPE_BUF 4096
+
 static const char stat_map[] =
 "st_dev,2:space,10:space,4:st_mode,4:st_nlink,4:st_uid,4"
 ":st_gid,4:st_rdev,2:space,10:st_size,8:st_blksize,4:st_blocks,4"
@@ -2971,13 +2974,14 @@ cris_pipe_nonempty (host_callback *cb AT
 
 static void
 cris_pipe_empty (host_callback *cb,
-		 int reader ATTRIBUTE_UNUSED,
+		 int reader,
 		 int writer)
 {
   int i;
   SIM_CPU *cpu = current_cpu_for_cb_callback;
   bfd_byte r10_buf[4];
-  int remaining = cb->pipe_buffer[writer].size;
+  int remaining
+    = cb->pipe_buffer[writer].size - cb->pipe_buffer[reader].size;
 
   /* We need to find the thread that waits for this pipe.  */
   for (i = 0; i < SIM_TARGET_MAX_THREADS; i++)
@@ -2985,6 +2989,7 @@ cris_pipe_empty (host_callback *cb,
 	&& cpu->thread_data[i].pipe_write_fd == writer)
       {
 	int retval;
+
 	/* Temporarily switch to this cpu context, so we can change the
 	   PC by ordinary calls.  */
 
@@ -2995,19 +3000,25 @@ cris_pipe_empty (host_callback *cb,
 		cpu->thread_data[i].cpu_context,
 		cpu->thread_cpu_data_size);
 
-	/* The return value is supposed to contain the number of written
-	   bytes, which is the number of bytes requested and returned at
-	   the write call.  We subtract the remaining bytes from that,
-	   but making sure we still get a positive number.
-	   The return value may also be a negative number; an error
-	   value.  We cover this case by comparing against remaining,
-	   which is always >= 0.  */
+	/* The return value is supposed to contain the number of
+	   written bytes, which is the number of bytes requested and
+	   returned at the write call.  You might think the right
+	   thing is to adjust the return-value to be only the
+	   *consumed* number of bytes, but it isn't.  We're only
+	   called if the pipe buffer is fully consumed or it is being
+	   closed, possibly with remaining bytes.  For the latter
+	   case, the writer is still supposed to see success for
+	   PIPE_BUF bytes (a constant which we happen to know and is
+	   unlikely to change).  The return value may also be a
+	   negative number; an error value.  This case is covered
+	   because "remaining" is always >= 0.  */
 	(*CPU_REG_FETCH (cpu)) (cpu, H_GR_R10, r10_buf, 4);
 	retval = (int) bfd_getl_signed_32 (r10_buf);
-	if (retval >= remaining)
-	  bfd_putl32 (retval - remaining, r10_buf);
-	(*CPU_REG_STORE (cpu)) (cpu, H_GR_R10, r10_buf, 4);
-
+	if (retval - remaining > TARGET_PIPE_BUF)
+	  {
+	    bfd_putl32 (retval - remaining, r10_buf);
+	    (*CPU_REG_STORE (cpu)) (cpu, H_GR_R10, r10_buf, 4);
+	  }
 	sim_pc_set (cpu, sim_pc_get (cpu) + 2);
 	memcpy (cpu->thread_data[i].cpu_context,
 		&cpu->cpu_data_placeholder,

Index: sim/cris/c/pipe2.c
===================================================================
RCS file: /cvs/src/src/sim/testsuite/sim/cris/c/pipe2.c,v
retrieving revision 1.1
diff -p -u -r1.1 pipe2.c
--- sim/cris/c/pipe2.c	21 Nov 2005 04:48:19 -0000	1.1
+++ sim/cris/c/pipe2.c	30 Sep 2006 01:27:17 -0000
@@ -1,6 +1,6 @@
 /* Check that closing a pipe with a nonempty buffer works.
 #notarget: cris*-*-elf
-#output: got: a\nexit: 0\n
+#output: got: a\ngot: b\nexit: 0\n
 */
 
 
@@ -14,7 +14,7 @@
 #include <errno.h>
 #include <sys/types.h>
 #include <sys/wait.h>
-
+#include <string.h>
 int pip[2];
 
 int pipemax;
@@ -23,7 +23,8 @@ int
 process (void *arg)
 {
   char *s = arg;
-  char *buf = malloc (pipemax * 100);
+  int lots = pipemax + 256;
+  char *buf = malloc (lots);
   int ret;
 
   if (buf == NULL)
@@ -37,12 +38,17 @@ process (void *arg)
 
   *buf = s[1];
 
-  /* The second write should only successful for at most the PIPE_MAX
-     part, but no error.  */
-  ret = write (pip[1], buf, pipemax * 10);
-  if (ret != 0 && ret != pipemax - 1 && ret != pipemax)
+  /* The second write may or may not be successful for the whole
+     write, but should be successful for at least the pipemax part.
+     As linux/limits.h clamps PIPE_BUF to 4096, but the page size is
+     actually 8k, we can get away with that much.  There should be no
+     error, though.  Doing this on host shows that for
+     x86_64-unknown-linux-gnu (2.6.14-1.1656_FC4) pipemax * 10 can be
+     successfully written, perhaps for similar reasons.  */
+  ret = write (pip[1], buf, lots);
+  if (ret < pipemax)
     {
-      fprintf (stderr, "ret: %d\n", ret);
+      fprintf (stderr, "ret: %d, %s, %d\n", ret, strerror (errno), pipemax);
       fflush (0);
       abort ();
     }
@@ -104,6 +110,19 @@ main (void)
 
   printf ("got: %c\n", buf[0]);
 
+  /* Need to read out something from the second write too before
+     closing, or the writer can get EPIPE. */
+  while ((retcode = read (pip[0], buf, 1)) == 0)
+    ;
+
+  if (retcode != 1)
+    {
+      fprintf (stderr, "Bad read 2: %d\n", retcode);
+      abort ();
+    }
+
+  printf ("got: %c\n", buf[0]);
+
   if (close (pip[0]) != 0)
     {
       perror ("pip close");


brgds, H-P