Skip to contentSkip to conversation

Avoid a fork before running last command given to -c

PATCH In review Jonathan Nieder 3 participants 17 messages Apr 10, 2011 – Jul 7, 2011

lore
Share & tools
lore
#0 lore raw

cover Avoid a fork before running last command given to -c

Hi,

Jilles Tjoelker wrote[0]:

Quoted 3 lines

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(-)
#1 lore raw

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

From: Jilles Tjoelker <[email protected]>
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]

---
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(-)
src/input.c +17 -0
index d31c45b..39d7893 100644
@@ -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.
  */
src/input.h +1 -0
index 50a7797..d123045 100644
@@ -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);
Signature · 1 line
1.7.5.rc0
#2 lore raw

PATCH 2/4 Make eval flags public

In reply to Jonathan Nieder

From: Jilles Tjoelker <[email protected]>
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.]

---
 src/eval.c |    5 -----
 src/eval.h |    5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)
src/eval.c +0 -4
index 426c03a..5c26133 100644
@@ -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 */
#3 lore raw

PATCH 3/4 Take advantage of EV_EXIT in evalstring

In reply to Jonathan Nieder

From: Jilles Tjoelker <[email protected]>
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.]

---
 src/eval.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
src/eval.c +4 -1
index 5c26133..77a9d00 100644
@@ -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)
Signature · 1 line
1.7.5.rc0
#4 lore raw

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

In reply to Jonathan Nieder

From: Jilles Tjoelker <[email protected]>
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.]

---
 src/main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
src/main.c +1 -1
index b38dc27..af987c6 100644
@@ -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 */
Signature · 1 line
1.7.5.rc0
#5 lore raw

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

Quoted 14 lines · Jonathan Nieder

From: Jilles Tjoelker <[email protected]>
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.

Quoted 7 lines

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.]

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,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#6 lore raw

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

Quoted 11 lines · Herbert Xu
Quoted 9 lines · Jonathan Nieder

From: Jilles Tjoelker <[email protected]>
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.

[...]

Quoted 3 lines

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
#7 lore raw

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

Quoted 7 lines · Jonathan Nieder

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,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#8 lore raw

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

Quoted 10 lines · Herbert Xu
Quoted 7 lines · Jonathan Nieder

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?

ChangeLog +4 -0
index 7367c33..c457fc8 100644
@@ -1,3 +1,7 @@
+2011-07-07  Herbert Xu <[email protected]>
+
+	* Optimize dash -c "command" to avoid a fork.
+
 2011-04-10  Jonathan Nieder <[email protected]>
  
 	* Remove unused EV_BACKCMD flag.
src/Makefile.am +2 -2
index ba68d55..05ed70a 100644
@@ -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
src/eval.c +1 -5
index 86423b4..6e7b932 100644
@@ -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)
src/eval.h +4 -0
index ac394e8..4e4de30 100644
@@ -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);
src/main.c +1 -1
index b38dc27..af987c6 100644
@@ -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 */
src/mktokens +6 -3
index 7c873af..cd52241 100644
@@ -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};"}'
 
src/parser.c +3 -2
index 528d005..6de2762 100644
@@ -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)
src/parser.h +8 -0
index e6caed6..2875cce 100644
@@ -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,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#9 lore raw

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

Quoted 1 line · Herbert Xu

OK, what about this patch?

Neat. Let's see:

Quoted 2 lines
src/parser.c +0 -0

[...]

src/parser.c list(int nlflag) · line 216

Quoted 8 lines
src/parser.c +1 -0
@@ -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.

src/parser.h line 37

Quoted 7 lines
src/parser.h +1 -0
@@ -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(-)
src/parser.c +1 -0
index 6de27629..9c0ef606 100644
@@ -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() */
src/parser.h +0 -1
index 2875cce6..8735890e 100644
@@ -34,8 +34,6 @@
  *	@(#)parser.h	8.3 (Berkeley) 5/4/95
  */
 
-#include "token.h"
#10 lore raw

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

Quoted 4 lines · Jonathan Nieder

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.

Quoted 7 lines

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?

src/Makefile.am +7 -3
index 05ed70a..a552087 100644
@@ -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,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#11 lore raw

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

Quoted 11 lines · Herbert Xu
Quoted 7 lines · Jonathan Nieder

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.
---

src/Makefile.am +2 -0
index 05ed70a1..de193dd4 100644
@@ -65,5 +65,7 @@ syntax.c syntax.h: mksyntax
 signames.c: mksignames
 	./$^
 
+mksyntax: token.h
+
 $(HELPERS): %: %.c
 	$(COMPILE_FOR_BUILD) -o $@ $<
#12 lore raw

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

Quoted 22 lines · Jonathan Nieder
Quoted 11 lines · Herbert Xu
Quoted 7 lines · Jonathan Nieder

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.
---

src/Makefile.am +0 -0
index 05ed70a1..de193dd4 100644

src/Makefile.am syntax.c syntax.h: mksyntax · line 71

Quoted 7 lines
src/Makefile.am +2 -0
@@ -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.

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#13 lore raw

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

In reply to 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.

---
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(-)
src/eval.c +19 -44
index 77a9d00..120e186 100644
@@ -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));
src/eval.h +0 -1
index b1b13f5..4e4de30 100644
@@ -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 */
Signature · 1 line
1.7.5.rc0
#14 lore raw

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

Quoted 9 lines · 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.

Quoted 8 lines

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.

Patch applied. Thanks a lot!

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#15 lore raw
In reply to Jonathan Nieder
Quoted 15 lines · Jonathan Nieder

Hi,

Jilles Tjoelker wrote[0]:

Quoted 3 lines

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.

Quoted 7 lines
   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,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#16 lore raw
Quoted 20 lines · Herbert Xu
Quoted 18 lines · Jonathan Nieder

Jilles Tjoelker wrote[0]:

Quoted 3 lines

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
Quoted 4 lines
Quoted 1 line

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 .

Signature · 1 line
Jilles Tjoelker