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

[Joel E. Denny]

On Wed, 3 Feb 2010, Joel E. Denny wrote:

> I'm rewriting the patch, and I'll figure out what goes in what release 
> later....

The new patch is below.  It's simpler, so I feel more comfortable about 
including it in 2.4.2.  In a day or two, I'll probably push it to 
branch-2.4.2, branch-2.5, and master if there are no objections.

> > but I'm suspicious that 
> > there's a race condition

I'm now sure that there is a race condition.  Before the fix, when I run 
the new test group many times in a row, it occasionally succeeds.  
Hopefully I've managed to fix any occasional failures.

>From ccd30bf8ecd66b967959c2b970a1f83f9928f795 Mon Sep 17 00:00:00 2001
From: Joel E. Denny <address@hidden>
Date: Thu, 4 Feb 2010 05:18:44 -0500
Subject: [PATCH] portability: don't exit before draining m4 output pipe.

M4's output pipe was not being drained upon fatal errors during
scan_skel.  As a result, broken-pipe messages from M4 were seen
on at least AIX, HP-UX, Solaris, and RHEL4, and this caused a
failure in the test suite.  The problem was that, on platforms
where the default disposition for SIGPIPE is ignore instead of
terminate, M4 sometimes saw fwrite fail with errno=EPIPE and
then reported it.  However, there's some sort of race condition,
because the new test group occasionally succeeded.

Reported by Albert Chin at
<http://lists.gnu.org/archive/html/bug-bison/2010-02/msg00004.html>.

* NEWS (2.4.2): Document.
* src/complain.c, src/complain.h (fatal_at, fatal): Drain the
M4 output pipe, if any, before exiting.
* 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 (m4_pipe): New global variable.
(output_skeleton): Set m4_pipe to the pipe to which M4 writes.
Assert that scan_skel completely drains the pipe.
* src/output.h (m4_pipe): Extern it.
* src/scan-skel.l (at_directive_perform): Update
output_file_name_check invocation.
* tests/output.at (AT_CHECK_CONFLICTING_OUTPUT): Check that the
grammar file actually isn't overwritten.
(Conflicting output files: -o foo.y): Update expected output.
* tests/skeletons.at (Fatal errors but M4 continues producing
output): New test group.
---
 ChangeLog          |   40 ++++++++++++++++++++++++++++++++++++++++
 NEWS               |    6 ++++--
 src/complain.c     |    5 +++++
 src/files.c        |   47 +++++++++++++++++++++++++++++++++--------------
 src/files.h        |    2 +-
 src/output.c       |    9 ++++++++-
 src/output.h       |    4 ++++
 src/scan-skel.l    |   12 ++++++------
 tests/output.at    |    4 +++-
 tests/skeletons.at |   42 ++++++++++++++++++++++++++++++++++++++++++
 10 files changed, 146 insertions(+), 25 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index b54880b..1074ce4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,43 @@
+2010-02-04  Joel E. Denny  <address@hidden>
+
+       portability: don't exit before draining m4 output pipe.
+
+       M4's output pipe was not being drained upon fatal errors during
+       scan_skel.  As a result, broken-pipe messages from M4 were seen
+       on at least AIX, HP-UX, Solaris, and RHEL4, and this caused a
+       failure in the test suite.  The problem was that, on platforms
+       where the default disposition for SIGPIPE is ignore instead of
+       terminate, M4 sometimes saw fwrite fail with errno=EPIPE and
+       then reported it.  However, there's some sort of race condition,
+       because the new test group occasionally succeeded.
+
+       Reported by Albert Chin at
+       <http://lists.gnu.org/archive/html/bug-bison/2010-02/msg00004.html>.
+
+       * NEWS (2.4.2): Document.
+       * src/complain.c, src/complain.h (fatal_at, fatal): Drain the
+       M4 output pipe, if any, before exiting.
+       * 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 (m4_pipe): New global variable.
+       (output_skeleton): Set m4_pipe to the pipe to which M4 writes.
+       Assert that scan_skel completely drains the pipe.
+       * src/output.h (m4_pipe): Extern it.
+       * src/scan-skel.l (at_directive_perform): Update
+       output_file_name_check invocation.
+       * tests/output.at (AT_CHECK_CONFLICTING_OUTPUT): Check that the
+       grammar file actually isn't overwritten.
+       (Conflicting output files: -o foo.y): Update expected output.
+       * tests/skeletons.at (Fatal errors but M4 continues producing
+       output): New test group.
+
 2010-02-01  Joel E. Denny  <address@hidden>
 
        tests: link lib/libbison.a for gnulib.
