summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPaul Smith <psmith@gnu.org>2013-05-04 13:10:56 -0400
committerPaul Smith <psmith@gnu.org>2013-05-04 13:10:56 -0400
commitb8467292c9be14b31c028d472107946770ed977e (patch)
tree83a4a38f0209303e73e93d03218528e1ba07c366
parentcb2be0db36b9730d9879dec4332141ee4db9377f (diff)
downloadgunmake-b8467292c9be14b31c028d472107946770ed977e.tar.gz
Improve sync handling for -Ojob/-Otarget and recursion.
If we are not going to sync a command line then dump any collected output first to preserve ordering. Do some code cleanup: * Move the handle init to a separate function. * Move the temp file truncation to the output function. * Remember whether we sync in a variable for readability. * Handle EINTR and short writes in child_out(). * Always call sync_output() in case output_sync was changed due to error.
-rw-r--r--ChangeLog13
-rw-r--r--job.c267
-rw-r--r--misc.c19
-rw-r--r--tests/ChangeLog5
-rw-r--r--tests/scripts/features/output-sync10
5 files changed, 183 insertions, 131 deletions
diff --git a/ChangeLog b/ChangeLog
index 9b347f6..498e127 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2013-05-04 Paul Smith <psmith@gnu.org>
+
+ * job.c (child_out): Handle EINTR and incomplete write scenarios.
+ (sync_init): New function: separate the initialization code.
+ (assign_child_tempfiles): Remove truncation from this function,
+ (sync_output): and add it here after output is generated.
+ (reap_children): Always call sync_output() in case output_sync was
+ reset after the child started, due to error.
+ (start_job_command): Create new sync_cmd variable. Use new method
+ for initializing the handle.
+ If we're not syncing the output be sure any output we've saved is
+ dumped immediately before starting the child.
+
2013-05-04 Eli Zaretskii <eliz@gnu.org>
* job.c (start_job_command): Make the condition for creating a
diff --git a/job.c b/job.c
index 548f21a..343d35c 100644
--- a/job.c
+++ b/job.c
@@ -244,9 +244,12 @@ unsigned long job_counter = 0;
unsigned int jobserver_tokens = 0;
#ifdef OUTPUT_SYNC
+
/* Semaphore for use in -j mode with output_sync. */
+static sync_handle_t sync_handle = -1;
-sync_handle_t sync_handle = -1;
+/* Is make's stdout going to the same place as stderr? */
+static int combined_output = 0;
#define STREAM_OK(_s) ((fcntl (fileno (_s), F_GETFD) != -1) || (errno != EBADF))
@@ -467,7 +470,7 @@ is_bourne_compatible_shell (const char *path)
}
-/* Write a message relating to a child. Write it to the child's output
+/* Write a message in the child's context. Write it to the child's output
sync file if present, otherwise to the terminal. */
static void
@@ -477,16 +480,27 @@ child_out (const struct child *child, const char *msg, int out)
if (outfd >= 0)
{
+ int t = strlen (msg);
+ int l;
lseek (outfd, 0, SEEK_END);
- write (outfd, msg, strlen (msg));
- write (outfd, "\n", 1);
+ while (t)
+ {
+ EINTRLOOP (l, write (outfd, msg, t));
+ if (l == t)
+ break;
+ if (l < 0)
+ {
+ perror ("write()");
+ break;
+ }
+ t -= l;
+ msg += l;
+ }
}
else
{
FILE *outf = out ? stdout : stderr;
-
fputs (msg, outf);
- putc ('\n', outf);
fflush (outf);
}
}
@@ -503,7 +517,7 @@ child_error (const struct child *child,
const char *post = "";
const char *dump = "";
const struct file *f = child->file;
- gmk_floc *flocp = &f->cmds->fileinfo;
+ const gmk_floc *flocp = &f->cmds->fileinfo;
const char *msg;
const char *nm;
unsigned int l;
@@ -529,8 +543,8 @@ child_error (const struct child *child,
nm = a;
}
- msg = message_s (strlen (nm) + strlen (f->name),
- 0, _("%s: recipe for target '%s' failed"), nm, f->name);
+ msg = message_s (strlen (nm) + strlen (f->name), 0,
+ _("%s: recipe for target '%s' failed"), nm, f->name);
child_out (child, msg, 1);
l = strlen (pre) + strlen (f->name) + strlen (post);
@@ -539,17 +553,17 @@ child_error (const struct child *child,
if (exit_code & 1 != 0)
return;
- msg = error_s (l + INTEGER_LENGTH, NILF, _("%s[%s] Error 0x%x%s"),
- pre, f->name, exit_code, post);
+ msg = error_s (l + INTEGER_LENGTH, NILF,
+ _("%s[%s] Error 0x%x%s"), pre, f->name, exit_code, post);
#else
if (exit_sig == 0)
- msg = error_s (l + INTEGER_LENGTH, NILF, _("%s[%s] Error %d%s"),
- pre, f->name, exit_code, post);
+ msg = error_s (l + INTEGER_LENGTH, NILF,
+ _("%s[%s] Error %d%s"), pre, f->name, exit_code, post);
else
{
const char *s = strsignal (exit_sig);
- msg = error_s (l + strlen (s) + strlen (dump),
- NILF, _("%s[%s] %s%s%s"), pre, f->name, s, dump, post);
+ msg = error_s (l + strlen (s) + strlen (dump), NILF,
+ _("%s[%s] %s%s%s"), pre, f->name, s, dump, post);
}
#endif /* VMS */
@@ -591,52 +605,85 @@ child_handler (int sig UNUSED)
}
#ifdef OUTPUT_SYNC
-/* Adds file descriptors to the child structure to support output_sync;
- one for stdout and one for stderr as long as they are (a) open and
- (b) separate. If stdout and stderr share a device they can share a
- temp file too. */
-static int
-assign_child_tempfiles (struct child *c, int combined)
+
+/* Set up the sync handle and configure combined_output.
+ Disables output_sync on error. */
+static void
+sync_init ()
{
- /* If we don't have a temp file, get one. */
- if (c->outfd < 0 && c->errfd < 0)
+#ifdef WINDOWS32
+ if ((!STREAM_OK (stdout) && !STREAM_OK (stderr))
+ || (sync_handle = create_mutex ()) == -1)
{
- if (STREAM_OK (stdout))
- {
- c->outfd = open_tmpfd ();
- CLOSE_ON_EXEC (c->outfd);
- }
+ perror_with_name ("output-sync suppressed: ", "stderr");
+ output_sync = 0;
+ }
+ else
+ {
+ combined_output = same_stream (stdout, stderr);
+ prepare_mutex_handle_string (sync_handle);
+ }
- if (STREAM_OK (stderr))
- {
- if (c->outfd >= 0 && combined)
- c->errfd = c->outfd;
- else
- {
- c->errfd = open_tmpfd ();
- CLOSE_ON_EXEC (c->errfd);
- }
- }
+#else
+ if (STREAM_OK (stdout))
+ {
+ struct stat stbuf_o, stbuf_e;
+
+ sync_handle = fileno (stdout);
+ combined_output =
+ fstat (fileno (stdout), &stbuf_o) == 0 &&
+ fstat (fileno (stderr), &stbuf_e) == 0 &&
+ stbuf_o.st_dev == stbuf_e.st_dev &&
+ stbuf_o.st_ino == stbuf_e.st_ino;
+ }
+ else if (STREAM_OK (stderr))
+ sync_handle = fileno (stderr);
+ else
+ {
+ perror_with_name ("output-sync suppressed: ", "stderr");
+ output_sync = 0;
+ }
+#endif
+}
- return 1;
+/* Adds file descriptors to the child structure to support output_sync; one
+ for stdout and one for stderr as long as they are open. If stdout and
+ stderr share a device they can share a temp file too.
+ Will reset output_sync on error. */
+static void
+assign_child_tempfiles (struct child *c)
+{
+ /* If we have a temp file, we're done. */
+ if (c->outfd >= 0 || c->errfd >= 0)
+ return;
+
+ if (STREAM_OK (stdout))
+ {
+ c->outfd = open_tmpfd ();
+ if (c->outfd < 0)
+ goto error;
+ CLOSE_ON_EXEC (c->outfd);
}
- /* We already have a temp file. On per-job output, truncate and reset. */
- if (output_sync == OUTPUT_SYNC_JOB)
+ if (STREAM_OK (stderr))
{
- if (c->outfd >= 0)
+ if (c->outfd >= 0 && combined_output)
+ c->errfd = c->outfd;
+ else
{
- lseek(c->outfd, 0, SEEK_SET);
- ftruncate(c->outfd, 0);
- }
- if (c->errfd >= 0 && c->errfd != c->outfd)
- {
- lseek(c->errfd, 0, SEEK_SET);
- ftruncate(c->outfd, 0);
+ c->errfd = open_tmpfd ();
+ if (c->errfd < 0)
+ goto error;
+ CLOSE_ON_EXEC (c->errfd);
}
}
- return 1;
+ return;
+
+ error:
+ if (c->outfd >= 0)
+ close (c->outfd);
+ output_sync = 0;
}
/* Support routine for sync_output() */
@@ -750,6 +797,20 @@ sync_output (struct child *c)
/* Exit the critical section. */
if (sem)
release_semaphore (sem);
+
+ /* Truncate and reset the output, in case we use it again. */
+ if (c->outfd >= 0)
+ {
+ int e;
+ lseek(c->outfd, 0, SEEK_SET);
+ EINTRLOOP (e, ftruncate(c->outfd, 0));
+ }
+ if (c->errfd >= 0 && c->errfd != c->outfd)
+ {
+ int e;
+ lseek(c->errfd, 0, SEEK_SET);
+ EINTRLOOP (e, ftruncate(c->errfd, 0));
+ }
}
}
#endif /* OUTPUT_SYNC */
@@ -1095,7 +1156,8 @@ reap_children (int block, int err)
else
{
#ifdef OUTPUT_SYNC
- /* If we're sync'ing per job, write it now. */
+ /* If we're sync'ing per line, write the previous line's
+ output before starting the next one. */
if (output_sync == OUTPUT_SYNC_JOB)
sync_output (c);
#endif
@@ -1130,9 +1192,8 @@ reap_children (int block, int err)
/* When we get here, all the commands for c->file are finished. */
#ifdef OUTPUT_SYNC
- /* Synchronize parallel output if requested */
- if (output_sync)
- sync_output (c);
+ /* Synchronize any remaining parallel output. */
+ sync_output (c);
#endif /* OUTPUT_SYNC */
/* At this point c->file->update_status contains 0 or 2. But
@@ -1334,6 +1395,7 @@ start_job_command (struct child *child)
#if !defined(_AMIGA) && !defined(WINDOWS32)
static int bad_stdin = -1;
#endif
+ int sync_cmd = 0;
char *p;
/* Must be volatile to silence bogus GCC warning about longjmp/vfork. */
volatile int flags;
@@ -1521,6 +1583,11 @@ start_job_command (struct child *child)
fflush (stdout);
fflush (stderr);
+ /* Are we going to synchronize this command's output? Do so if either we're
+ in SYNC_MAKE mode or this command is not recursive. We'll also check
+ output_sync separately below in case it changes due to error. */
+ sync_cmd = output_sync == OUTPUT_SYNC_MAKE || !(flags & COMMANDS_RECURSE);
+
#ifndef VMS
#if !defined(WINDOWS32) && !defined(_AMIGA) && !defined(__MSDOS__)
@@ -1646,42 +1713,16 @@ start_job_command (struct child *child)
#else /* !__EMX__ */
#ifdef OUTPUT_SYNC
- if (output_sync)
- {
- static int combined_output;
- /* If output_sync is turned on, find a resource to
- synchronize on. This block is traversed only once. */
- if (sync_handle == -1)
- {
- if (STREAM_OK (stdout))
- {
- struct stat stbuf_o, stbuf_e;
-
- sync_handle = fileno (stdout);
- combined_output =
- fstat (fileno (stdout), &stbuf_o) == 0 &&
- fstat (fileno (stderr), &stbuf_e) == 0 &&
- stbuf_o.st_dev == stbuf_e.st_dev &&
- stbuf_o.st_ino == stbuf_e.st_ino;
- }
- else if (STREAM_OK (stderr))
- sync_handle = fileno (stderr);
- else
- {
- perror_with_name ("output-sync suppressed: ", "stderr");
- output_sync = 0;
- }
- }
+ if (output_sync && sync_handle == -1)
+ sync_init();
- /* If it still looks like we can synchronize, create a temp
- file to hold stdout (and one for stderr if separate). */
- if (output_sync == OUTPUT_SYNC_MAKE
- || (output_sync && !(flags & COMMANDS_RECURSE)))
- {
- if (!assign_child_tempfiles (child, combined_output))
- output_sync = 0;
- }
- }
+ if (output_sync && sync_cmd)
+ /* If we still want to sync, make sure we have temp files. */
+ assign_child_tempfiles (child);
+ else
+ /* We don't want to sync this command: to avoid misordered
+ output ensure any already-synched content is written. */
+ sync_output (child);
#endif /* OUTPUT_SYNC */
child->pid = vfork ();
@@ -1710,8 +1751,7 @@ start_job_command (struct child *child)
#ifdef OUTPUT_SYNC
/* Divert child output if output_sync in use. Don't capture
recursive make output unless we are synchronizing "make" mode. */
- if (output_sync && (output_sync == OUTPUT_SYNC_MAKE
- || !(flags & COMMANDS_RECURSE)))
+ if (output_sync && sync_cmd)
{
int outfd = fileno (stdout);
int errfd = fileno (stderr);
@@ -1812,38 +1852,16 @@ start_job_command (struct child *child)
char* arg0;
#ifdef OUTPUT_SYNC
- if (output_sync)
- {
- static int combined_output;
- /* If output_sync is turned on, create a mutex to
- synchronize on. This is done only once. */
- if (sync_handle == -1)
- {
- if ((!STREAM_OK (stdout) && !STREAM_OK (stderr))
- || (sync_handle = create_mutex ()) == -1)
- {
- perror_with_name ("output-sync suppressed: ", "stderr");
- output_sync = 0;
- }
- else
- {
- combined_output = same_stream (stdout, stderr);
- prepare_mutex_handle_string (sync_handle);
- }
- }
- /* If we can synchronize, create a temporary file to hold
- child's stdout, and another one for its stderr, if they
- are separate. */
- if (output_sync == OUTPUT_SYNC_MAKE
- || (output_sync && !(flags & COMMANDS_RECURSE)))
- {
- if (!assign_child_tempfiles (child, combined_output))
- {
- perror_with_name ("output-sync suppressed: ", "stderr");
- output_sync = 0;
- }
- }
- }
+ if (output_sync && sync_handle == -1)
+ sync_init();
+
+ if (output_sync && sync_cmd)
+ /* If we still want to sync, make sure we have temp files. */
+ assign_child_tempfiles (child);
+ else
+ /* We don't want to sync this command: to avoid misordered output
+ ensure any already-synched content is written. */
+ sync_output (child);
#endif /* OUTPUT_SYNC */
/* make UNC paths safe for CreateProcess -- backslash format */
@@ -1859,8 +1877,7 @@ start_job_command (struct child *child)
#ifdef OUTPUT_SYNC
/* Divert child output if output_sync in use. Don't capture
recursive make output unless we are synchronizing "make" mode. */
- if (output_sync && (output_sync == OUTPUT_SYNC_MAKE
- || !(flags & COMMANDS_RECURSE)))
+ if (output_sync && sync_cmd)
hPID = process_easy(argv, child->environment,
child->outfd, child->errfd);
else
diff --git a/misc.c b/misc.c
index 12b9d3d..766874f 100644
--- a/misc.c
+++ b/misc.c
@@ -182,7 +182,8 @@ concat (unsigned int num, ...)
/* If we had a standard-compliant vsnprintf() this would be a lot simpler.
Maybe in the future we'll include gnulib's version. */
-/* Return a formatted string buffer. */
+/* Return a formatted string buffer.
+ LENGTH must be the maximum length of all format arguments, stringified. */
const char *
message_s (unsigned int length, int prefix, const char *fmt, ...)
@@ -193,7 +194,7 @@ message_s (unsigned int length, int prefix, const char *fmt, ...)
va_list args;
/* Compute the maximum buffer size we'll need, and make sure we have it. */
- length += strlen (fmt) + strlen (program) + 4 + INTEGER_LENGTH + 1;
+ length += strlen (fmt) + strlen (program) + 4 + INTEGER_LENGTH + 2;
if (length > bsize)
{
bsize = length * 2;
@@ -214,10 +215,13 @@ message_s (unsigned int length, int prefix, const char *fmt, ...)
vsprintf (bp, fmt, args);
va_end (args);
+ strcat (bp, "\n");
+
return buffer;
}
-/* Return a formatted error message in a buffer. */
+/* Return a formatted error message in a buffer.
+ LENGTH must be the maximum length of all format arguments, stringified. */
const char *
error_s (unsigned int length, const gmk_floc *flocp, const char *fmt, ...)
@@ -228,7 +232,7 @@ error_s (unsigned int length, const gmk_floc *flocp, const char *fmt, ...)
va_list args;
/* Compute the maximum buffer size we'll need, and make sure we have it. */
- length += (strlen (fmt) + strlen (program) + 4 + INTEGER_LENGTH + 1
+ length += (strlen (fmt) + strlen (program) + 4 + INTEGER_LENGTH + 2
+ (flocp && flocp->filenm ? strlen (flocp->filenm) : 0));
if (length > bsize)
{
@@ -249,10 +253,13 @@ error_s (unsigned int length, const gmk_floc *flocp, const char *fmt, ...)
vsprintf (bp, fmt, args);
va_end (args);
+ strcat (bp, "\n");
+
return buffer;
}
-/* Print a message on stdout. */
+/* Print a message on stdout. We could use message_s() to format it but then
+ we'd need a va_list version... */
void
message (int prefix, const char *fmt, ...)
@@ -950,7 +957,7 @@ char *mktemp (char *template);
# endif
#endif
-/* This is only used by output-sync, and it may not be portable to Windows. */
+/* This is only used by output-sync, and it may not be portable. */
#ifdef OUTPUT_SYNC
/* Returns a file descriptor to a temporary file. The file is automatically
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 0a72ebf..991e96f 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@
+2013-05-04 Paul Smith <psmith@gnu.org>
+
+ * scripts/features/output-sync (output_sync_set): New test for
+ ordered recursive output for -Ojob / -Otarget.
+
2013-05-03 Eli Zaretskii <eliz@gnu.org>
* scripts/features/load: Fix signatures of testload_gmk_setup and
diff --git a/tests/scripts/features/output-sync b/tests/scripts/features/output-sync
index c8ff291..a1560ad 100644
--- a/tests/scripts/features/output-sync
+++ b/tests/scripts/features/output-sync
@@ -244,5 +244,15 @@ foo: end
# Remove temporary directories and contents.
output_sync_clean();
+# Ensure recursion doesn't mis-order or double-print output
+run_make_test(qq!
+all:
+\t\@echo foo
+\t\@+echo bar
+!,
+ '-j -Ojob', "foo\nbar\n");
+
+run_make_test(undef, '-j -Otarget', "foo\nbar\n");
+
# This tells the test driver that the perl test script executed properly.
1;