* [PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c
@ 2011-04-10 7:18 Jonathan Nieder
2011-04-10 7:21 ` [PATCH 1/4] [INPUT] Introduce preadateof predicate to check for end of input Jonathan Nieder
` (5 more replies)
0 siblings, 6 replies; 17+ messages in thread
From: Jonathan Nieder @ 2011-04-10 7:18 UTC (permalink / raw)
To: dash; +Cc: Jilles Tjoelker, Drake Wilson, Reuben Thomas
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(-)
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/4] [INPUT] Introduce preadateof predicate to check for end of input 2011-04-10 7:18 [PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c Jonathan Nieder @ 2011-04-10 7:21 ` Jonathan Nieder 2011-04-10 7:22 ` [PATCH 2/4] [EVAL] Make eval flags public Jonathan Nieder ` (4 subsequent siblings) 5 siblings, 0 replies; 17+ messages in thread From: Jonathan Nieder @ 2011-04-10 7:21 UTC (permalink / raw) To: dash; +Cc: Jilles Tjoelker, Drake Wilson, Reuben Thomas 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/4] [EVAL] Make eval flags public 2011-04-10 7:18 [PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c Jonathan Nieder 2011-04-10 7:21 ` [PATCH 1/4] [INPUT] Introduce preadateof predicate to check for end of input Jonathan Nieder @ 2011-04-10 7:22 ` Jonathan Nieder 2011-04-10 7:35 ` [PATCH 3/4] [EVAL] Take advantage of EV_EXIT in evalstring Jonathan Nieder ` (3 subsequent siblings) 5 siblings, 0 replies; 17+ messages in thread From: Jonathan Nieder @ 2011-04-10 7:22 UTC (permalink / raw) To: dash; +Cc: Jilles Tjoelker, Drake Wilson, Reuben Thomas 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 */ ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/4] [EVAL] Take advantage of EV_EXIT in evalstring 2011-04-10 7:18 [PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c Jonathan Nieder 2011-04-10 7:21 ` [PATCH 1/4] [INPUT] Introduce preadateof predicate to check for end of input Jonathan Nieder 2011-04-10 7:22 ` [PATCH 2/4] [EVAL] Make eval flags public Jonathan Nieder @ 2011-04-10 7:35 ` Jonathan Nieder 2011-04-10 7:36 ` [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork Jonathan Nieder ` (2 subsequent siblings) 5 siblings, 0 replies; 17+ messages in thread From: Jonathan Nieder @ 2011-04-10 7:35 UTC (permalink / raw) To: dash; +Cc: Jilles Tjoelker, Drake Wilson, Reuben Thomas 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork 2011-04-10 7:18 [PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c Jonathan Nieder ` (2 preceding siblings ...) 2011-04-10 7:35 ` [PATCH 3/4] [EVAL] Take advantage of EV_EXIT in evalstring Jonathan Nieder @ 2011-04-10 7:36 ` Jonathan Nieder 2011-07-07 3:48 ` Herbert Xu 2011-04-10 7:38 ` [PATCH 5/4] [EVAL] Remove unused EV_BACKCMD flag Jonathan Nieder 2011-04-15 13:07 ` [PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c Herbert Xu 5 siblings, 1 reply; 17+ messages in thread From: Jonathan Nieder @ 2011-04-10 7:36 UTC (permalink / raw) To: dash; +Cc: Jilles Tjoelker, Drake Wilson, Reuben Thomas 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork 2011-04-10 7:36 ` [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork Jonathan Nieder @ 2011-07-07 3:48 ` Herbert Xu 2011-07-07 4:27 ` Jonathan Nieder 0 siblings, 1 reply; 17+ messages in thread From: Herbert Xu @ 2011-07-07 3:48 UTC (permalink / raw) To: Jonathan Nieder; +Cc: dash, Jilles Tjoelker, Drake Wilson, Reuben Thomas 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork 2011-07-07 3:48 ` Herbert Xu @ 2011-07-07 4:27 ` Jonathan Nieder 2011-07-07 4:57 ` Herbert Xu 0 siblings, 1 reply; 17+ messages in thread From: Jonathan Nieder @ 2011-07-07 4:27 UTC (permalink / raw) To: Herbert Xu; +Cc: dash, Jilles Tjoelker, Drake Wilson, Reuben Thomas 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork 2011-07-07 4:27 ` Jonathan Nieder @ 2011-07-07 4:57 ` Herbert Xu 2011-07-07 5:56 ` Herbert Xu 0 siblings, 1 reply; 17+ messages in thread From: Herbert Xu @ 2011-07-07 4:57 UTC (permalink / raw) To: Jonathan Nieder; +Cc: dash, Jilles Tjoelker, Drake Wilson, Reuben Thomas 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork 2011-07-07 4:57 ` Herbert Xu @ 2011-07-07 5:56 ` Herbert Xu 2011-07-07 7:48 ` Jonathan Nieder 0 siblings, 1 reply; 17+ messages in thread From: Herbert Xu @ 2011-07-07 5:56 UTC (permalink / raw) To: Jonathan Nieder; +Cc: dash, Jilles Tjoelker, Drake Wilson, Reuben Thomas 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork 2011-07-07 5:56 ` Herbert Xu @ 2011-07-07 7:48 ` Jonathan Nieder 2011-07-07 8:22 ` Herbert Xu 0 siblings, 1 reply; 17+ messages in thread From: Jonathan Nieder @ 2011-07-07 7:48 UTC (permalink / raw) To: Herbert Xu; +Cc: dash, Jilles Tjoelker, Drake Wilson, Reuben Thomas 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" ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork 2011-07-07 7:48 ` Jonathan Nieder @ 2011-07-07 8:22 ` Herbert Xu 2011-07-07 8:37 ` Jonathan Nieder 0 siblings, 1 reply; 17+ messages in thread From: Herbert Xu @ 2011-07-07 8:22 UTC (permalink / raw) To: Jonathan Nieder; +Cc: dash, Jilles Tjoelker, Drake Wilson, Reuben Thomas 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork 2011-07-07 8:22 ` Herbert Xu @ 2011-07-07 8:37 ` Jonathan Nieder 2011-07-07 8:39 ` Herbert Xu 0 siblings, 1 reply; 17+ messages in thread From: Jonathan Nieder @ 2011-07-07 8:37 UTC (permalink / raw) To: Herbert Xu; +Cc: dash, Jilles Tjoelker, Drake Wilson, Reuben Thomas 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 $@ $< -- ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork 2011-07-07 8:37 ` Jonathan Nieder @ 2011-07-07 8:39 ` Herbert Xu 0 siblings, 0 replies; 17+ messages in thread From: Herbert Xu @ 2011-07-07 8:39 UTC (permalink / raw) To: Jonathan Nieder; +Cc: dash, Jilles Tjoelker, Drake Wilson, Reuben Thomas 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/4] [EVAL] Remove unused EV_BACKCMD flag 2011-04-10 7:18 [PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c Jonathan Nieder ` (3 preceding siblings ...) 2011-04-10 7:36 ` [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork Jonathan Nieder @ 2011-04-10 7:38 ` Jonathan Nieder 2011-07-07 3:56 ` Herbert Xu 2011-04-15 13:07 ` [PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c Herbert Xu 5 siblings, 1 reply; 17+ messages in thread From: Jonathan Nieder @ 2011-04-10 7:38 UTC (permalink / raw) To: dash; +Cc: Jilles Tjoelker, Drake Wilson, Reuben Thomas 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 5/4] [EVAL] Remove unused EV_BACKCMD flag 2011-04-10 7:38 ` [PATCH 5/4] [EVAL] Remove unused EV_BACKCMD flag Jonathan Nieder @ 2011-07-07 3:56 ` Herbert Xu 0 siblings, 0 replies; 17+ messages in thread From: Herbert Xu @ 2011-07-07 3:56 UTC (permalink / raw) To: Jonathan Nieder; +Cc: dash, Jilles Tjoelker, Drake Wilson, Reuben Thomas 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c 2011-04-10 7:18 [PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c Jonathan Nieder ` (4 preceding siblings ...) 2011-04-10 7:38 ` [PATCH 5/4] [EVAL] Remove unused EV_BACKCMD flag Jonathan Nieder @ 2011-04-15 13:07 ` Herbert Xu 2011-04-17 22:13 ` Jilles Tjoelker 5 siblings, 1 reply; 17+ messages in thread From: Herbert Xu @ 2011-04-15 13:07 UTC (permalink / raw) To: Jonathan Nieder; +Cc: dash, Jilles Tjoelker, Drake Wilson, Reuben Thomas 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c 2011-04-15 13:07 ` [PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c Herbert Xu @ 2011-04-17 22:13 ` Jilles Tjoelker 0 siblings, 0 replies; 17+ messages in thread From: Jilles Tjoelker @ 2011-04-17 22:13 UTC (permalink / raw) To: Herbert Xu; +Cc: Jonathan Nieder, dash, Drake Wilson, Reuben Thomas 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2011-07-07 8:39 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-04-10 7:18 [PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c Jonathan Nieder 2011-04-10 7:21 ` [PATCH 1/4] [INPUT] Introduce preadateof predicate to check for end of input Jonathan Nieder 2011-04-10 7:22 ` [PATCH 2/4] [EVAL] Make eval flags public Jonathan Nieder 2011-04-10 7:35 ` [PATCH 3/4] [EVAL] Take advantage of EV_EXIT in evalstring Jonathan Nieder 2011-04-10 7:36 ` [PATCH 4/4] [MAIN] Optimize dash -c "command" to avoid a fork Jonathan Nieder 2011-07-07 3:48 ` Herbert Xu 2011-07-07 4:27 ` Jonathan Nieder 2011-07-07 4:57 ` Herbert Xu 2011-07-07 5:56 ` Herbert Xu 2011-07-07 7:48 ` Jonathan Nieder 2011-07-07 8:22 ` Herbert Xu 2011-07-07 8:37 ` Jonathan Nieder 2011-07-07 8:39 ` Herbert Xu 2011-04-10 7:38 ` [PATCH 5/4] [EVAL] Remove unused EV_BACKCMD flag Jonathan Nieder 2011-07-07 3:56 ` Herbert Xu 2011-04-15 13:07 ` [PATCH/RFC dash 0/4] Avoid a fork before running last command given to -c Herbert Xu 2011-04-17 22:13 ` Jilles Tjoelker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).