RFC Fix for 9964.cc and 9507.cc on ia64-hpux

["Zack Weinberg" <zack at codesourcery dot com>]

The libstdc++ tests 27_io/basic_filebuf/close/char/9964.cc and
27_io/basic_filebuf/open/char/9507.cc have been failing on ia64-hpux
for a very long time (specifically, they broke in between 3.3.2 and
3.3.3 - they weren't called that then, but they were present and
failing).  The majority of the problem is bad test case code.
However, there are also severe issues with _M_open_mode.  The
following patch attempts to fix both.

In 9507.cc, we want to open a FIFO with ios_base::ate so that it will
try to seek and fail.  The bug logs clearly indicate that the topic of
the test is a successful open but a failed seek.  Now, the current
libstdc++ basic_filebuf is a wrapper around stdio.  There is no code
string you can pass to fopen() which will make it call open() with
O_WRONLY and not O_TRUNC.  The HPUX kernel rejects O_WRONLY|O_TRUNC on
a FIFO (errno set to EINVAL).  The VERIFY()s succeed in this case,
since you do get a closed stream, but the reader process blocks
forever waiting for *someone* to open the other end of the FIFO, which
makes the test time out.  9964.cc is the same way, only here we get an
explicit failure (and then the test times out).

The cure proposed in this patch is to use in|out|ate (9507) or in|out
(9964) which corresponds to "r+" and O_RDWR handed down to the kernel.  
The HPUX kernel is happy with this.  I've also added calls to alarm()
and wait() to ensure that the tests never run for more than ten
seconds nor do they leave child processes hanging around.

Unfortunately, there may be some other UNIX kernel that objects to
opening FIFOs with O_RDWR.  As I said above, there is no way to get
fopen() to call open() with just O_WRONLY.  We cannot use an explicit
open() and then basic_filebuf::sys_open() because the latter ignores
ios_base::ate; if this problem exists in real life, we're going to
have to add an entry point that does what sys_open() does but honors
ios_base::ate.

_M_open_mode gets the "Not Helping" award - it is coded in a severely
obfuscated fashion which makes it next-to-impossible to tell whether
it is actually producing the correct fopen() code strings from
openmode flags.  It is also doing somewhat more work than is
necessary, and it has no error handling whatsoever.  Pass in a
combination of flags that it doesn't handle, and you get a garbage
second argument to fopen.  I have gutted and rewritten it to hew
strictly to the table in [lib.filebuf.members]/2.  Unfortunately, this
constitutes an ABI change since _M_open_mode was exported.  I've
handled this by giving the rewritten function a new name, making it
static in basic_file_stdio.cc, and providing a wrapper under the old
name and interface (it doesn't actually *implement* the old interface
- the __rw_mode and __p_mode args are ignored - but no one should be
calling this function anyway).  I would like to dispense with the
wrapper, but I don't know if it's safe.

So, comments?  Ideally I would like to apply variations on this patch
to the 3.3 and 3.4 branches as well as mainline.

Bootstrapped ia64-hpux.

zw


        * config/io/basic_file_stdio.cc (fopen_mode): New static
        function. 
        (__basic_file<char>::sys_open, __basic_file<char>::open):
        Use fopen_mode, not _M_open_mode.
        (__basic_file<char>::_M_open_mode): Now just a
        backward-compatibility wrapper around fopen_mode.
        * config/io/basic_file_stdio.h: Wrap _M_open_mode in
        _GLIBCXX_DEPRECATED.

        * testsuite/27_io/basic_filebuf/close/char/9964.cc:
        Call fb.open with ios_base::in|ios_base::out, not
        ios_base::out|ios_base::trunc.  Set alarms to prevent
        hanging.  Wait for child process.

        * testsuite/27_io/basic_filebuf/open/char/9507.cc:
        Call fbuf.open with ios_base::in|ios_base::out|ios_base::ate, 
        not ios_base::out|ios_base::ate.  Set alarms to prevent
        hanging.  Wait for child process.

===================================================================
Index: config/io/basic_file_stdio.cc
--- config/io/basic_file_stdio.cc	10 Dec 2003 17:37:22 -0000	1.25
+++ config/io/basic_file_stdio.cc	3 Feb 2004 02:30:44 -0000
@@ -70,6 +70,38 @@
 
 #include <limits> // For <off_t>::max() and min()
 
