[PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c

[PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c

[Jonathan Nieder]

Hi,

Jilles Tjoelker wrote[0]:

> Regarding sh -c optimization, I am in favour of this. Uselessly waiting
> 'sh -c' processes annoy me. I made the change for FreeBSD 8.0 sh, which
> is very similar to dash. The SVN changeset is r194128.

So I grabbed that changeset with

	svn log -v svn://svn.freebsd.org/base/head/bin/sh -r 194128
	svn diff -r 194127:194128 svn://svn.freebsd.org/base/head/bin/sh

and made it a tiny bit smaller.   Here's the result.

   text    data     bss     dec     hex filename
  83994    1784   11128   96906   17a8a dash.before-O2
  83994    1784   11128   96906   17a8a dash.before-Os
  84146    1784   11128   97058   17b22 dash.after-O2
  84146    1784   11128   97058   17b22 dash.after-Os

On this amd64 the cost is 152 bytes of text.  Thoughts?

[0] http://bugs.debian.org/436466

Jilles Tjoelker (4):
  [INPUT] Introduce preadateof predicate to check for end of input
  [EVAL] Make eval flags public
  [EVAL] Take advantage of EV_EXIT in evalstring
  [MAIN] Optimize dash -c "command" to avoid a fork

 src/eval.c  |   11 +++++------
 src/eval.h  |    5 +++++
 src/input.c |   17 +++++++++++++++++
 src/input.h |    1 +
 src/main.c  |    2 +-
 5 files changed, 29 insertions(+), 7 deletions(-)

[PATCH 1/4] [INPUT] Introduce preadateof predicate to check for end of input

[Jonathan Nieder]

From: Jilles Tjoelker <jilles@stack.nl>
Date: Sat, 13 Jun 2009 16:17:45 -0500

For example, this can be used to detect tail calls in the string
passed to -c.

[jn: originally from Jilles as part of FreeBSD SVN r194128]

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Could be squashed with patch 3.  I split it out while considering
using the same trick in cmdloop.

 src/input.c |   17 +++++++++++++++++
 src/input.h |    1 +
 2 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/src/input.c b/src/input.c
index d31c45b..39d7893 100644
--- a/src/input.c
+++ b/src/input.c
@@ -325,6 +325,23 @@ again:
 }
 
 /*
+ * Returns if we are certain we are at EOF. Does not cause any more input
+ * to be read from the outside world.
+ */
+
+int
+preadateof(void)
+{
+	if (parsenleft > 0)
+		return 0;
+	if (parsefile->strpush)
+		return 0;
+	if (parsenleft == EOF_NLEFT || parsefile->buf == NULL)
+		return 1;
+	return 0;
+}
+
+/*
  * Undo the last call to pgetc.  Only one character may be pushed back.
  * PEOF may be pushed back.
  */
diff --git a/src/input.h b/src/input.h
index 50a7797..d123045 100644
--- a/src/input.h
+++ b/src/input.h
@@ -53,6 +53,7 @@ extern char *parsenextc;	/* next character in input buffer */
 int pgetc(void);
 int pgetc2(void);
 int preadbuffer(void);
+int preadateof(void);
 void pungetc(void);
 void pushstring(char *, void *);
 void popstring(void);
-- 
1.7.5.rc0

[PATCH 2/4] [EVAL] Make eval flags public

[Jonathan Nieder]

From: Jilles Tjoelker <jilles@stack.nl>
Date: Sat, 13 Jun 2009 16:17:45 -0500

Evaltree and its variants support three flags:

 - EV_EXIT means to exit when evaluation is complete (for use by the
   "exec" builtin, pipelines, and subshells).
 - EV_TESTED means that this code is the argument to a conditional.
   If some part of it fails and the -e flag is set, it will not cause
   the shell to exit.
 - EV_BACKCMD is an unused vestige from an experiment long past.

There has been no need to advertise the possible flags outside eval.c
so far because the only external callers, main and cmdloop, always
use 0 (no flags).  Expose the flags in eval.h so that can change.

[jn: originally from Jilles as part of FreeBSD SVN r194128; split up
 for clarity.]

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 src/eval.c |    5 -----
 src/eval.h |    5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/eval.c b/src/eval.c
index 426c03a..5c26133 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -65,11 +65,6 @@
 #endif
 
 
-/* flags in argument to evaltree */
-#define EV_EXIT 01		/* exit after evaluating tree */
-#define EV_TESTED 02		/* exit status is checked; ignore -e flag */
-#define EV_BACKCMD 04		/* command executing within back quotes */

[PATCH 3/4] [EVAL] Take advantage of EV_EXIT in evalstring

[Jonathan Nieder]

From: Jilles Tjoelker <jilles@stack.nl>
Date: Sat, 13 Jun 2009 16:17:45 -0500

Check if there may be additional data in the string after parsing each
command.  If there is none, let the EV_EXIT flag take effect so a fork
can be omitted in specific cases.

No change in behavior intended --- all current callers leave the
EV_EXIT flag cleared.

[jn: the original from FreeBSD SVN r194128 would unconditionally exit
 if the EV_EXIT bit was set. but for simplicity and consistency with
 other non-evaltree eval* commands, this version relies on the caller
 to exit when the command is empty.  One can insert a

	evaltree(NULL, flags);

 call before popfile() to get the original's semantics.

 Any outstanding bugs are my fault.]

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 src/eval.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/src/eval.c b/src/eval.c
index 5c26133..77a9d00 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -165,7 +165,10 @@ evalstring(char *s, int flags)
 
 	status = 0;
 	while ((n = parsecmd(0)) != NEOF) {
-		evaltree(n, flags);
+		if (preadateof())
+			evaltree(n, flags);
+		else
+			evaltree(n, flags & ~EV_EXIT);
 		status = exitstatus;
 		popstackmark(&smark);
 		if (evalskip)
-- 
1.7.5.rc0

[PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork

[Jonathan Nieder]

From: Jilles Tjoelker <jilles@stack.nl>
Date: Sat, 13 Jun 2009 16:17:45 -0500

This change only affects strings passed to -c, when the -s option is
not used.

Use the EV_EXIT flag to inform the eval machinery that the string
being passed is the entirety of input.  This way, a fork may be
omitted in many special cases.

If there are empty lines after the last command, the evalcmd will not
see the end early enough and forks will not be omitted. The same thing
seems to happen in bash.

Example:
  sh -c 'ps lT'
No longer shows a shell process waiting for ps to finish.

[jn: ported from FreeBSD SVN r194128.  Bugs are mine.]

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 src/main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/main.c b/src/main.c
index b38dc27..af987c6 100644
--- a/src/main.c
+++ b/src/main.c
@@ -171,7 +171,7 @@ state2:
 state3:
 	state = 4;
 	if (minusc)
-		evalstring(minusc, 0);
+		evalstring(minusc, sflag ? 0 : EV_EXIT);
 
 	if (sflag || minusc == NULL) {
 state4:	/* XXX ??? - why isn't this before the "if" statement */
-- 
1.7.5.rc0

Re: [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork

[Herbert Xu]

On Sun, Apr 10, 2011 at 07:36:49AM +0000, Jonathan Nieder wrote:
> From: Jilles Tjoelker <jilles@stack.nl>
> Date: Sat, 13 Jun 2009 16:17:45 -0500
> 
> This change only affects strings passed to -c, when the -s option is
> not used.
> 
> Use the EV_EXIT flag to inform the eval machinery that the string
> being passed is the entirety of input.  This way, a fork may be
> omitted in many special cases.
> 
> If there are empty lines after the last command, the evalcmd will not
> see the end early enough and forks will not be omitted. The same thing
> seems to happen in bash.
> 
> Example:
>   sh -c 'ps lT'
> No longer shows a shell process waiting for ps to finish.
> 
> [jn: ported from FreeBSD SVN r194128.  Bugs are mine.]
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

OK I will take this patch since its impact is much smaller than
one that hacks around the input path, and seems to achieve most of
what you want anyway.

I will roll it and the EV_EXIT exporting patch into one.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork

[Jonathan Nieder]

Herbert Xu wrote:
> On Sun, Apr 10, 2011 at 07:36:49AM +0000, Jonathan Nieder wrote:

>> From: Jilles Tjoelker <jilles@stack.nl>
>> Date: Sat, 13 Jun 2009 16:17:45 -0500
>>
>> This change only affects strings passed to -c, when the -s option is
>> not used.
>>
>> Use the EV_EXIT flag to inform the eval machinery that the string
>> being passed is the entirety of input.  This way, a fork may be
>> omitted in many special cases.
[...]
> OK I will take this patch since its impact is much smaller than
> one that hacks around the input path, and seems to achieve most of
> what you want anyway.

Will that work?  Without the preadateof check, I would worry that
passing EV_EXIT to evalstring would break:

	$ dash -c 'echo one
		echo two'
	one

Re: [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork

[Herbert Xu]

On Wed, Jul 06, 2011 at 11:27:53PM -0500, Jonathan Nieder wrote:
>
> Will that work?  Without the preadateof check, I would worry that
> passing EV_EXIT to evalstring would break:
> 
> 	$ dash -c 'echo one
> 		echo two'
> 	one

You're right.  I'll back this out for now.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork

[Herbert Xu]

On Thu, Jul 07, 2011 at 12:57:19PM +0800, Herbert Xu wrote:
> On Wed, Jul 06, 2011 at 11:27:53PM -0500, Jonathan Nieder wrote:
> >
> > Will that work?  Without the preadateof check, I would worry that
> > passing EV_EXIT to evalstring would break:
> > 
> > 	$ dash -c 'echo one
> > 		echo two'
> > 	one
> 
> You're right.  I'll back this out for now.

OK, what about this patch?

diff --git a/ChangeLog b/ChangeLog
index 7367c33..c457fc8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2011-07-07  Herbert Xu <herbert@gondor.apana.org.au>
+
+	* Optimize dash -c "command" to avoid a fork.
+
 2011-04-10  Jonathan Nieder <jrnieder@gmail.com>
  
 	* Remove unused EV_BACKCMD flag.
diff --git a/src/Makefile.am b/src/Makefile.am
index ba68d55..05ed70a 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -33,7 +33,7 @@ dash_LDADD = builtins.o init.o nodes.o signames.o syntax.o
 
 HELPERS = mkinit mksyntax mknodes mksignames
 
-BUILT_SOURCES = builtins.h nodes.h syntax.h token.h
+BUILT_SOURCES = builtins.h nodes.h syntax.h token.h token_vars.h
 CLEANFILES = \
 	$(BUILT_SOURCES) $(patsubst %.o,%.c,$(dash_LDADD)) \
 	$(HELPERS) builtins.def
@@ -44,7 +44,7 @@ EXTRA_DIST = \
 	mktokens mkbuiltins builtins.def.in mkinit.c \
 	mknodes.c nodetypes nodes.c.pat mksyntax.c mksignames.c
 
-token.h: mktokens
+token.h token_vars.h: mktokens
 	sh $^
 
 builtins.def: builtins.def.in $(top_builddir)/config.h
diff --git a/src/eval.c b/src/eval.c
index 86423b4..6e7b932 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -65,10 +65,6 @@
 #endif
 
 
-/* flags in argument to evaltree */
-#define EV_EXIT 01		/* exit after evaluating tree */
-#define EV_TESTED 02		/* exit status is checked; ignore -e flag */
-
 int evalskip;			/* set if we are skipping commands */
 STATIC int skipcount;		/* number of levels to skip */
 MKINIT int loopnest;		/* current loop nesting level */
@@ -169,7 +165,7 @@ evalstring(char *s, int flags)
 
 	status = 0;
 	while ((n = parsecmd(0)) != NEOF) {
-		evaltree(n, flags);
+		evaltree(n, flags & ~(parser_eof() ? 0 : EV_EXIT));
 		status = exitstatus;
 		popstackmark(&smark);
 		if (evalskip)
diff --git a/src/eval.h b/src/eval.h
index ac394e8..4e4de30 100644
--- a/src/eval.h
+++ b/src/eval.h
@@ -46,6 +46,10 @@ struct backcmd {		/* result of evalbackcmd */
 	struct job *jp;		/* job structure for command */
 };
 
+/* flags in argument to evaltree */
+#define EV_EXIT 01		/* exit after evaluating tree */
+#define EV_TESTED 02		/* exit status is checked; ignore -e flag */
+
 int evalstring(char *, int);
 union node;	/* BLETCH for ansi C */
 void evaltree(union node *, int);
diff --git a/src/main.c b/src/main.c
index b38dc27..af987c6 100644
--- a/src/main.c
+++ b/src/main.c
@@ -171,7 +171,7 @@ state2:
 state3:
 	state = 4;
 	if (minusc)
-		evalstring(minusc, 0);
+		evalstring(minusc, sflag ? 0 : EV_EXIT);
 
 	if (sflag || minusc == NULL) {
 state4:	/* XXX ??? - why isn't this before the "if" statement */
diff --git a/src/mktokens b/src/mktokens
index 7c873af..cd52241 100644
--- a/src/mktokens
+++ b/src/mktokens
@@ -71,13 +71,16 @@ TEND	1	"}"
 nl=`wc -l /tmp/ka$$`
 exec > token.h
 awk '{print "#define " $1 " " NR-1}' /tmp/ka$$
+
+exec > token_vars.h
+
 echo '
 /* Array indicating which tokens mark the end of a list */
-const char tokendlist[] = {'
+static const char tokendlist[] = {'
 awk '{print "\t" $2 ","}' /tmp/ka$$
 echo '};
 
-const char *const tokname[] = {'
+static const char *const tokname[] = {'
 sed -e 's/"/\\"/g' \
     -e 's/[^	 ]*[	 ][	 ]*[^	 ]*[	 ][	 ]*\(.*\)/	"\1",/' \
     /tmp/ka$$
@@ -85,7 +88,7 @@ echo '};
 '
 sed 's/"//g' /tmp/ka$$ | awk '
 /TNOT/{print "#define KWDOFFSET " NR-1; print ""; 
-      print "STATIC const char *const parsekwd[] = {"}
+      print "static const char *const parsekwd[] = {"}
 /TNOT/,/neverfound/{if (last) print "	\"" last "\","; last = $3}
 END{print "	\"" last "\"\n};"}'
 
diff --git a/src/parser.c b/src/parser.c
index 528d005..6de2762 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -64,7 +64,7 @@
  */
 
 /* values returned by readtoken */
-#include "token.h"
+#include "token_vars.h"
 
 
 
@@ -86,7 +86,7 @@ struct heredoc *heredoclist;	/* list of here documents to read */
 int doprompt;			/* if set, prompt the user */
 int needprompt;			/* true if interactive and at start of line */
 int lasttoken;			/* last token read */
-MKINIT int tokpushback;		/* last token pushed back */
+int tokpushback;		/* last token pushed back */
 char *wordtext;			/* text of last word returned by readtoken */
 int checkkwd;
 struct nodelist *backquotelist;
@@ -210,6 +210,7 @@ list(int nlflag)
 				parseheredoc();
 			else
 				pungetc();		/* push back EOF on input */
+			tokpushback++;
 			return n1;
 		default:
 			if (nlflag == 1)
diff --git a/src/parser.h b/src/parser.h
index e6caed6..2875cce 100644
--- a/src/parser.h
+++ b/src/parser.h
@@ -34,6 +34,8 @@
  *	@(#)parser.h	8.3 (Berkeley) 5/4/95
  */
 
+#include "token.h"
+
 /* control characters in argument strings */
 #define CTL_FIRST -127		/* first 'special' character */
 #define CTLESC -127		/* escape next character */
@@ -73,6 +75,7 @@
  * must be distinct from NULL, so we use the address of a variable that
  * happens to be handy.
  */
+extern int lasttoken;
 extern int tokpushback;
 #define NEOF ((union node *)&tokpushback)
 extern int whichprompt;		/* 1 == PS1, 2 == PS2 */
@@ -91,3 +94,8 @@ goodname(const char *p)
 {
 	return !*endofname(p);
 }
+
+static inline int parser_eof(void)
+{
+	return tokpushback && lasttoken == TEOF;
+}

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork

[Jonathan Nieder]

Herbert Xu wrote:

> OK, what about this patch?

Neat.  Let's see:

> --- a/src/parser.c
> +++ b/src/parser.c
[...]
> @@ -210,6 +210,7 @@ list(int nlflag)
>  				parseheredoc();
>  			else
>  				pungetc();		/* push back EOF on input */
> +			tokpushback++;
>  			return n1;
>  		default:
>  			if (nlflag == 1)

This means to push back the TEOF instead of calling pgetc again and
again.  Should be safe.

By the way, is the pungetc() call needed?  I tried to provoke
misbehavior using here documents and reading from the terminal but
didn't manage to come up with a relevant scenario.

> --- a/src/parser.h
> +++ b/src/parser.h
> @@ -34,6 +34,8 @@
>   *	@(#)parser.h	8.3 (Berkeley) 5/4/95
>   */
>  
> +#include "token.h"

mksyntax #include-s parser.h, so after a "make clean":

	gcc -include ../config.h -DBSD=1 -DSHELL -DIFS_BROKEN  -g -Os -Wall -o mksyntax mksyntax.c
	In file included from mksyntax.c:43:0:
	parser.h:37:19: fatal error: token.h: No such file or directory

The following (on top) fixes it here.
---
 src/parser.c |    1 +
 src/parser.h |   11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/parser.c b/src/parser.c
index 6de27629..9c0ef606 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -40,6 +40,7 @@
 
 #include "shell.h"
 #include "parser.h"
+#include "token.h"
 #include "nodes.h"
 #include "expand.h"	/* defines rmescapes() */
 #include "exec.h"	/* defines find_builtin() */
diff --git a/src/parser.h b/src/parser.h
index 2875cce6..8735890e 100644
--- a/src/parser.h
+++ b/src/parser.h
@@ -34,8 +34,6 @@
  *	@(#)parser.h	8.3 (Berkeley) 5/4/95
  */
 
-#include "token.h"

Re: [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork

[Herbert Xu]

On Thu, Jul 07, 2011 at 02:48:56AM -0500, Jonathan Nieder wrote:
>
> By the way, is the pungetc() call needed?  I tried to provoke
> misbehavior using here documents and reading from the terminal but
> didn't manage to come up with a relevant scenario.

It's probably not needed.  I didn't touch it in order to minimise
the changes.

> mksyntax #include-s parser.h, so after a "make clean":
> 
> 	gcc -include ../config.h -DBSD=1 -DSHELL -DIFS_BROKEN  -g -Os -Wall -o mksyntax mksyntax.c
> 	In file included from mksyntax.c:43:0:
> 	parser.h:37:19: fatal error: token.h: No such file or directory
> 
> The following (on top) fixes it here.

Oops, does this patch fix it?

diff --git a/src/Makefile.am b/src/Makefile.am
index 05ed70a..a552087 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -12,7 +12,8 @@ AM_CFLAGS_FOR_BUILD = -g -O2 $(COMMON_CFLAGS)
 AM_CPPFLAGS_FOR_BUILD = $(COMMON_CPPFLAGS)
 
 COMPILE_FOR_BUILD = \
-	$(CC_FOR_BUILD) $(AM_CPPFLAGS_FOR_BUILD) $(CPPFLAGS_FOR_BUILD) \
+	$(CC_FOR_BUILD) $(DEFAULT_INCLUDES) $(AM_CPPFLAGS_FOR_BUILD) \
+	$(CPPFLAGS_FOR_BUILD) \
 	$(AM_CFLAGS_FOR_BUILD) $(CFLAGS_FOR_BUILD) 
 
 bin_PROGRAMS = dash
@@ -31,12 +32,12 @@ dash_SOURCES = \
 	show.h system.h trap.h var.h
 dash_LDADD = builtins.o init.o nodes.o signames.o syntax.o
 
-HELPERS = mkinit mksyntax mknodes mksignames
+HELPERS = mkinit mknodes mksignames
 
 BUILT_SOURCES = builtins.h nodes.h syntax.h token.h token_vars.h
 CLEANFILES = \
 	$(BUILT_SOURCES) $(patsubst %.o,%.c,$(dash_LDADD)) \
-	$(HELPERS) builtins.def
+	$(HELPERS) mksyntax builtins.def
 
 man_MANS = dash.1
 EXTRA_DIST = \
@@ -65,5 +66,8 @@ syntax.c syntax.h: mksyntax
 signames.c: mksignames
 	./$^
 
+mksyntax: mksyntax.c token.h
+	$(COMPILE_FOR_BUILD) -o $@ $<
+
 $(HELPERS): %: %.c
 	$(COMPILE_FOR_BUILD) -o $@ $<

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork

[Jonathan Nieder]

Herbert Xu wrote:
> On Thu, Jul 07, 2011 at 02:48:56AM -0500, Jonathan Nieder wrote:

>> mksyntax #include-s parser.h, so after a "make clean":
>> 
>> 	gcc -include ../config.h -DBSD=1 -DSHELL -DIFS_BROKEN  -g -Os -Wall -o mksyntax mksyntax.c
>> 	In file included from mksyntax.c:43:0:
>> 	parser.h:37:19: fatal error: token.h: No such file or directory
>> 
>> The following (on top) fixes it here.
>
> Oops, does this patch fix it?

Yes, thanks!  Silly me, mistaking mksyntax for mktokens...

The following works, too.
---
diff --git i/src/Makefile.am w/src/Makefile.am
index 05ed70a1..de193dd4 100644
--- i/src/Makefile.am
+++ w/src/Makefile.am
@@ -65,5 +65,7 @@ syntax.c syntax.h: mksyntax
 signames.c: mksignames
 	./$^
 
+mksyntax: token.h
+
 $(HELPERS): %: %.c
 	$(COMPILE_FOR_BUILD) -o $@ $<
-- 

Re: [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork

[Herbert Xu]

On Thu, Jul 07, 2011 at 03:37:03AM -0500, Jonathan Nieder wrote:
> Herbert Xu wrote:
> > On Thu, Jul 07, 2011 at 02:48:56AM -0500, Jonathan Nieder wrote:
> 
> >> mksyntax #include-s parser.h, so after a "make clean":
> >> 
> >> 	gcc -include ../config.h -DBSD=1 -DSHELL -DIFS_BROKEN  -g -Os -Wall -o mksyntax mksyntax.c
> >> 	In file included from mksyntax.c:43:0:
> >> 	parser.h:37:19: fatal error: token.h: No such file or directory
> >> 
> >> The following (on top) fixes it here.
> >
> > Oops, does this patch fix it?
> 
> Yes, thanks!  Silly me, mistaking mksyntax for mktokens...
> 
> The following works, too.
> ---
> diff --git i/src/Makefile.am w/src/Makefile.am
> index 05ed70a1..de193dd4 100644
> --- i/src/Makefile.am
> +++ w/src/Makefile.am
> @@ -65,5 +65,7 @@ syntax.c syntax.h: mksyntax
>  signames.c: mksignames
>  	./$^
>  
> +mksyntax: token.h
> +
>  $(HELPERS): %: %.c
>  	$(COMPILE_FOR_BUILD) -o $@ $<

Thanks, I'll kill the redundant command.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH 5/4] [EVAL] Remove unused EV_BACKCMD flag

[Jonathan Nieder]

The original ash defered forking commands in backquotes so builtins
could be run in the same context as the shell.  This behavior was
controlled using the EV_BACKCMD to evaltree.

Unfortunately, as Matthias Scheler noticed in 1999 (NetBSD PR/7814),
the result was counterintuitive; for example, echo "`cd /`" would
change the cwd.  So ash 0.3.5 left out that optimization.  The
EV_BACKCMD codepath stayed around, unused.

Some time between ash 0.3.5-11 and ash 0.3.8-37, Debian ash omitted
the EV_BACKCMD pathway by guarding it with #ifdef notyet.  In dash
0.5.1 and later, the commented code is no more.  Let's finish the job
and remove the last vestiges.  If someone wants to work on omitting
the fork in backcmd, the remaining hints are not going to be very
helpful, anyway.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thanks for reading.  Thoughts?

The new sizes in the cover letter are overestimated by 16 bytes.
Sorry about that.

 src/eval.c |   63 ++++++++++++++++++-----------------------------------------
 src/eval.h |    1 -
 2 files changed, 19 insertions(+), 45 deletions(-)

diff --git a/src/eval.c b/src/eval.c
index 77a9d00..120e186 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -592,6 +592,9 @@ evalpipe(union node *n, int flags)
 void
 evalbackcmd(union node *n, struct backcmd *result)
 {
+	int pip[2];
+	struct job *jp;
+
 	result->fd = -1;
 	result->buf = NULL;
 	result->nleft = 0;
@@ -600,52 +603,24 @@ evalbackcmd(union node *n, struct backcmd *result)
 		goto out;
 	}
 
-#ifdef notyet
-	/*
-	 * For now we disable executing builtins in the same
-	 * context as the shell, because we are not keeping
-	 * enough state to recover from changes that are
-	 * supposed only to affect subshells. eg. echo "`cd /`"
-	 */
-	if (n->type == NCMD) {
-		struct ifsregion saveifs;
-		struct ifsregion *savelastp;
-		struct nodelist *saveargbackq;
-
-		saveifs = ifsfirst;
-		savelastp = ifslastp;
-		saveargbackq = argbackq;
-
-		exitstatus = oexitstatus;
-		evalcommand(n, EV_BACKCMD, result);
-
-		ifsfirst = saveifs;
-		ifslastp = savelastp;
-		argbackq = saveargbackq;
-	} else
-#endif
-	{
-		int pip[2];
-		struct job *jp;
-
-		if (pipe(pip) < 0)
-			sh_error("Pipe call failed");
-		jp = makejob(n, 1);
-		if (forkshell(jp, n, FORK_NOJOB) == 0) {
-			FORCEINTON;
-			close(pip[0]);
-			if (pip[1] != 1) {
-				dup2(pip[1], 1);
-				close(pip[1]);
-			}
-			ifsfree();
-			evaltreenr(n, EV_EXIT);
-			/* NOTREACHED */
+	if (pipe(pip) < 0)
+		sh_error("Pipe call failed");
+	jp = makejob(n, 1);
+	if (forkshell(jp, n, FORK_NOJOB) == 0) {
+		FORCEINTON;
+		close(pip[0]);
+		if (pip[1] != 1) {
+			dup2(pip[1], 1);
+			close(pip[1]);
 		}
-		close(pip[1]);
-		result->fd = pip[0];
-		result->jp = jp;
+		ifsfree();
+		evaltreenr(n, EV_EXIT);
+		/* NOTREACHED */
 	}
+	close(pip[1]);
+	result->fd = pip[0];
+	result->jp = jp;
+
 out:
 	TRACE(("evalbackcmd done: fd=%d buf=0x%x nleft=%d jp=0x%x\n",
 		result->fd, result->buf, result->nleft, result->jp));
diff --git a/src/eval.h b/src/eval.h
index b1b13f5..4e4de30 100644
--- a/src/eval.h
+++ b/src/eval.h
@@ -49,7 +49,6 @@ struct backcmd {		/* result of evalbackcmd */
 /* flags in argument to evaltree */
 #define EV_EXIT 01		/* exit after evaluating tree */
 #define EV_TESTED 02		/* exit status is checked; ignore -e flag */
-#define EV_BACKCMD 04		/* command executing within back quotes */
 
 int evalstring(char *, int);
 union node;	/* BLETCH for ansi C */
-- 
1.7.5.rc0

Re: [PATCH 5/4] [EVAL] Remove unused EV_BACKCMD flag

[Herbert Xu]

On Sun, Apr 10, 2011 at 07:38:37AM +0000, Jonathan Nieder wrote:
> The original ash defered forking commands in backquotes so builtins
> could be run in the same context as the shell.  This behavior was
> controlled using the EV_BACKCMD to evaltree.
> 
> Unfortunately, as Matthias Scheler noticed in 1999 (NetBSD PR/7814),
> the result was counterintuitive; for example, echo "`cd /`" would
> change the cwd.  So ash 0.3.5 left out that optimization.  The
> EV_BACKCMD codepath stayed around, unused.
> 
> Some time between ash 0.3.5-11 and ash 0.3.8-37, Debian ash omitted
> the EV_BACKCMD pathway by guarding it with #ifdef notyet.  In dash
> 0.5.1 and later, the commented code is no more.  Let's finish the job
> and remove the last vestiges.  If someone wants to work on omitting
> the fork in backcmd, the remaining hints are not going to be very
> helpful, anyway.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Patch applied.  Thanks a lot!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c

[Herbert Xu]

On Sun, Apr 10, 2011 at 07:18:17AM +0000, Jonathan Nieder wrote:
> Hi,
> 
> Jilles Tjoelker wrote[0]:
> 
> > Regarding sh -c optimization, I am in favour of this. Uselessly waiting
> > 'sh -c' processes annoy me. I made the change for FreeBSD 8.0 sh, which
> > is very similar to dash. The SVN changeset is r194128.
> 
> So I grabbed that changeset with
> 
> 	svn log -v svn://svn.freebsd.org/base/head/bin/sh -r 194128
> 	svn diff -r 194127:194128 svn://svn.freebsd.org/base/head/bin/sh
> 
> and made it a tiny bit smaller.   Here's the result.
> 
>    text    data     bss     dec     hex filename
>   83994    1784   11128   96906   17a8a dash.before-O2
>   83994    1784   11128   96906   17a8a dash.before-Os
>   84146    1784   11128   97058   17b22 dash.after-O2
>   84146    1784   11128   97058   17b22 dash.after-Os
> 
> On this amd64 the cost is 152 bytes of text.  Thoughts?

I must say that I don't see much value in this feature.  Adding
exec to the invocation is trivial.

Having said that, I will review the patches to see if they make
sense individually.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c

[Jilles Tjoelker]

On Fri, Apr 15, 2011 at 09:07:09PM +0800, Herbert Xu wrote:
> On Sun, Apr 10, 2011 at 07:18:17AM +0000, Jonathan Nieder wrote:
> > Jilles Tjoelker wrote[0]:

> > > Regarding sh -c optimization, I am in favour of this. Uselessly waiting
> > > 'sh -c' processes annoy me. I made the change for FreeBSD 8.0 sh, which
> > > is very similar to dash. The SVN changeset is r194128.

> > So I grabbed that changeset with

> > 	svn log -v svn://svn.freebsd.org/base/head/bin/sh -r 194128
> > 	svn diff -r 194127:194128 svn://svn.freebsd.org/base/head/bin/sh

> > and made it a tiny bit smaller.   Here's the result.

> >    text    data     bss     dec     hex filename
> >   83994    1784   11128   96906   17a8a dash.before-O2
> >   83994    1784   11128   96906   17a8a dash.before-Os
> >   84146    1784   11128   97058   17b22 dash.after-O2
> >   84146    1784   11128   97058   17b22 dash.after-Os

> > On this amd64 the cost is 152 bytes of text.  Thoughts?

> I must say that I don't see much value in this feature.  Adding
> exec to the invocation is trivial.

It is trivial when writing command lines that are obviously going to be
passed to sh -c, but in practice it is often not done. The optimization
would be useful with system(), popen() and Makefiles; rarely does one
see an "exec" in such contexts. In a Makefile, "exec" can be actively
detrimental since it usually forces the command to be run using the
shell, preventing a direct execve() by make.

In all contexts, "exec" is detrimental if a builtin version of the
executed utility exists. If the utility is a special builtin, prepending
"exec" is very likely to cause the command to stop working, and
otherwise it adds a useless execve().

It was proposed to add text encouraging "exec" prepending to POSIX, but
this was rejected. See http://austingroupbugs.net/view.php?id=236 and
http://thread.gmane.org/gmane.comp.standards.posix.austin.general/1918 .

-- 
Jilles Tjoelker