Re: Test 52 failure on AIX, HP-UX, Solaris

[Joel E. Denny]

On Tue, 2 Feb 2010, Albert Chin wrote:

> On Tue, Feb 02, 2010 at 01:19:38AM -0500, Joel E. Denny wrote:
> > While the following patch is obviously not a good solution, would you
> > please confirm that it does allow the test group to pass on the
> > affected platforms?
> 
> Confirmed on RHEL4/x86-64.

Thanks.

Here's an attempt at fixing this.  Not yet pushed.  I now need to decide 
whether this fix is too much for 2.4.2.  If so, I suppose I could just 
comment out the affected test group for 2.4.2, but I'm suspicious that 
there's a race condition that could sometimes affect other test groups.  
I'd appreciate comments from anyone.

>From 09f99a629f7aa0d696646d28d23cde47588f4e38 Mon Sep 17 00:00:00 2001
From: Joel E. Denny <address@hidden>
Date: Tue, 2 Feb 2010 04:10:30 -0500
Subject: [PATCH] portability: don't exit before reaping m4 child.

This was happening at fatal errors during scan_skel.  Broken
pipe messages were seen on at least AIX, HP-UX, Solaris, and
RHEL4, and this caused failures in the test suite.  Is this more
a race condition than a portability issue?
Reported by Albert Chin at
<http://lists.gnu.org/archive/html/bug-bison/2010-02/msg00004.html>.
* NEWS (2.4.2): Document.
* lib/subpipe.c, lib/subpipe.h (reap_subpipe): Add
ignore_failure argument to avoid a redundant error message after
already reporting a fatal error.
* src/complain.c, src/complain.h (fatal_at_delayed,
fatal_delayed): New functions.
* src/files.c (compute_output_file_names): Update invocations
of output_file_name_check.
(output_file_name_check): In the case that the grammar file
would be overwritten, use complain instead of fatal, but replace
the output file name with /dev/null.  Use the /dev/null solution
for case of two conflicting output files as well because it
seems safer in case Bison one day tries to open both files at
the same time.
* src/files.h (output_file_name_check): Update prototype.
* src/output.c (output_skeleton): On a fatal error from
scan_skel, reap m4 process before exiting.
* src/scan-skel.h (scan_skel): Update prototype.
* src/scan-skel.l (skel_lex): Delay fatal errors and return
false iff there were any.
(scan_skel): Propagate skel_lex's return.
(at_directive_perform): Delay fatal errors and return false iff
there were any.  Update output_file_name_check invocation.
(fail_for_at_directive_too_few_args): Delay fatal error.
(fail_for_at_directive_too_many_args): Delay fatal error.
(fail_for_invalid_at): Delay fatal error.
* output.at (AT_CHECK_CONFLICTING_OUTPUT): Check that the
grammar file actually isn't overwritten.
(Conflicting output files: -o foo.y): Update expected output.
---
 ChangeLog       |   39 ++++++++++++++++++++++++++
 NEWS            |    5 ++-
 lib/subpipe.c   |    4 +-
 lib/subpipe.h   |    2 +-
 src/complain.c  |   12 ++++++++
 src/complain.h  |    8 +++++
 src/files.c     |   47 ++++++++++++++++++++++---------
 src/files.h     |    2 +-
 src/output.c    |   10 +++++--
 src/scan-skel.h |    2 +-
 src/scan-skel.l |   81 ++++++++++++++++++++++++++++++++++++-------------------
 tests/output.at |    4 ++-
 12 files changed, 163 insertions(+), 53 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index b54880b..8d665b7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,42 @@
