Avoid a fork before running last command given to -c
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(-)
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
@@ -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
@@ -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
PATCH 2/4 Make eval flags public
In reply to Jonathan NiederFrom: 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
@@ -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 Take advantage of EV_EXIT in evalstring
In reply to Jonathan NiederFrom: 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
@@ -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
PATCH 4/4 Optimize dash -c "command" to avoid a fork
In reply to Jonathan NiederFrom: 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
@@ -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
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 -0500This change only affects strings passed to -c, when the -s option is not used.
Use the
EV_EXITflag 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.]
Signed-off-by Jonathan Nieder <[email protected]>
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
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 -0500This change only affects strings passed to -c, when the -s option is not used.
Use the
EV_EXITflag 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
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_EXITto 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
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_EXITto evalstring would break:$ dash -c 'echo one echo two' oneYou're right. I'll back this out for now.
OK, what about this patch?
ChangeLog +4 -0
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 (); elsepungetc ();/* push back EOF on input */ + tokpushback++;return n1;default :if (nlflag ==1 )
src/parser.h +8 -0
@@ -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
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 (); elsepungetc ();/* 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
@@ -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
@@ -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
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 mksyntaxmksyntax.cIn file included from mksyntax.c:43:0:
parser.h:37:19: fatal error:token.h: No such file or directoryThe following (on top) fixes it here.
Oops, does this patch fix it?
src/Makefile.am +7 -3
@@ -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
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 mksyntaxmksyntax.cIn file included from mksyntax.c:43:0:
parser.h:37:19: fatal error:token.h: No such file or directoryThe 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
@@ -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
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 mksyntaxmksyntax.cIn file included from mksyntax.c:43:0:
parser.h:37:19: fatal error:token.h: No such file or directoryThe 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
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
[PATCH 5/4] [EVAL] Remove unused EV_BACKCMD flag
In reply to Jonathan NiederThe 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
@@ -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
@@ -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
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_BACKCMDto 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. TheEV_BACKCMDcodepath 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_BACKCMDpathway 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 <[email protected]>
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
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/shand 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-OsOn 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
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/shand 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