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 <[email protected]> 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 <[email protected]> + + 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 <[email protected]> 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. */ -...@[^@{}`(\n]* fail_for_invalid_at (yytext); +...@[^@{}`(\n]* fail_for_invalid_at (yytext); return false; \n out_lineno++; ECHO; [...@\n]+ 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
