[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Test 52 failure on AIX, HP-UX, Solaris
From: |
Joel E. Denny |
Subject: |
Re: Test 52 failure on AIX, HP-UX, Solaris |
Date: |
Thu, 4 Feb 2010 12:19:07 -0500 (EST) |
User-agent: |
Alpine 1.00 (DEB 882 2007-12-20) |
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
- Re: Test 52 failure on AIX, HP-UX, Solaris, Joel E. Denny, 2010/02/02
- Re: Test 52 failure on AIX, HP-UX, Solaris, Akim Demaille, 2010/02/03
- Re: Test 52 failure on AIX, HP-UX, Solaris, Joel E. Denny, 2010/02/03
- Re: Test 52 failure on AIX, HP-UX, Solaris,
Joel E. Denny <=
- Re: Test 52 failure on AIX, HP-UX, Solaris, Joel E. Denny, 2010/02/04
- Re: Test 52 failure on AIX, HP-UX, Solaris, Juan Manuel Guerrero, 2010/02/06
- Re: Test 52 failure on AIX, HP-UX, Solaris, Joel E. Denny, 2010/02/06
- Re: Test 52 failure on AIX, HP-UX, Solaris, Joel E. Denny, 2010/02/22