diff --git a/NEWS b/NEWS
index 9b33d13..e209f6d 100644
--- a/NEWS
+++ b/NEWS
@@ -3,8 +3,10 @@ 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.  As part of those fixes, fatal Bison errors no longer cause M4
+   to report a broken pipe on the affected platforms.
 
 ** `%prec IDENTIFIER' requires IDENTIFIER to be defined separately.
 
diff --git a/src/complain.c b/src/complain.c
index d187624..9ea9fe3 100644
--- a/src/complain.c
+++ b/src/complain.c
@@ -27,6 +27,7 @@
 #include "complain.h"
 #include "files.h"
 #include "getargs.h"
+#include "output.h"
 
 bool complaint_issued;
 
@@ -128,6 +129,8 @@ void
 fatal_at (location loc, const char *message, ...)
 {
   ERROR_MESSAGE (&loc, _("fatal error"), message);
+  if (m4_pipe != NULL)
+    while (EOF != getc (m4_pipe)) ;
   exit (EXIT_FAILURE);
 }
 
@@ -135,5 +138,7 @@ void
 fatal (const char *message, ...)
 {
   ERROR_MESSAGE (NULL, _("fatal error"), message);
+  if (m4_pipe != NULL)
+    while (EOF != getc (m4_pipe)) ;
   exit (EXIT_FAILURE);
 }
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..a9a56b1 100644
--- a/src/output.c
+++ b/src/output.c
@@ -41,6 +41,8 @@
 #include "symtab.h"
 #include "tables.h"
 
+FILE *m4_pipe = NULL;
+
 # define ARRAY_CARDINALITY(Array) (sizeof (Array) / sizeof *(Array))
 
 static struct obstack format_obstack;
@@ -577,12 +579,17 @@ output_skeleton (void)
   /* Read and process m4's output.  */
   timevar_push (TV_M4);
   end_of_output_subpipe (pid, filter_fd);
-  in = fdopen (filter_fd[1], "r");
+  in = m4_pipe = fdopen (filter_fd[1], "r");
   if (! in)
     error (EXIT_FAILURE, get_errno (),
           "fdopen");
   scan_skel (in);
+  /* scan_skel should have read all of M4's output.  Otherwise, when we
+     close the pipe, we risk letting M4 report a broken-pipe to the
+     Bison user.  */
+  aver (feof (in));
   xfclose (in);
+  m4_pipe = NULL;
   reap_subpipe (pid, m4);
   timevar_pop (TV_M4);
 }
diff --git a/src/output.h b/src/output.h
index bf805f5..a5ac0fd 100644
--- a/src/output.h
+++ b/src/output.h
@@ -24,4 +24,8 @@
 void output (void);
 char const *compute_pkgdatadir (void);
 
+/* Before exiting, fatal errors must drain this pipe, unless it's NULL.
+   That avoids a broken-pipe complaint from M4 to the Bison user.  */
+extern FILE *m4_pipe;
+
 #endif /* !OUTPUT_H_ */
diff --git a/src/scan-skel.l b/src/scan-skel.l
index 90a52dd..cd30576 100644
--- a/src/scan-skel.l
+++ b/src/scan-skel.l
@@ -107,7 +107,7 @@ 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.  */
 
@@ -174,10 +174,10 @@ 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 void
+at_directive_perform (int at_directive_argc,
+                      char *at_directive_argv[],
+                      char **outnamep, int *out_linenop)
 {
   if (0 == strcmp (at_directive_argv[0], "@basename"))
     {
@@ -276,7 +276,7 @@ void at_directive_perform (int at_directive_argc,
           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;
     }
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)
 
 
diff --git a/tests/skeletons.at b/tests/skeletons.at
index f96d13e..18acbc0 100644
--- a/tests/skeletons.at
+++ b/tests/skeletons.at
@@ -288,3 +288,45 @@ foo.y:1.5-6: fatal error: M4 should exit immediately here
 ]])
 
 AT_CLEANUP
+
+
+## ------------------------------------------------ ##
+## Fatal errors but M4 continues producing output.  ##
+## ------------------------------------------------ ##
+
+# At one time, if Bison encountered a fatal error during M4 processing,
+# Bison failed to drain M4's output pipe.  The result was a SIGPIPE.
+# On some platforms, the default disposition for SIGPIPE is terminate,
+# which was fine.  On others, it's ignore, which caused M4 to report
+# the broken pipe to the user, but we don't want to bother the user with
+# that.
+
+# There is a race condition somewhere.  That is, before the associated
+# fix, running this test group many times in a row would occasionally
+# produce a pass among all the failures.
+
+AT_SETUP([[Fatal errors but M4 continues producing output]])
+
+AT_DATA([[gen-skel.pl]],
+[[use warnings;
+use strict;
+my $M4 = "m4";
+my $DNL = "d"."nl";
+print "${M4}_divert_push(0)$DNL\n";
+print '@output(@,@)', "\n";
+(print "garbage"x10, "\n") for (1..1000);
+print "${M4}_divert_pop(0)\n";
+]])
+AT_CHECK([[perl gen-skel.pl > skel.c || exit 77]])
+
+AT_DATA([[input.y]],
+[[%skeleton "./skel.c"
+%%
+start: ;
+]])
+
+AT_BISON_CHECK([[input.y]], [[1]], [[]],
+[[input.y: fatal error: too many arguments for @output directive in skeleton
+]])
+
+AT_CLEANUP
-- 
1.5.4.3