Module Name: src
Committed By: rillig
Date: Fri Mar 7 06:50:35 UTC 2025
Modified Files:
src/usr.bin/make: job.c main.c make.h
Log Message:
make: clean up comments and code for parallel mode
Most of the comments didn't help understand the code.
The Finish function was named way too soft for what it does, which is to
exit without cleaning up properly. Since it was used only once, inline
it. The code from that function causes jobs to be terminated early when
an unrelated target has failed, resulting in partially written files,
due to SIGPIPE.
To generate a diff of this commit:
cvs rdiff -u -r1.486 -r1.487 src/usr.bin/make/job.c
cvs rdiff -u -r1.638 -r1.639 src/usr.bin/make/main.c
cvs rdiff -u -r1.349 -r1.350 src/usr.bin/make/make.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/usr.bin/make/job.c
diff -u src/usr.bin/make/job.c:1.486 src/usr.bin/make/job.c:1.487
--- src/usr.bin/make/job.c:1.486 Mon Feb 24 23:06:40 2025
+++ src/usr.bin/make/job.c Fri Mar 7 06:50:34 2025
@@ -1,4 +1,4 @@
-/* $NetBSD: job.c,v 1.486 2025/02/24 23:06:40 rillig Exp $ */
+/* $NetBSD: job.c,v 1.487 2025/03/07 06:50:34 rillig Exp $ */
/*
* Copyright (c) 1988, 1989, 1990 The Regents of the University of California.
@@ -102,10 +102,7 @@
* define the shell that is used for the creation
* commands in jobs mode.
*
- * Job_Finish Perform any final processing which needs doing.
- * This includes the execution of any commands
- * which have been/were attached to the .END
- * target. It should only be called when the
+ * Job_Finish Make the .END target. Should only be called when the
* job table is empty.
*
* Job_AbortAll Abort all currently running jobs. Do not handle
@@ -141,7 +138,7 @@
#include "trace.h"
/* "@(#)job.c 8.2 (Berkeley) 3/19/94" */
-MAKE_RCSID("$NetBSD: job.c,v 1.486 2025/02/24 23:06:40 rillig Exp $");
+MAKE_RCSID("$NetBSD: job.c,v 1.487 2025/03/07 06:50:34 rillig Exp $");
/*
* A shell defines how the commands are run. All commands for a target are
@@ -660,7 +657,7 @@ JobPassSig_suspend(int signo)
act.sa_flags = 0;
(void)sigaction(signo, &act, NULL);
- DEBUG1(JOB, "JobPassSig passing signal %d to self.\n", signo);
+ DEBUG1(JOB, "JobPassSig_suspend passing signal %d to self.\n", signo);
(void)kill(getpid(), signo);
@@ -1222,8 +1219,16 @@ JobFinish(Job *job, int status)
if (return_job_token)
Job_TokenReturn();
- if (aborting == ABORT_ERROR && jobTokensRunning == 0)
- Finish(job_errors);
+ if (aborting == ABORT_ERROR && jobTokensRunning == 0) {
+ if (shouldDieQuietly(NULL, -1)) {
+ /*
+ * TODO: better clean up properly, to avoid killing
+ * child processes by SIGPIPE.
+ */
+ exit(2);
+ }
+ Fatal("%d error%s", job_errors, job_errors == 1 ? "" : "s");
+ }
}
static void
@@ -1930,27 +1935,14 @@ JobRun(GNode *targ)
}
}
-/*
- * Handle the exit of a child. Called from Make_Make.
- *
- * The job descriptor is removed from the list of children.
- *
- * Notes:
- * We do waits, blocking or not, according to the wisdom of our
- * caller, until there are no more children to report. For each
- * job, call JobFinish to finish things off.
- */
void
Job_CatchChildren(void)
{
- int pid; /* pid of dead child */
- int status; /* Exit/termination status */
+ int pid;
+ int status;
- /* Don't even bother if we know there's no one around. */
if (jobTokensRunning == 0)
return;
-
- /* Have we received SIGCHLD since last call? */
if (caught_sigchld == 0)
return;
caught_sigchld = 0;
@@ -1969,21 +1961,19 @@ Job_CatchChildren(void)
void
JobReapChild(pid_t pid, int status, bool isJobs)
{
- Job *job; /* job descriptor for dead child */
+ Job *job;
- /* Don't even bother if we know there's no one around. */
if (jobTokensRunning == 0)
return;
job = JobFindPid(pid, JOB_ST_RUNNING, isJobs);
if (job == NULL) {
- if (isJobs) {
- if (!lurking_children)
- Error("Child (%d) status %x not in table?",
- pid, status);
- }
- return; /* not ours */
+ if (isJobs && !lurking_children)
+ Error("Child (%d) status %x not in table?",
+ pid, status);
+ return;
}
+
if (WIFSTOPPED(status)) {
DEBUG2(JOB, "Process %d (%s) stopped.\n",
job->pid, job->node->name);
@@ -2015,12 +2005,6 @@ JobReapChild(pid_t pid, int status, bool
JobFinish(job, status);
}
-/*
- * Catch the output from our children, if we're using pipes do so. Otherwise
- * just block time until we get a signal(most likely a SIGCHLD) since there's
- * no point in just spinning when there's nothing to do and the reaping of a
- * child can wait for a while.
- */
void
Job_CatchOutput(void)
{
@@ -2030,7 +2014,7 @@ Job_CatchOutput(void)
(void)fflush(stdout);
- /* The first fd in the list is the job token pipe */
+ /* Skip the first fd in the list, as it is the job token pipe. */
do {
nready = poll(fds + 1 - wantToken, fdsLen - 1 + wantToken,
POLL_MSEC);
@@ -2041,13 +2025,12 @@ Job_CatchOutput(void)
if (nready > 0 && readyfd(&childExitJob)) {
char token = 0;
- ssize_t count;
- count = read(childExitJob.inPipe, &token, 1);
+ ssize_t count = read(childExitJob.inPipe, &token, 1);
if (count == 1) {
if (token == DO_JOB_RESUME[0])
/*
* Complete relay requested from our SIGCONT
- * handler
+ * handler.
*/
JobRestartJobs();
} else if (count == 0)
@@ -2125,10 +2108,7 @@ Shell_Init(void)
}
}
-/*
- * Return the string literal that is used in the current command shell
- * to produce a newline character.
- */
+/* Return the shell string literal that results in a newline character. */
const char *
Shell_GetNewline(void)
{
@@ -2157,12 +2137,11 @@ AddSig(int sig, SignalProc handler)
}
}
-/* Initialize the process module. */
void
Job_Init(void)
{
Job_SetPrefix();
- /* Allocate space for all the job info */
+
job_table = bmake_malloc((size_t)opts.maxJobs * sizeof *job_table);
memset(job_table, 0, (size_t)opts.maxJobs * sizeof *job_table);
job_table_end = job_table + opts.maxJobs;
@@ -2173,8 +2152,8 @@ Job_Init(void)
job_errors = 0;
/*
- * There is a non-zero chance that we already have children.
- * eg after 'make -f- <<EOF'
+ * There is a non-zero chance that we already have children,
+ * e.g. after 'make -f- <<EOF'.
* Since their termination causes a 'Child (pid) not in table'
* message, Collect the status of any that are already dead, and
* suppress the error message if there are any undead ones.
@@ -2194,7 +2173,6 @@ Job_Init(void)
JobCreatePipe(&childExitJob, 3);
{
- /* Preallocate enough for the maximum number of jobs. */
size_t nfds = (npseudojobs + (size_t)opts.maxJobs) *
nfds_per_job();
fds = bmake_malloc(sizeof *fds * nfds);
@@ -2206,24 +2184,19 @@ Job_Init(void)
watchfd(&childExitJob);
sigemptyset(&caught_signals);
- /* Install a SIGCHLD handler. */
(void)bmake_signal(SIGCHLD, JobChildSig);
sigaddset(&caught_signals, SIGCHLD);
- /*
- * Catch the four signals that POSIX specifies if they aren't ignored.
- * JobPassSig will take care of calling JobInterrupt if appropriate.
- */
+ /* Handle the signals specified by POSIX. */
AddSig(SIGINT, JobPassSig_int);
AddSig(SIGHUP, JobPassSig_term);
AddSig(SIGTERM, JobPassSig_term);
AddSig(SIGQUIT, JobPassSig_term);
/*
- * There are additional signals that need to be caught and passed if
- * either the export system wants to be told directly of signals or if
- * we're giving each job its own process group (since then it won't get
- * signals from the terminal driver as we own the terminal)
+ * These signals need to be passed to the jobs, as each job has its
+ * own process group and thus the terminal driver doesn't forward the
+ * signals itself.
*/
AddSig(SIGTSTP, JobPassSig_suspend);
AddSig(SIGTTOU, JobPassSig_suspend);
@@ -2232,10 +2205,7 @@ Job_Init(void)
AddSig(SIGCONT, JobContinueSig);
(void)Job_RunTarget(".BEGIN", NULL);
- /*
- * Create the .END node now, even though no code in the unit tests
- * depends on it. See also Targ_GetEndNode in Compat_MakeAll.
- */
+ /* Create the .END node, see Targ_GetEndNode in Compat_MakeAll. */
(void)Targ_GetEndNode();
}
@@ -2261,7 +2231,6 @@ JobSigReset(void)
(void)bmake_signal(SIGCHLD, SIG_DFL);
}
-/* Find a shell in 'shells' given its name, or return NULL. */
static Shell *
FindShellByName(const char *name)
{
@@ -2283,40 +2252,35 @@ FindShellByName(const char *name)
* line The shell spec
*
* Results:
- * false if the specification was incorrect.
- *
- * Side Effects:
- * 'shell' points to a Shell structure (either predefined or
- * created from the shell spec), shellPath is the full path of the
- * shell described by 'shell', while shellName is just the
- * final component of shellPath.
+ * Returns false if the specification was incorrect.
+ * If successful, 'shell' is usable, shellPath is the full path of the
+ * shell described by 'shell', and shellName is the final component of
+ * shellPath.
*
* Notes:
- * A shell specification consists of a .SHELL target, with dependency
- * operator, followed by a series of blank-separated words. Double
- * quotes can be used to use blanks in words. A backslash escapes
+ * A shell specification has the form ".SHELL: keyword=value...". Double
+ * quotes can be used to enclose blanks in words. A backslash escapes
* anything (most notably a double-quote and a space) and
- * provides the functionality it does in C. Each word consists of
- * keyword and value separated by an equal sign. There should be no
- * unnecessary spaces in the word. The keywords are as follows:
+ * provides the usual escape sequences from C. There should be no
+ * unnecessary spaces in the word. The keywords are:
* name Name of shell.
* path Location of shell.
* quiet Command to turn off echoing.
* echo Command to turn echoing on
- * filter Result of turning off echoing that shouldn't be
- * printed.
- * echoFlag Flag to turn echoing on at the start
- * errFlag Flag to turn error checking on at the start
- * hasErrCtl True if shell has error checking control
- * newline String literal to represent a newline char
- * check Command to turn on error checking if hasErrCtl
- * is true or template of command to echo a command
- * for which error checking is off if hasErrCtl is
- * false.
- * ignore Command to turn off error checking if hasErrCtl
- * is true or template of command to execute a
- * command so as to ignore any errors it returns if
- * hasErrCtl is false.
+ * filter The output from the shell command that turns off
+ * echoing, to be filtered from the final output.
+ * echoFlag Flag to turn echoing on at the start.
+ * errFlag Flag to turn error checking on at the start.
+ * hasErrCtl True if the shell has error checking control.
+ * newline String literal to represent a newline character.
+ * check If hasErrCtl is true: The command to turn on error
+ * checking. If hasErrCtl is false: The template for a
+ * shell command that echoes a command for which error
+ * checking is off.
+ * ignore If hasErrCtl is true: The command to turn off error
+ * checking. If hasErrCtl is false: The template for a
+ * shell command that executes a command so as to ignore
+ * any errors it returns.
*/
bool
Job_ParseShell(char *line)
@@ -2337,7 +2301,6 @@ Job_ParseShell(char *line)
memset(&newShell, 0, sizeof newShell);
- /* Parse the specification by keyword. */
wordsList = Str_Words(line, true);
words = wordsList.words;
argc = wordsList.len;
@@ -2401,12 +2364,6 @@ Job_ParseShell(char *line)
}
if (path == NULL) {
- /*
- * If no path was given, the user wants one of the
- * pre-defined shells, yes? So we find the one s/he wants
- * with the help of FindShellByName and set things up the
- * right way. shellPath will be set up by Shell_Init.
- */
if (newShell.name == NULL) {
Parse_Error(PARSE_FATAL,
"Neither path nor name specified");
@@ -2422,10 +2379,6 @@ Job_ParseShell(char *line)
shell = sh;
shellName = newShell.name;
if (shellPath != NULL) {
- /*
- * Shell_Init has already been called!
- * Do it again.
- */
free(shellPath);
shellPath = NULL;
Shell_Init();
@@ -2448,7 +2401,7 @@ Job_ParseShell(char *line)
shell = bmake_malloc(sizeof *shell);
*shell = newShell;
}
- /* this will take care of shellErrFlag */
+ /* This will take care of shellErrFlag. */
Shell_Init();
}
@@ -2463,7 +2416,7 @@ Job_ParseShell(char *line)
}
/*
- * Do not free up the words themselves, since they might be in use
+ * Do not free up the words themselves, since they may be in use
* by the shell specification.
*/
free(words);
@@ -2471,21 +2424,14 @@ Job_ParseShell(char *line)
}
/*
- * Handle the receipt of an interrupt.
- *
- * All children are killed. Another job will be started if the .INTERRUPT
- * target is defined.
- *
- * Input:
- * runINTERRUPT Non-zero if commands for the .INTERRUPT target
- * should be executed
- * signo signal received
+ * After receiving an interrupt signal, terminate all child processes and if
+ * necessary make the .INTERRUPT target.
*/
static void
JobInterrupt(bool runINTERRUPT, int signo)
{
- Job *job; /* job descriptor in that element */
- GNode *interrupt; /* the node describing the .INTERRUPT target */
+ Job *job;
+ GNode *interrupt;
sigset_t mask;
aborting = ABORT_INTERRUPT;
@@ -2522,11 +2468,7 @@ JobInterrupt(bool runINTERRUPT, int sign
exit(signo); /* XXX: why signo? */
}
-/*
- * Do the final processing, i.e. run the commands attached to the .END target.
- *
- * Return the number of errors reported.
- */
+/* Make the .END target, returning the number of job-related errors. */
int
Job_Finish(void)
{
@@ -2542,7 +2484,6 @@ Job_Finish(void)
}
#ifdef CLEANUP
-/* Clean up any memory used by the jobs module. */
void
Job_End(void)
{
@@ -2550,14 +2491,11 @@ Job_End(void)
}
#endif
-/*
- * Waits for all running jobs to finish and returns.
- * Sets 'aborting' to ABORT_WAIT to prevent other jobs from starting.
- */
+/* Waits for all running jobs to finish. */
void
Job_Wait(void)
{
- aborting = ABORT_WAIT;
+ aborting = ABORT_WAIT; /* Prevent other jobs from starting. */
while (jobTokensRunning != 0) {
Job_CatchOutput();
}
@@ -2568,14 +2506,12 @@ Job_Wait(void)
* Abort all currently running jobs without handling output or anything.
* This function is to be called only in the event of a major error.
* Most definitely NOT to be called from JobInterrupt.
- *
- * All children are killed, not just the firstborn.
*/
void
Job_AbortAll(void)
{
- Job *job; /* the job descriptor in that element */
- int foo;
+ Job *job;
+ int status;
aborting = ABORT_ERROR;
@@ -2583,25 +2519,18 @@ Job_AbortAll(void)
for (job = job_table; job < job_table_end; job++) {
if (job->status != JOB_ST_RUNNING)
continue;
- /*
- * kill the child process with increasingly drastic
- * signals to make darn sure it's dead.
- */
KILLPG(job->pid, SIGINT);
KILLPG(job->pid, SIGKILL);
}
}
- /*
- * Catch as many children as want to report in at first, then give up
- */
- while (waitpid((pid_t)-1, &foo, WNOHANG) > 0)
+ while (waitpid(-1, &status, WNOHANG) > 0)
continue;
}
/*
* Tries to restart stopped jobs if there are slots available.
- * Called in process context in response to a SIGCONT.
+ * Called in response to a SIGCONT.
*/
static void
JobRestartJobs(void)
@@ -2666,10 +2595,6 @@ clearfd(Job *job)
fdsLen--;
#if defined(USE_FILEMON) && !defined(USE_FILEMON_DEV)
if (useMeta) {
- /*
- * Sanity check: there should be two fds per job, so the job's
- * pollfd number should be even.
- */
assert(nfds_per_job() == 2);
if (i % 2 != 0)
Punt("odd-numbered fd with meta");
@@ -2708,7 +2633,7 @@ JobTokenAdd(void)
{
char tok = JOB_TOKENS[aborting], tok1;
- /* If we are depositing an error token flush everything else */
+ /* If we are depositing an error token, flush everything else. */
while (tok != '+' && read(tokenWaitJob.inPipe, &tok1, 1) == 1)
continue;
@@ -2718,7 +2643,6 @@ JobTokenAdd(void)
continue;
}
-/* Get a temp file */
int
Job_TempFile(const char *pattern, char *tfile, size_t tfile_sz)
{
@@ -2734,7 +2658,7 @@ Job_TempFile(const char *pattern, char *
return fd;
}
-/* Prep the job token pipe in the root make process. */
+/* Prepare the job token pipe in the root make process. */
void
Job_ServerStart(int max_tokens, int jp_0, int jp_1)
{
@@ -2742,7 +2666,6 @@ Job_ServerStart(int max_tokens, int jp_0
char jobarg[64];
if (jp_0 >= 0 && jp_1 >= 0) {
- /* Pipe passed in from parent */
tokenWaitJob.inPipe = jp_0;
tokenWaitJob.outPipe = jp_1;
(void)fcntl(jp_0, F_SETFD, FD_CLOEXEC);
@@ -2784,7 +2707,7 @@ Job_TokenReturn(void)
/*
* Attempt to withdraw a token from the pool.
*
- * If pool is empty, set wantToken so that we wake up when a token is
+ * If the pool is empty, set wantToken so that we wake up when a token is
* released.
*
* Returns true if a token was withdrawn, and false if the pool is currently
@@ -2823,8 +2746,13 @@ Job_TokenWithdraw(void)
while (write(tokenWaitJob.outPipe, &tok, 1) == -1 &&
errno == EAGAIN)
continue;
- if (shouldDieQuietly(NULL, 1))
- exit(6); /* we aborted */
+ if (shouldDieQuietly(NULL, 1)) {
+ /*
+ * TODO: better clean up properly, to avoid killing
+ * child processes by SIGPIPE.
+ */
+ exit(6);
+ }
Fatal("A failure has been detected "
"in another branch of the parallel make");
}
@@ -2840,12 +2768,7 @@ Job_TokenWithdraw(void)
return true;
}
-/*
- * Run the named target if found. If a filename is specified, then set that
- * to the sources.
- *
- * Exits if the target fails.
- */
+/* Make the named target if found, exit if the target fails. */
bool
Job_RunTarget(const char *target, const char *fname)
{
Index: src/usr.bin/make/main.c
diff -u src/usr.bin/make/main.c:1.638 src/usr.bin/make/main.c:1.639
--- src/usr.bin/make/main.c:1.638 Sun Jan 19 12:59:39 2025
+++ src/usr.bin/make/main.c Fri Mar 7 06:50:34 2025
@@ -1,4 +1,4 @@
-/* $NetBSD: main.c,v 1.638 2025/01/19 12:59:39 rillig Exp $ */
+/* $NetBSD: main.c,v 1.639 2025/03/07 06:50:34 rillig Exp $ */
/*
* Copyright (c) 1988, 1989, 1990, 1993
@@ -83,9 +83,6 @@
* Fatal Print an error message and exit.
*
* Punt Abort all jobs and exit with a message.
- *
- * Finish Finish things up by printing the number of errors
- * that occurred, and exit.
*/
#include <sys/types.h>
@@ -111,7 +108,7 @@
#include "trace.h"
/* "@(#)main.c 8.3 (Berkeley) 3/19/94" */
-MAKE_RCSID("$NetBSD: main.c,v 1.638 2025/01/19 12:59:39 rillig Exp $");
+MAKE_RCSID("$NetBSD: main.c,v 1.639 2025/03/07 06:50:34 rillig Exp $");
#if defined(MAKE_NATIVE)
__COPYRIGHT("@(#) Copyright (c) 1988, 1989, 1990, 1993 "
"The Regents of the University of California. "
@@ -1940,19 +1937,6 @@ DieHorribly(void)
exit(2); /* Not 1 so -q can distinguish error */
}
-/*
- * Called when aborting due to errors in child shell to signal abnormal exit.
- * The program exits.
- * Errors is the number of errors encountered in Make_Make.
- */
-void
-Finish(int errs)
-{
- if (shouldDieQuietly(NULL, -1))
- exit(2);
- Fatal("%d error%s", errs, errs == 1 ? "" : "s");
-}
-
int
unlink_file(const char *file)
{
Index: src/usr.bin/make/make.h
diff -u src/usr.bin/make/make.h:1.349 src/usr.bin/make/make.h:1.350
--- src/usr.bin/make/make.h:1.349 Sun Jan 19 10:57:10 2025
+++ src/usr.bin/make/make.h Fri Mar 7 06:50:34 2025
@@ -1,4 +1,4 @@
-/* $NetBSD: make.h,v 1.349 2025/01/19 10:57:10 rillig Exp $ */
+/* $NetBSD: make.h,v 1.350 2025/03/07 06:50:34 rillig Exp $ */
/*
* Copyright (c) 1988, 1989, 1990, 1993
@@ -866,7 +866,6 @@ void Error(const char *, ...) MAKE_ATTR_
void Fatal(const char *, ...) MAKE_ATTR_PRINTFLIKE(1, 2) MAKE_ATTR_DEAD;
void Punt(const char *, ...) MAKE_ATTR_PRINTFLIKE(1, 2) MAKE_ATTR_DEAD;
void DieHorribly(void) MAKE_ATTR_DEAD;
-void Finish(int) MAKE_ATTR_DEAD;
int unlink_file(const char *) MAKE_ATTR_USE;
void execDie(const char *, const char *);
char *getTmpdir(void) MAKE_ATTR_USE;
