Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 1 Dec 2011 00:16:46 +0000
From:      Alexander Best <arundel@freebsd.org>
To:        Max Khon <fjoe@FreeBSD.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r228157 - head/usr.bin/make
Message-ID:  <20111201001646.GA49249@freebsd.org>
In-Reply-To: <201111301807.pAUI7cXI008371@svn.freebsd.org>
References:  <201111301807.pAUI7cXI008371@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Wed Nov 30 11, Max Khon wrote:
> Author: fjoe
> Date: Wed Nov 30 18:07:38 2011
> New Revision: 228157
> URL: http://svn.freebsd.org/changeset/base/228157
> 
> Log:
>   - Fix segmentation fault when running "+command" when run with -jX -n due
>   to Compat_RunCommand() being called with `cmd' that is not on the node->commands
>   list
>   - Make ellipsis ("..." command) handling consistent: check for "..." command
>   in job make after variables expansion to match compat make behavior
>   - Fix empty command handling (after variables expansion and @+- modifiers
>   are processed): now empty commands are ignored in compat make and are not
>   printed in job make case
>   - Bump MAKE_VERSION to 5-2011-11-30-0

it would also be nice, if at some point, somebody could dive into the code to
see why 'make buildkernel' will let clang produce coloured output, but
'make -j(N>1) buildkernel' doesn't (and why adding a -B switch to that command
fixes it).

cheers.
alex

> 
> Modified:
>   head/usr.bin/make/Makefile
>   head/usr.bin/make/job.c
> 
> Modified: head/usr.bin/make/Makefile
> ==============================================================================
> --- head/usr.bin/make/Makefile	Wed Nov 30 17:39:00 2011	(r228156)
> +++ head/usr.bin/make/Makefile	Wed Nov 30 18:07:38 2011	(r228157)
> @@ -10,7 +10,9 @@ SRCS=	arch.c buf.c cond.c dir.c for.c ha
>  
>  NO_SHARED?=	YES
>  
> -CFLAGS+=-DMAKE_VERSION=\"5200408120\"
> +# Version has the RYYYYMMDDX format, where R is from RELENG_<R>
> +CFLAGS+=-DMAKE_VERSION=\"5201111300\"
> +
>  # There is no obvious performance improvement currently.
>  # CFLAGS+=-DUSE_KQUEUE
>  
> 
> Modified: head/usr.bin/make/job.c
> ==============================================================================
> --- head/usr.bin/make/job.c	Wed Nov 30 17:39:00 2011	(r228156)
> +++ head/usr.bin/make/job.c	Wed Nov 30 18:07:38 2011	(r228157)
> @@ -381,7 +381,7 @@ static int JobStart(GNode *, int, Job *)
>  static void JobDoOutput(Job *, Boolean);
>  static void JobInterrupt(int, int);
>  static void JobRestartJobs(void);
> -static int Compat_RunCommand(char *, struct GNode *);
> +static int Compat_RunCommand(LstNode *, struct GNode *);
>  
>  static GNode	    *curTarg = NULL;
>  static GNode	    *ENDNode;
> @@ -647,7 +647,7 @@ JobPassSig(int signo)
>   *	numCommands is incremented if the command is actually printed.
>   */
>  static int
> -JobPrintCommand(char *cmd, Job *job)
> +JobPrintCommand(LstNode *cmdNode, Job *job)
>  {
>  	Boolean	noSpecials;	/* true if we shouldn't worry about
>  				 * inserting special commands into
> @@ -658,40 +658,30 @@ JobPrintCommand(char *cmd, Job *job)
>  				 * off before printing the command
>  				 * and need to turn it back on */
>  	const char *cmdTemplate;/* Template to use when printing the command */
> -	char	*cmdStart;	/* Start of expanded command */
> -	LstNode	*cmdNode;	/* Node for replacing the command */
> +	char	*cmd;		/* Expanded command */
>  
>  	noSpecials = (noExecute && !(job->node->type & OP_MAKE));
>  
> -	if (strcmp(cmd, "...") == 0) {
> -		job->node->type |= OP_SAVE_CMDS;
> -		if ((job->flags & JOB_IGNDOTS) == 0) {
> -			job->tailCmds =
> -			    Lst_Succ(Lst_Member(&job->node->commands, cmd));
> -			return (1);
> -		}
> -		return (0);
> -	}
> -
>  #define	DBPRINTF(fmt, arg)			\
>  	DEBUGF(JOB, (fmt, arg));		\
>  	fprintf(job->cmdFILE, fmt, arg);	\
>  	fflush(job->cmdFILE);
>  
> -	numCommands += 1;
> -
>  	/*
>  	 * For debugging, we replace each command with the result of expanding
>  	 * the variables in the command.
>  	 */
> -	cmdNode = Lst_Member(&job->node->commands, cmd);
> -
> -	cmd = Buf_Peel(Var_Subst(cmd, job->node, FALSE));
> -	cmdStart = cmd;
> -
> -	Lst_Replace(cmdNode, cmdStart);
> -
> -	cmdTemplate = "%s\n";
> +	cmd = Buf_Peel(Var_Subst(Lst_Datum(cmdNode), job->node, FALSE));
> +	if (strcmp(cmd, "...") == 0) {
> +		free(cmd);
> +		job->node->type |= OP_SAVE_CMDS;
> +		if ((job->flags & JOB_IGNDOTS) == 0) {
> +			job->tailCmds = Lst_Succ(cmdNode);
> +			return (1);
> +		}
> +		return (0);
> +	}
> +	Lst_Replace(cmdNode, cmd);
>  
>  	/*
>  	 * Check for leading @', -' or +'s to control echoing, error checking,
> @@ -715,7 +705,7 @@ JobPrintCommand(char *cmd, Job *job)
>  				 * but this one needs to be - use compat mode
>  				 * just for it.
>  				 */
> -				Compat_RunCommand(cmd, job->node);
> +				Compat_RunCommand(cmdNode, job->node);
>  				return (0);
>  			}
>  			break;
> @@ -726,6 +716,16 @@ JobPrintCommand(char *cmd, Job *job)
>  	while (isspace((unsigned char)*cmd))
>  		cmd++;
>  
> +	/*
> +	 * Ignore empty commands
> +	 */
> +	if (*cmd == '\0') {
> +		return (0);
> +	}
> +
> +	cmdTemplate = "%s\n";
> +	numCommands += 1;
> +
>  	if (shutUp) {
>  		if (!(job->flags & JOB_SILENT) && !noSpecials &&
>  		    commandShell->hasEchoCtl) {
> @@ -1665,7 +1665,7 @@ JobStart(GNode *gn, int flags, Job *prev
>  				    Lst_Succ(gn->compat_command);
>  
>  			if (gn->compat_command == NULL ||
> -			    JobPrintCommand(Lst_Datum(gn->compat_command), job))
> +			    JobPrintCommand(gn->compat_command, job))
>  				noExec = TRUE;
>  
>  			if (noExec && !(job->flags & JOB_FIRST)) {
> @@ -1689,7 +1689,7 @@ JobStart(GNode *gn, int flags, Job *prev
>  			 */
>  			numCommands = 0;
>  			LST_FOREACH(ln, &gn->commands) {
> -				if (JobPrintCommand(Lst_Datum(ln), job))
> +				if (JobPrintCommand(ln, job))
>  					break;
>  			}
>  
> @@ -1723,7 +1723,7 @@ JobStart(GNode *gn, int flags, Job *prev
>  		 */
>  		if (cmdsOK) {
>  			LST_FOREACH(ln, &gn->commands) {
> -				if (JobPrintCommand(Lst_Datum(ln), job))
> +				if (JobPrintCommand(ln, job))
>  					break;
>  			}
>  		}
> @@ -2809,7 +2809,7 @@ CompatInterrupt(int signo)
>  		gn = Targ_FindNode(".INTERRUPT", TARG_NOCREATE);
>  		if (gn != NULL) {
>  			LST_FOREACH(ln, &gn->commands) {
> -				if (Compat_RunCommand(Lst_Datum(ln), gn))
> +				if (Compat_RunCommand(ln, gn))
>  					break;
>  			}
>  		}
> @@ -2884,16 +2884,15 @@ shellneed(ArgArray *aa, char *cmd)
>   *	The node's 'made' field may be set to ERROR.
>   */
>  static int
> -Compat_RunCommand(char *cmd, GNode *gn)
> +Compat_RunCommand(LstNode *cmdNode, GNode *gn)
>  {
>  	ArgArray	aa;
> -	char		*cmdStart;	/* Start of expanded command */
> +	char		*cmd;		/* Expanded command */
>  	Boolean		silent;		/* Don't print command */
>  	Boolean		doit;		/* Execute even in -n */
>  	Boolean		errCheck;	/* Check errors */
>  	int		reason;		/* Reason for child's death */
>  	int		status;		/* Description of child's death */
> -	LstNode		*cmdNode;	/* Node where current cmd is located */
>  	char		**av;		/* Argument vector for thing to exec */
>  	ProcStuff	ps;
>  
> @@ -2901,31 +2900,16 @@ Compat_RunCommand(char *cmd, GNode *gn)
>  	errCheck = !(gn->type & OP_IGNORE);
>  	doit = FALSE;
>  
> -	cmdNode = Lst_Member(&gn->commands, cmd);
> -	cmdStart = Buf_Peel(Var_Subst(cmd, gn, FALSE));
> -
> -	/*
> -	 * brk_string will return an argv with a NULL in av[0], thus causing
> -	 * execvp() to choke and die horribly. Besides, how can we execute a
> -	 * null command? In any case, we warn the user that the command
> -	 * expanded to nothing (is this the right thing to do?).
> -	 */
> -	if (*cmdStart == '\0') {
> -		free(cmdStart);
> -		Error("%s expands to empty string", cmd);
> -		return (0);
> -	} else {
> -		cmd = cmdStart;
> -	}
> -	Lst_Replace(cmdNode, cmdStart);
> -
> +	cmd = Buf_Peel(Var_Subst(Lst_Datum(cmdNode), gn, FALSE));
>  	if ((gn->type & OP_SAVE_CMDS) && (gn != ENDNode)) {
> -		Lst_AtEnd(&ENDNode->commands, cmdStart);
> +		Lst_AtEnd(&ENDNode->commands, cmd);
>  		return (0);
> -	} else if (strcmp(cmdStart, "...") == 0) {
> +	} else if (strcmp(cmd, "...") == 0) {
> +		free(cmd);
>  		gn->type |= OP_SAVE_CMDS;
>  		return (0);
>  	}
> +	Lst_Replace(cmdNode, cmd);
>  
>  	while (*cmd == '@' || *cmd == '-' || *cmd == '+') {
>  		switch (*cmd) {
> @@ -2949,6 +2933,13 @@ Compat_RunCommand(char *cmd, GNode *gn)
>  		cmd++;
>  
>  	/*
> +	 * Ignore empty commands
> +	 */
> +	if (*cmd == '\0') {
> +		return (0);
> +	}
> +
> +	/*
>  	 * Print the command before echoing if we're not supposed to be quiet
>  	 * for this one. We also print the command if -n given, but not if '+'.
>  	 */
> @@ -3022,7 +3013,8 @@ Compat_RunCommand(char *cmd, GNode *gn)
>  		 * therefore do not free it when debugging.
>  		 */
>  		if (!DEBUG(GRAPH2)) {
> -			free(cmdStart);
> +			free(Lst_Datum(cmdNode));
> +			Lst_Replace(cmdNode, NULL);
>  		}
>  
>  		/*
> @@ -3166,8 +3158,7 @@ Compat_Make(GNode *gn, GNode *pgn)
>  			if (!touchFlag) {
>  				curTarg = gn;
>  				LST_FOREACH(ln, &gn->commands) {
> -					if (Compat_RunCommand(Lst_Datum(ln),
> -					    gn))
> +					if (Compat_RunCommand(ln, gn))
>  						break;
>  				}
>  				curTarg = NULL;
> @@ -3345,7 +3336,7 @@ Compat_Run(Lst *targs)
>  		gn = Targ_FindNode(".BEGIN", TARG_NOCREATE);
>  		if (gn != NULL) {
>  			LST_FOREACH(ln, &gn->commands) {
> -				if (Compat_RunCommand(Lst_Datum(ln), gn))
> +				if (Compat_RunCommand(ln, gn))
>  					break;
>  			}
>  			if (gn->made == ERROR) {
> @@ -3386,7 +3377,7 @@ Compat_Run(Lst *targs)
>  	 */
>  	if (makeErrors == 0) {
>  		LST_FOREACH(ln, &ENDNode->commands) {
> -			if (Compat_RunCommand(Lst_Datum(ln), ENDNode))
> +			if (Compat_RunCommand(ln, ENDNode))
>  				break;
>  		}
>  	}



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111201001646.GA49249>