+2010-02-02  Joel E. Denny  <address@hidden>
+
+       portability: don't exit before reaping m4 child.
+       This was happening at fatal errors during scan_skel.  Broken
+       pipe messages were seen on at least AIX, HP-UX, Solaris, and
+       RHEL4, and this caused failures in the test suite.  Is this more
+       a race condition than a portability issue?
+       Reported by Albert Chin at
+       <http://lists.gnu.org/archive/html/bug-bison/2010-02/msg00004.html>.
+       * NEWS (2.4.2): Document.
+       * lib/subpipe.c, lib/subpipe.h (reap_subpipe): Add
+       ignore_failure argument to avoid a redundant error message after
+       already reporting a fatal error.
+       * src/complain.c, src/complain.h (fatal_at_delayed,
+       fatal_delayed): New functions.
+       * src/files.c (compute_output_file_names): Update invocations
+       of output_file_name_check.
+       (output_file_name_check): In the case that the grammar file
+       would be overwritten, use complain instead of fatal, but replace
+       the output file name with /dev/null.  Use the /dev/null solution
+       for case of two conflicting output files as well because it
+       seems safer in case Bison one day tries to open both files at
+       the same time.
+       * src/files.h (output_file_name_check): Update prototype.
+       * src/output.c (output_skeleton): On a fatal error from
+       scan_skel, reap m4 process before exiting.
+       * src/scan-skel.h (scan_skel): Update prototype.
+       * src/scan-skel.l (skel_lex): Delay fatal errors and return
+       false iff there were any.
+       (scan_skel): Propagate skel_lex's return.
+       (at_directive_perform): Delay fatal errors and return false iff
+       there were any.  Update output_file_name_check invocation.
+       (fail_for_at_directive_too_few_args): Delay fatal error.
+       (fail_for_at_directive_too_many_args): Delay fatal error.
+       (fail_for_invalid_at): Delay fatal error.
+       * output.at (AT_CHECK_CONFLICTING_OUTPUT): Check that the
+       grammar file actually isn't overwritten.
+       (Conflicting output files: -o foo.y): Update expected output.
+
 2010-02-01  Joel E. Denny  <address@hidden>
 
        tests: link lib/libbison.a for gnulib.
diff --git a/NEWS b/NEWS
index 9b33d13..8f33793 100644
--- a/NEWS
+++ b/NEWS
@@ -3,8 +3,9 @@ Bison News
 
 * Changes in version 2.4.2 (????-??-??):
 
