Re: Test 52 failure on AIX, HP-UX, Solaris
Joel E. Denny <jdenny <at> clemson.edu>
2010-02-02 09:30:12 GMT
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 <jdenny <at> clemson.edu>
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 <jdenny <at> ces.clemson.edu>
+
+ 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 <jdenny <at> ces.clemson.edu>
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
RSS Feed