+// Map ios_base::openmode flags to a string for use in fopen().
+// Table of valid combinations as given in [lib.filebuf.members]/2.
+static const char *fopen_mode(std::ios_base::openmode mode)
+{
+  enum {
+    in     = std::ios_base::in,
+    out    = std::ios_base::out,
+    trunc  = std::ios_base::trunc,
+    app    = std::ios_base::app,
+    binary = std::ios_base::binary
+  };
+
+  switch (mode & (in|out|trunc|app|binary))
+    {
+    case (   out                 ): return "w";  
+    case (   out      |app       ): return "a";  
+    case (   out|trunc           ): return "w";  
+    case (in                     ): return "r";  
+    case (in|out                 ): return "r+"; 
+    case (in|out|trunc           ): return "w+"; 
+
+    case (   out          |binary): return "wb"; 
+    case (   out      |app|binary): return "ab"; 
+    case (   out|trunc    |binary): return "wb"; 
+    case (in              |binary): return "rb"; 
+    case (in|out          |binary): return "r+b";
+    case (in|out|trunc    |binary): return "w+b";
+
+    default: return 0; // invalid
+    }
+}
+
 namespace std 
 {
   // Definitions for __basic_file<char>.
@@ -79,53 +111,6 @@ namespace std 
   __basic_file<char>::~__basic_file()
   { this->close(); }
       
-  void 
-  __basic_file<char>::_M_open_mode(ios_base::openmode __mode, int& __p_mode, 
-				   int&, char* __c_mode)
-  {  
-    bool __testb = __mode & ios_base::binary;
-    bool __testi = __mode & ios_base::in;
-    bool __testo = __mode & ios_base::out;
-    bool __testt = __mode & ios_base::trunc;
-    bool __testa = __mode & ios_base::app;
-      
-    // Set __c_mode for use in fopen.
-    // Set __p_mode for use in open.
-    if (!__testi && __testo && !__testt && !__testa)
-      {
-	strcpy(__c_mode, "w");
-	__p_mode = O_WRONLY | O_CREAT;
-      }
-    if (!__testi && __testo && !__testt && __testa)
-      {
-	strcpy(__c_mode, "a");
-	__p_mode = O_WRONLY | O_CREAT | O_APPEND;
-      }
-    if (!__testi && __testo && __testt && !__testa)
-      {
-	strcpy(__c_mode, "w");
-	__p_mode = O_WRONLY | O_CREAT | O_TRUNC;
-      }
-
-    if (__testi && !__testo && !__testt && !__testa)
-      {
-	strcpy(__c_mode, "r");
-	__p_mode = O_RDONLY;
-      }
-    if (__testi && __testo && !__testt && !__testa)
-      {
-	strcpy(__c_mode, "r+");
-	__p_mode = O_RDWR | O_CREAT;
-      }
-    if (__testi && __testo && __testt && !__testa)
-      {
-	strcpy(__c_mode, "w+");
-	__p_mode = O_RDWR | O_CREAT | O_TRUNC;
-      }
-    if (__testb)
-      strcat(__c_mode, "b");
-  }
-  
   __basic_file<char>*
   __basic_file<char>::sys_open(__c_file* __file, ios_base::openmode) 
   {
@@ -144,11 +129,11 @@ namespace std 
   __basic_file<char>::sys_open(int __fd, ios_base::openmode __mode)
   {
     __basic_file* __ret = NULL;
-    int __p_mode = 0;
-    int __rw_mode = 0;
-    char __c_mode[4];
-    
-    _M_open_mode(__mode, __p_mode, __rw_mode, __c_mode);
+    const char *__c_mode = fopen_mode(__mode);
+
+    if (!__c_mode)
+      return NULL;
+
     if (!this->is_open() && (_M_cfile = fdopen(__fd, __c_mode)))
       {
 	_M_cfile_created = true;
@@ -164,11 +149,10 @@ namespace std 
 			   int /*__prot*/)
   {
     __basic_file* __ret = NULL;
-    int __p_mode = 0;
-    int __rw_mode = 0;
-    char __c_mode[4];
-      
-    _M_open_mode(__mode, __p_mode, __rw_mode, __c_mode);
+    const char *__c_mode = fopen_mode(__mode);
+
+    if (!__c_mode)
+      return NULL;
 
     if (!this->is_open())
       {
@@ -310,4 +294,15 @@ namespace std 
     return 0;
   }
 
+#ifdef _GLIBCXX_DEPRECATED
+  // For backward binary compatibility.  Nobody should be calling
+  // this function.  Does not do anything with __p_mode or __rw_mode.
+  void __basic_file<char>::
+  _M_open_mode(ios_base::openmode __mode, int&, int&, char* __c_mode)
+  {
+    char* r = fopen_mode(__mode);
+    if (r)
+      strcpy(__c_mode, r);
+  }
+#endif
 }  // namespace std
===================================================================
Index: config/io/basic_file_stdio.h
--- config/io/basic_file_stdio.h	7 Dec 2003 03:46:13 -0000	1.16
+++ config/io/basic_file_stdio.h	3 Feb 2004 02:30:44 -0000
@@ -62,10 +62,12 @@ namespace std 
 
     public:
       __basic_file(__c_lock* __lock = 0);
-      
+
+#ifdef _GLIBCXX_DEPRECATED
       void 
       _M_open_mode(ios_base::openmode __mode, int& __p_mode, int& __rw_mode, 
 		   char* __c_mode);
+#endif
       
       __basic_file* 
       open(const char* __name, ios_base::openmode __mode, int __prot = 0664);
===================================================================
Index: testsuite/27_io/basic_filebuf/close/char/9964.cc
--- testsuite/27_io/basic_filebuf/close/char/9964.cc	12 Jan 2004 08:11:05 -0000	1.5
+++ testsuite/27_io/basic_filebuf/close/char/9964.cc	3 Feb 2004 02:30:44 -0000
@@ -29,6 +29,7 @@
 #include <fcntl.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <sys/wait.h>
 #include <testsuite_hooks.h>
 
 // libstdc++/9964
@@ -45,11 +46,15 @@ void test_07()
   unlink(name);  
   try_mkfifo(name, S_IRWXU);
   
-  int child = fork();
+  pid_t child = fork();
   VERIFY( child != -1 );
 
   if (child == 0)
     {
+      // Opening a FIFO may block forever.
+      // Set an alarm to prevent this.
+      alarm(10);
+
       filebuf fbin;
       fbin.open(name, ios_base::in);
       sleep(2);
@@ -57,9 +62,19 @@ void test_07()
       exit(0);
     }
   
-  filebuf fb;
   sleep(1);
-  filebuf* ret = fb.open(name, ios_base::out | ios_base::trunc);
+
+  // Opening an FIFO may block forever.
+  // Set an alarm to prevent this.
+  alarm(10);
+
+  // There is no way to get fbuf.open to call open(2) with O_WRONLY and
+  // not O_TRUNC.  Passing O_TRUNC may cause the kernel to reject the
+  // open call.  So, we use in|out, which maps to O_RDWR, which
+  // hopefully will not cause problems.
+
+  filebuf fb;
+  filebuf* ret = fb.open(name, ios_base::in | ios_base::out);
   VERIFY( ret != NULL );
   VERIFY( fb.is_open() );
 
@@ -67,8 +82,13 @@ void test_07()
   fb.sputc('a');
 
   ret = fb.close();
-  VERIFY( ret == NULL );
+  VERIFY( ret != NULL );
   VERIFY( !fb.is_open() );
+
+  // Make sure the child did its bit correctly.
+  int status;
+  VERIFY( wait(&status) == child );
+  VERIFY( WIFEXITED(status) && WEXITSTATUS(status) == 0 );
 }
 
 int
===================================================================
Index: testsuite/27_io/basic_filebuf/open/char/9507.cc
--- testsuite/27_io/basic_filebuf/open/char/9507.cc	12 Jan 2004 08:11:07 -0000	1.4
+++ testsuite/27_io/basic_filebuf/open/char/9507.cc	3 Feb 2004 02:30:44 -0000
@@ -29,6 +29,7 @@
 #include <fcntl.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <sys/wait.h>
 #include <testsuite_hooks.h>
 
 // libstdc++/9507
@@ -42,20 +43,41 @@ void test_06()
 
   unlink(name);
   try_mkfifo(name, S_IRWXU);
-	
-  if (!fork())
+
+  pid_t pid = fork();
+  VERIFY( pid != -1 );
+
+  if (!pid)
     {
       std::filebuf fbuf;
+      // Opening a FIFO may block forever.
+      // Set an alarm to prevent this.
+      alarm(10);
+
       fbuf.open(name, std::ios_base::in);
       fbuf.sgetc();
       fbuf.close();
       exit(0);
     }
 
+  // Opening an FIFO may block forever.
+  // Set an alarm to prevent this.
+  alarm(10);
+
+  // There is no way to get fbuf.open to call open(2) with O_WRONLY and
+  // not O_TRUNC.  Passing O_TRUNC may cause the kernel to reject the
+  // open call.  So, we use in|out|ate, which maps to O_RDWR, which
+  // hopefully will not cause problems.
+
   std::filebuf fbuf;
-  std::filebuf* r = fbuf.open(name, std::ios_base::out | std::ios_base::ate);
+  std::filebuf* r = fbuf.open(name, std::ios_base::in | std::ios_base::out | std::ios_base::ate);
   VERIFY( !fbuf.is_open() );
   VERIFY( r == NULL );
+
+  // Make sure the child did its bit correctly.
+  int status;
+  VERIFY( wait(&status) == pid );
+  VERIFY( WIFEXITED(status) && WEXITSTATUS(status) == 0 );
 }
 
 int
@@ -64,5 +86,3 @@ main()
   test_06();
   return 0;
 }
-
-