-** Some portability problems in the testsuite that resulted in failures
-   on at least Solaris 2.7 have been fixed.
+** Some portability problems that resulted in testsuite failures on
+   some versions of at least Solaris, AIX, HP-UX, and RHEL4 have been
+   fixed.
 
 ** `%prec IDENTIFIER' requires IDENTIFIER to be defined separately.
 
diff --git a/lib/subpipe.c b/lib/subpipe.c
index 1b7c36d..cdbd353 100644
--- a/lib/subpipe.c
+++ b/lib/subpipe.c
@@ -147,13 +147,13 @@ create_subpipe (char const * const *argv, int fd[2])
 /* Wait for the subprocess to exit.  */
 
 void
-reap_subpipe (pid_t pid, char const *program)
+reap_subpipe (pid_t pid, char const *program, int ignore_failure)
 {
 #if HAVE_WAITPID || defined waitpid
   int wstatus;
   if (waitpid (pid, &wstatus, 0) < 0)
     error (EXIT_FAILURE, errno, "waitpid");
-  else
+  else if (!ignore_failure)
     {
       int status = WIFEXITED (wstatus) ? WEXITSTATUS (wstatus) : -1;
       if (status)
diff --git a/lib/subpipe.h b/lib/subpipe.h
index ff791a8..708a447 100644
--- a/lib/subpipe.h
+++ b/lib/subpipe.h
@@ -25,4 +25,4 @@
 void init_subpipe (void);
 pid_t create_subpipe (char const * const *, int[2]);
 void end_of_output_subpipe (pid_t, int[2]);
-void reap_subpipe (pid_t, char const *);
+void reap_subpipe (pid_t, char const *, int);
diff --git a/src/complain.c b/src/complain.c
index d187624..7b53aaa 100644
--- a/src/complain.c
+++ b/src/complain.c
@@ -137,3 +137,15 @@ fatal (const char *message, ...)
   ERROR_MESSAGE (NULL, _("fatal error"), message);
   exit (EXIT_FAILURE);
 }
+
+void
+fatal_at_delayed (location loc, const char *message, ...)
+{
+  ERROR_MESSAGE (&loc, _("fatal error"), message);
+}
+
+void
+fatal_delayed (const char *message, ...)
+{
+  ERROR_MESSAGE (NULL, _("fatal error"), message);
+}
diff --git a/src/complain.h b/src/complain.h
index 9d24f25..29120cd 100644
--- a/src/complain.h
+++ b/src/complain.h
@@ -48,6 +48,14 @@ void fatal (char const *format, ...)
 void fatal_at (location loc, char const *format, ...)
   __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3)));
 
+/** A fatal error, but assume the caller will immediately exit.  */
+
+void fatal_delayed (char const *format, ...)
+  __attribute__ ((__format__ (__printf__, 1, 2)));
+
+void fatal_at_delayed (location loc, char const *format, ...)
+  __attribute__ ((__format__ (__printf__, 2, 3)));
+
 /** Whether an error was reported.  */
 extern bool complaint_issued;
 
diff --git a/src/files.c b/src/files.c
index 9761de9..e8bb200 100644
--- a/src/files.c
+++ b/src/files.c
@@ -328,21 +328,21 @@ compute_output_file_names (void)
     {
       if (! spec_graph_file)
        spec_graph_file = concat2 (all_but_tab_ext, ".dot");
-      output_file_name_check (spec_graph_file);
+      output_file_name_check (&spec_graph_file);
     }
 
   if (xml_flag)
     {
       if (! spec_xml_file)
        spec_xml_file = concat2 (all_but_tab_ext, ".xml");
-      output_file_name_check (spec_xml_file);
+      output_file_name_check (&spec_xml_file);
     }
 
   if (report_flag)
     {
       if (!spec_verbose_file)
         spec_verbose_file = concat2 (all_but_tab_ext, OUTPUT_EXT);
-      output_file_name_check (spec_verbose_file);
+      output_file_name_check (&spec_verbose_file);
     }
 
   free (all_but_tab_ext);
@@ -351,18 +351,37 @@ compute_output_file_names (void)
 }
 
 void
-output_file_name_check (char const *file_name)
+output_file_name_check (char **file_name)
 {
-  if (0 == strcmp (file_name, grammar_file))
-    fatal (_("refusing to overwrite the input file %s"), quote (file_name));
-  {
-    int i;
-    for (i = 0; i < file_names_count; i++)
-      if (0 == strcmp (file_names[i], file_name))
-        warn (_("conflicting outputs to file %s"), quote (file_name));
-  }
-  file_names = xnrealloc (file_names, ++file_names_count, sizeof *file_names);
-  file_names[file_names_count-1] = xstrdup (file_name);
+  bool conflict = false;
+  if (0 == strcmp (*file_name, grammar_file))
+    {
+      complain (_("refusing to overwrite the input file %s"),
+                quote (*file_name));
+      conflict = true;
+    }
+  else
+    {
+      int i;
+      for (i = 0; i < file_names_count; i++)
+        if (0 == strcmp (file_names[i], *file_name))
+          {
+            warn (_("conflicting outputs to file %s"),
+                  quote (*file_name));
+            conflict = true;
+          }
+    }
+  if (conflict)
+    {
+      free (*file_name);
+      *file_name = strdup ("/dev/null");
+    }
+  else
+    {
+      file_names = xnrealloc (file_names, ++file_names_count,
+                              sizeof *file_names);
+      file_names[file_names_count-1] = xstrdup (*file_name);
+    }
 }
 
 void
diff --git a/src/files.h b/src/files.h
index e8f28bf..5366783 100644
--- a/src/files.h
+++ b/src/files.h
@@ -63,7 +63,7 @@ extern char *all_but_ext;
 
 void compute_output_file_names (void);
 void output_file_names_free (void);
-void output_file_name_check (char const *file_name);
+void output_file_name_check (char **file_name);
 
 FILE *xfopen (const char *name, const char *mode);
 void xfclose (FILE *ptr);
diff --git a/src/output.c b/src/output.c
index 6a05704..b5d661d 100644
--- a/src/output.c
+++ b/src/output.c
@@ -581,9 +581,13 @@ output_skeleton (void)
   if (! in)
     error (EXIT_FAILURE, get_errno (),
           "fdopen");
-  scan_skel (in);
-  xfclose (in);
-  reap_subpipe (pid, m4);
+  {
+    bool fatal_error = !scan_skel (in);
+    xfclose (in);
+    reap_subpipe (pid, m4, fatal_error);
+    if (fatal_error)
+      exit (EXIT_FAILURE);
+  }
   timevar_pop (TV_M4);
 }
 
diff --git a/src/scan-skel.h b/src/scan-skel.h
index bef1a33..1cb2138 100644
--- a/src/scan-skel.h
+++ b/src/scan-skel.h
@@ -17,7 +17,7 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-void scan_skel (FILE *);
+bool scan_skel (FILE *);
 
 /* Pacify "make syntax-check".  */
 extern FILE *skel_in;
diff --git a/src/scan-skel.l b/src/scan-skel.l
index 90a52dd..bd91cbd 100644
--- a/src/scan-skel.l
+++ b/src/scan-skel.l
@@ -38,13 +38,13 @@
 #include "files.h"
 #include "scan-skel.h"
 
-#define YY_DECL static int skel_lex (void)
+#define YY_DECL static bool skel_lex (void)
 YY_DECL;
 
 #define QPUTS(String) \
    fputs (quotearg_style (c_quoting_style, String), yyout)
 
-static void at_directive_perform (int at_directive_argc,
+static bool at_directive_perform (int at_directive_argc,
                                   char *at_directive_argv[],
                                   char **outnamep, int *out_linenop);
 static void fail_for_at_directive_too_many_args (char const 
*at_directive_name);
@@ -88,7 +88,7 @@ static void fail_for_invalid_at (char const *at);
 }
 
   /* This pattern must not match more than the previous @ patterns. */
address@hidden@{}`(\n]* fail_for_invalid_at (yytext);
address@hidden@{}`(\n]* fail_for_invalid_at (yytext); return false;
 \n        out_lineno++; ECHO;
 address@hidden    ECHO;
 
@@ -98,7 +98,7 @@ static void fail_for_invalid_at (char const *at);
       free (outname);
       xfclose (yyout);
     }
-  return EOF;
+  return true;
 }
 
 <SC_AT_DIRECTIVE_ARGS>{
@@ -107,13 +107,16 @@ static void fail_for_invalid_at (char const *at);
   "@@" { obstack_1grow (&obstack_for_string, '@'); }
   "@{" { obstack_1grow (&obstack_for_string, '['); }
   "@}" { obstack_1grow (&obstack_for_string, ']'); }
-  "@`" /* Emtpy.  Useful for starting an argument
+  "@`" /* Empty.  Useful for starting an argument
           that begins with whitespace. */
   @\n  /* Empty.  */
 
   @[,)] {
     if (at_directive_argc >= AT_DIRECTIVE_ARGC_MAX)
-      fail_for_at_directive_too_many_args (at_directive_argv[0]);
+      {
+        fail_for_at_directive_too_many_args (at_directive_argv[0]);
+        return false;
+      }
 
     obstack_1grow (&obstack_for_string, '\0');
     at_directive_argv[at_directive_argc++] =
@@ -124,15 +127,16 @@ static void fail_for_invalid_at (char const *at);
       BEGIN SC_AT_DIRECTIVE_SKIP_WS;
     else
       {
-        at_directive_perform (at_directive_argc, at_directive_argv,
-                              &outname, &out_lineno);
+        if (!at_directive_perform (at_directive_argc, at_directive_argv,
+                                   &outname, &out_lineno))
+          return false;
         obstack_free (&obstack_for_string, at_directive_argv[0]);
         at_directive_argc = 0;
         BEGIN INITIAL;
       }
   }
 
