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 <[email protected]> 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 <[email protected]> + + 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 <[email protected]> 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