-  @.? { fail_for_invalid_at (yytext); }
+  @.? { fail_for_invalid_at (yytext); return false; }
 }
 
 <SC_AT_DIRECTIVE_SKIP_WS>{
@@ -142,7 +146,9 @@ static void fail_for_invalid_at (char const *at);
 
 <SC_AT_DIRECTIVE_ARGS,SC_AT_DIRECTIVE_SKIP_WS>{
   <<EOF>> {
-    fatal (_("unclosed %s directive in skeleton"), at_directive_argv[0]);
+    fatal_delayed (_("unclosed %s directive in skeleton"),
+                   at_directive_argv[0]);
+    return false;
   }
 }
 
@@ -152,7 +158,7 @@ static void fail_for_invalid_at (char const *at);
 | Scan a Bison skeleton.  |
 `------------------------*/
 
-void
+bool
 scan_skel (FILE *in)
 {
   static bool initialized = false;
@@ -163,7 +169,7 @@ scan_skel (FILE *in)
     }
   skel_in = in;
   skel__flex_debug = trace_flag & trace_skeleton;
-  skel_lex ();
+  return skel_lex ();
 }
 
 void
@@ -174,15 +180,18 @@ skel_scanner_free (void)
   yylex_destroy ();
 }
 
-static
-void at_directive_perform (int at_directive_argc,
-                           char *at_directive_argv[],
-                           char **outnamep, int *out_linenop)
+static bool
+at_directive_perform (int at_directive_argc,
+                      char *at_directive_argv[],
+                      char **outnamep, int *out_linenop)
 {
   if (0 == strcmp (at_directive_argv[0], "@basename"))
     {
       if (at_directive_argc > 2)
-        fail_for_at_directive_too_many_args (at_directive_argv[0]);
+        {
+          fail_for_at_directive_too_many_args (at_directive_argv[0]);
+          return false;
+        }
       fputs (last_component (at_directive_argv[1]), yyout);
     }
   else if (0 == strcmp (at_directive_argv[0], "@warn")
@@ -194,7 +203,7 @@ void at_directive_perform (int at_directive_argc,
         {
           case 'w': func = warn; break;
           case 'c': func = complain; break;
-          case 'f': func = fatal; break;
+          case 'f': func = fatal_delayed; break;
           default: aver (false); break;
         }
       switch (at_directive_argc)
@@ -220,8 +229,11 @@ void at_directive_perform (int at_directive_argc,
             break;
           default:
             fail_for_at_directive_too_many_args (at_directive_argv[0]);
+            return false;
             break;
         }
+      if (func == fatal_delayed)
+        return false;
     }
   else if (0 == strcmp (at_directive_argv[0], "@warn_at")
            || 0 == strcmp (at_directive_argv[0], "@complain_at")
@@ -230,12 +242,15 @@ void at_directive_perform (int at_directive_argc,
       void (*func)(location, char const *, ...);
       location loc;
       if (at_directive_argc < 4)
-        fail_for_at_directive_too_few_args (at_directive_argv[0]);
+        {
+          fail_for_at_directive_too_few_args (at_directive_argv[0]);
+          return false;
+        }
       switch (at_directive_argv[0][1])
         {
           case 'w': func = warn_at; break;
           case 'c': func = complain_at; break;
-          case 'f': func = fatal_at; break;
+          case 'f': func = fatal_at_delayed; break;
           default: aver (false); break;
         }
       boundary_set_from_string (&loc.start, at_directive_argv[1]);
@@ -263,43 +278,53 @@ void at_directive_perform (int at_directive_argc,
             break;
           default:
             fail_for_at_directive_too_many_args (at_directive_argv[0]);
+            return false;
             break;
         }
+      if (func == fatal_at_delayed)
+        return false;
     }
   else if (0 == strcmp (at_directive_argv[0], "@output"))
     {
       if (at_directive_argc > 2)
-        fail_for_at_directive_too_many_args (at_directive_argv[0]);
+        {
+          fail_for_at_directive_too_many_args (at_directive_argv[0]);
+          return false;
+        }
       if (*outnamep)
         {
           free (*outnamep);
           xfclose (yyout);
         }
       *outnamep = xstrdup (at_directive_argv[1]);
-      output_file_name_check (*outnamep);
+      output_file_name_check (outnamep);
       yyout = xfopen (*outnamep, "w");
       *out_linenop = 1;
     }
   else
-    fail_for_invalid_at (at_directive_argv[0]);
+    {
+      fail_for_invalid_at (at_directive_argv[0]);
+      return false;
+    }
+  return true;
 }
 
 static void
 fail_for_at_directive_too_few_args (char const *at_directive_name)
 {
-  fatal (_("too few arguments for %s directive in skeleton"),
-         at_directive_name);
+  fatal_delayed (_("too few arguments for %s directive in skeleton"),
+                 at_directive_name);
 }
 
 static void
 fail_for_at_directive_too_many_args (char const *at_directive_name)
 {
-  fatal (_("too many arguments for %s directive in skeleton"),
-         at_directive_name);
+  fatal_delayed (_("too many arguments for %s directive in skeleton"),
+                 at_directive_name);
 }
 
 static void
 fail_for_invalid_at (char const *at)
 {
-  fatal ("invalid @ in skeleton: %s", at);
+  fatal_delayed ("invalid @ in skeleton: %s", at);
 }
diff --git a/tests/output.at b/tests/output.at
index f8e1653..999ca18 100644
--- a/tests/output.at
+++ b/tests/output.at
@@ -137,7 +137,9 @@ AT_DATA([$1],
 foo: {};
 ]])
 
+[cp ]$1[ expout]
 AT_BISON_CHECK([$3 $1], $5, [], [$4])
+AT_CHECK([[cat $1]], [[0]], [expout])
 AT_CLEANUP
 ])
 
@@ -157,7 +159,7 @@ AT_CHECK_CONFLICTING_OUTPUT([foo.y],
 ])
 
 AT_CHECK_CONFLICTING_OUTPUT([foo.y], [], [-o foo.y],
-[foo.y: fatal error: refusing to overwrite the input file `foo.y'
+[foo.y: refusing to overwrite the input file `foo.y'
 ], 1)
 
 
-- 
1.5.4.3