Re: Strange parameter visibility

Re: Strange parameter visibility

[Bart Schaefer]

[> workers]

On Sat, Sep 17, 2016 at 9:37 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Sat, Sep 17, 2016 at 6:42 PM, Eric Cook <llua@gmx.com> wrote:
>>
>> It's a normal assignment just like the x=1. the weirdness is zsh
>> optimizing out a fork in the pipeline, so the scope of the
>> reassignment wasn't just a subshell.
>
> I have to conclude that's a bug -- it's OK to optimize out the fork,
> but not to optimize out its side effects (or cause new ones).

The addvars() that does this is exec.c:2847.  I tried copying the
save_/restore_params() wrapper from the nullexec branch near line 3455
(using !forked instead of POSIXBUILTINS as the test), but that results
in core dumps when running "make check".  Anybody with a better idea
what's going on here?

Re: Strange parameter visibility

[Peter Stephenson]

On Sat, 17 Sep 2016 20:11:37 +0200
Kamil Jońca <kjonca@o2.pl> wrote:
> --8<---------------cut here---------------start------------->8---
> %(x=1; x=2 | echo $x ; echo $x) 
> 2
> 2
> --8<---------------cut here---------------end--------------->8---
> 
> What there are "2"-s in second example?

Try this.

This isn't an optimisation gone awry --- we just plain miss out on
checking that the LHS of the pipeline is supposed to run in the shell so
we need to fork to do that, as we do in other cases.  It's obviously
never come up because the assigment is redundant, or should be.

We might be able to renumber the WC_ codes to make this more efficient.

pws

diff --git a/Src/exec.c b/Src/exec.c
index c79a278..e3915dd 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1844,7 +1844,8 @@ execpline2(Estate state, wordcode pcode,
 	/* if we are doing "foo | bar" where foo is a current *
 	 * shell command, do foo in a subshell and do the     *
 	 * rest of the pipeline in the current shell.         */
-	if (wc_code(code) >= WC_CURSH && (how & Z_SYNC)) {
+	if ((wc_code(code) >= WC_CURSH || wc_code(code) == WC_ASSIGN)
+	    && (how & Z_SYNC)) {
 	    int synch[2];
 	    struct timeval bgtime;
 
diff --git a/Test/A01grammar.ztst b/Test/A01grammar.ztst
index 921ff99..0b1085c 100644
--- a/Test/A01grammar.ztst
+++ b/Test/A01grammar.ztst
@@ -756,3 +756,10 @@
 >	print Stuff here
 >}
 >Stuff here
+
+  x=1
+  x=2 | echo $x
+  echo $x
+0:Assignment-only current shell commands in LHS of pipelin
+>1
+>1

Re: Strange parameter visibility

[Peter Stephenson]

On Thu, 29 Sep 2016 17:24:17 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> This isn't an optimisation gone awry --- we just plain miss out on
> checking that the LHS of the pipeline is supposed to run in the shell so
> we need to fork to do that, as we do in other cases.

This appears to be harder.

% unset x
% : ${x:=2} | echo $x
2

In this case, we don't know at the point where we start the pipeline
whether we're going to be in the current shell or not.  If the command
is external we want to keep the current code (not sure where this gets
hairy otherwise but it does).

Other cases people find are likely to be similar to this one.

pws

Re: Strange parameter visibility

[Bart Schaefer]

On Sep 29,  6:03pm, Peter Stephenson wrote:
} Subject: Re: Strange parameter visibility
}
} This appears to be harder.
} 
} % unset x
} % : ${x:=2} | echo $x
} 2
} 
} In this case, we don't know at the point where we start the pipeline
} whether we're going to be in the current shell or not.

Hrm.  But:

% y=3 : ${z:=2} | echo $y $z

% 

Why do we know *there* that we should fork before expanding ${z:=2},
when we don't know in the absence of the y=3 prefix?  Or is something
completely different happening, e.g., save_params() / restore_params()
that I was looking at before?

Under what circumstances is it possible to do anything useful with a
pipe without forking the left side?

I tried reducing the test that you changed in 39502 to looking at
nothing but (how & Z_SYNC) and the only Test/* to fail are the ones
looking at $pipestatus and setopt pipefail -- and as it happens I
can make the pipefail test fail by using /bin/true and /bin/false
in place of the builtin true and false, so there may be something
deeper wrong there anyway.

==========
 0
 1
 1
-2
+1
Test ./E01options.ztst failed: output differs from expected as
shown above for:
  (setopt pipefail
  /bin/true | /bin/true | /bin/true
  print $?
  /bin/true | /bin/false | /bin/true
  print $?
  exit 2 | /bin/false | /bin/true
  print $?
  /bin/false | exit 2 | /bin/true
  print $?)
Was testing: PIPE_FAIL option
==========

I can also make the $pipestatus test fail (one time in the 2048 loops
it performs) by using "exit 2" instead of "return 2" on the left sides
of the pipelines.  (Test/A05execution.ztst)

However, I get those failures on only one of the two hosts where I tried.
So these likely are still some kind of race condition.

} If the command is external we want to keep the current code (not sure
} where this gets hairy otherwise but it does).

Whether the command is external doesn't seem to matter, unless you mean
the command on the right-hand side of the pipe.

} Other cases people find are likely to be similar to this one.

Indeed.

Re: Strange parameter visibility

[Bart Schaefer]

On Sep 29,  2:28pm, Bart Schaefer wrote:
}
} can make the pipefail test fail by using /bin/true and /bin/false
} in place of the builtin true and false
} 
} I can also make the $pipestatus test fail (one time in the 2048 loops
} it performs) by using "exit 2" instead of "return 2" on the left sides
} of the pipelines.  (Test/A05execution.ztst)
} 
} However, I get those failures on only one of the two hosts where I tried.
} So these likely are still some kind of race condition.

And now I can't get them reliably even on the host where they first
appeared, so this looks really hard to resolve.  Sigh.

Re: Strange parameter visibility

[Peter Stephenson]

On Thu, 29 Sep 2016 14:28:21 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Sep 29,  6:03pm, Peter Stephenson wrote:
> } Subject: Re: Strange parameter visibility
> } % unset x
> } % : ${x:=2} | echo $x
> } 2
> } 
> } In this case, we don't know at the point where we start the pipeline
> } whether we're going to be in the current shell or not.
> 
> Hrm.  But:
> 
> % y=3 : ${z:=2} | echo $y $z
> 
> % 
> 
> Why do we know *there* that we should fork before expanding ${z:=2},
> when we don't know in the absence of the y=3 prefix?  Or is something
> completely different happening, e.g., save_params() / restore_params()
> that I was looking at before?

That LHS turns up in execpline2() as WC_ASSIGN, which I've just changed
(if you're using the change I put in).

I wonder if that means there are cases where the change is problematic
--- I hadn't realised this also covered assignments before running a
command.  Given your experience elsewhere, you might find this makes
pipestatus worse if you test it on relevant shell code, for example.

> Under what circumstances is it possible to do anything useful with a
> pipe without forking the left side?

The interesting thing --- which goes back into history and I've never
properly got my head round --- is how the fork at that point interacts
with the fork inside execcmd().  If it's a simple command being run
you'd have thought we either fork there or above, and not both, but I'm
not sure about that (and I'll need to check if that just got changed
with WC_ASSIGN with an external command --- if there's a double fork
for an external command I'll need to back that off, or think about
introducing WC_ASSIGN_ONLY).

One clue is the "output" parameter passed into execccmd() that says if
we're talking to a pipe.  That's used in the fork decision with the big
comment around exec.c line 3166: if we're executing a builtin or a shell
function and we're outputting to a pipe, we fork there.  I think the reason
we *don't* fork there for is_cursh is exacxtly because of that test in
execpline2() --- we knew at that point it was shell builtin code, so
didn't need to defer the decision.  We needed to defer the decision if
we hadn't analysed the command to see if it was a builtin or not.  If
it's not a builtin or current shell structure of some sort, we always
fork down in execcmd() (unless exec'ing).

Another ingredient here is the position of prefork(), the clue being in
the name, which is where the ${z:=2} assignment takes place.

If you're not confused, I haven't correctly transferred all the
limited information I do have...

pws

Re: Strange parameter visibility

[Peter Stephenson]

On Fri, 30 Sep 2016 09:50:32 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> I wonder if that means there are cases where the change is problematic
> --- I hadn't realised this also covered assignments before running a
> command.  Given your experience elsewhere, you might find this makes
> pipestatus worse if you test it on relevant shell code, for example.

I've now realised WC_ASSIGN just applies to the invididual assignment
--- it doesn't actually mark the command line at all, there could in
principle be any sort of command after it.  So probably the change is
unsafe and needs to be backed off at least for now.

I'm not sure it's worth scanning down the code and seeing what's next in
excpline2(), though that's probably the right thing to do.

A more robust fix is a complete refactoring of execcmd(), which is far
too big anyway, with the appropriate parts being called form above,
deciding on asignments and analysing the commands early, possibly
allowing there to be a single fork in one place.  However, that's a big
job --- and without extra code to delay prefork() I still don't think
that fixes the case ": ${x:=2} | echo $x", and I suspect the case
"cmd=:; $cmd ${x:=2} | echo $x" is pretty much unfixable without
delaying the assignment from the ${x:=2} until a later part of
processing, which might have side effects.

pws

diff --git a/Src/exec.c b/Src/exec.c
index e253d7b..2714edb 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1844,7 +1844,7 @@ execpline2(Estate state, wordcode pcode,
 	/* if we are doing "foo | bar" where foo is a current *
 	 * shell command, do foo in a subshell and do the     *
 	 * rest of the pipeline in the current shell.         */
-	if ((wc_code(code) >= WC_CURSH || wc_code(code) == WC_ASSIGN)
+	if ((wc_code(code) >= WC_CURSH)
 	    && (how & Z_SYNC)) {
 	    int synch[2];
 	    struct timeval bgtime;
index 0b1085c..1ad73c5 100644
--- a/Test/A01grammar.ztst
+++ b/Test/A01grammar.ztst
@@ -757,9 +757,12 @@
 >}
 >Stuff here
 
-  x=1
-  x=2 | echo $x
-  echo $x
-0:Assignment-only current shell commands in LHS of pipelin
->1
->1
+## This problem is hard to fix without significant changes to how
+## the shell forks for a pipeline.
+#
+#   x=1
+#   x=2 | echo $x
+#   echo $x
+# 0:Assignment-only current shell commands in LHS of pipelin
+# >1
+# >1

Re: Strange parameter visibility

[Peter Stephenson]

On Fri, 30 Sep 2016 10:36:25 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> A more robust fix is a complete refactoring of execcmd(), which is far
> too big anyway, with the appropriate parts being called form above,
> deciding on asignments and analysing the commands early, possibly
> allowing there to be a single fork in one place.  However, that's a big
> job --- and without extra code to delay prefork() I still don't think
> that fixes the case ": ${x:=2} | echo $x", and I suspect the case
> "cmd=:; $cmd ${x:=2} | echo $x" is pretty much unfixable without
> delaying the assignment from the ${x:=2} until a later part of
> processing, which might have side effects.

A start at refactoring, which I think is generally beneficial, already
fixes the original problem robustly with a small tweak: we can now
check within execpline2() that there are no arguments to the command,
so therefore it will be handled entirely within the shell.

This probably needs more exercising, but tests already passed (including
the new one which is restored).

If you thought "but I really liked the huge, monolithic execcmd()" please
keep your thoughts to yourself...

pws

diff --git a/Src/exec.c b/Src/exec.c
index 2714edb..349414c 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1808,6 +1808,7 @@ execpline2(Estate state, wordcode pcode,
 {
     pid_t pid;
     int pipes[2];
+    struct execcmd_params eparams;
 
     if (breaks || retflag)
 	return;
@@ -1825,17 +1826,16 @@ execpline2(Estate state, wordcode pcode,
 	else
 	    list_pipe_text[0] = '\0';
     }
-    if (WC_PIPE_TYPE(pcode) == WC_PIPE_END)
-	execcmd(state, input, output, how, last1 ? 1 : 2);
-    else {
+    if (WC_PIPE_TYPE(pcode) == WC_PIPE_END) {
+	execcmd_analyse(state, &eparams);
+	execcmd_exec(state, &eparams, input, output, how, last1 ? 1 : 2);
+    } else {
 	int old_list_pipe = list_pipe;
 	int subsh_close = -1;
-	Wordcode next = state->pc + (*state->pc), pc;
-	wordcode code;
+	Wordcode next = state->pc + (*state->pc), start_pc;
 
-	state->pc++;
-	for (pc = state->pc; wc_code(code = *pc) == WC_REDIR;
-	     pc += WC_REDIR_WORDS(code));
+	start_pc = ++state->pc;
+	execcmd_analyse(state, &eparams);
 
 	if (mpipe(pipes) < 0) {
 	    /* FIXME */
@@ -1844,7 +1844,7 @@ execpline2(Estate state, wordcode pcode,
 	/* if we are doing "foo | bar" where foo is a current *
 	 * shell command, do foo in a subshell and do the     *
 	 * rest of the pipeline in the current shell.         */
-	if ((wc_code(code) >= WC_CURSH)
+	if ((eparams.type >= WC_CURSH || !eparams.args)
 	    && (how & Z_SYNC)) {
 	    int synch[2];
 	    struct timeval bgtime;
@@ -1863,7 +1863,7 @@ execpline2(Estate state, wordcode pcode,
 	    } else if (pid) {
 		char dummy, *text;
 
-		text = getjobtext(state->prog, state->pc);
+		text = getjobtext(state->prog, start_pc);
 		addproc(pid, text, 0, &bgtime);
 		close(synch[1]);
 		read_loop(synch[0], &dummy, 1);
@@ -1874,14 +1874,14 @@ execpline2(Estate state, wordcode pcode,
 		entersubsh(((how & Z_ASYNC) ? ESUB_ASYNC : 0)
 			   | ESUB_PGRP | ESUB_KEEPTRAP);
 		close(synch[1]);
-		execcmd(state, input, pipes[1], how, 1);
+		execcmd_exec(state, &eparams, input, pipes[1], how, 1);
 		_exit(lastval);
 	    }
 	} else {
 	    /* otherwise just do the pipeline normally. */
 	    addfilelist(NULL, pipes[0]);
 	    subsh_close = pipes[0];
-	    execcmd(state, input, pipes[1], how, 0);
+	    execcmd_exec(state, &eparams, input, pipes[1], how, 0);
 	}
 	zclose(pipes[1]);
 	state->pc = next;
@@ -2537,55 +2537,51 @@ resolvebuiltin(const char *cmdarg, HashNode hn)
     return hn;
 }
 
+/*
+ * We are about to execute a command at the lowest level of the
+ * hierarchy.  Analyse the parameters from the wordcode.
+ */
+
 /**/
 static void
-execcmd(Estate state, int input, int output, int how, int last1)
+execcmd_analyse(Estate state, Execcmd_params eparams)
 {
-    HashNode hn = NULL;
-    LinkList args, filelist = NULL;
-    LinkNode node;
-    Redir fn;
-    struct multio *mfds[10];
-    char *text;
-    int save[10];
-    int fil, dfil, is_cursh, type, do_exec = 0, redir_err = 0, i, htok = 0;
-    int nullexec = 0, assign = 0, forked = 0, postassigns = 0;
-    int is_shfunc = 0, is_builtin = 0, is_exec = 0, use_defpath = 0;
-    /* Various flags to the command. */
-    int cflags = 0, orig_cflags = 0, checked = 0, oautocont = -1;
-    LinkList redir;
     wordcode code;
-    Wordcode beg = state->pc, varspc, assignspc = (Wordcode)0;
-    FILE *oxtrerr = xtrerr, *newxtrerr = NULL;
+    int i;
 
-    doneps4 = 0;
-    redir = (wc_code(*state->pc) == WC_REDIR ? ecgetredirs(state) : NULL);
+    eparams->beg = state->pc;
+    eparams->redir =
+	(wc_code(*state->pc) == WC_REDIR ? ecgetredirs(state) : NULL);
     if (wc_code(*state->pc) == WC_ASSIGN) {
 	cmdoutval = 0;
-	varspc = state->pc;
+	eparams->varspc = state->pc;
 	while (wc_code((code = *state->pc)) == WC_ASSIGN)
 	    state->pc += (WC_ASSIGN_TYPE(code) == WC_ASSIGN_SCALAR ?
 			  3 : WC_ASSIGN_NUM(code) + 2);
     } else
-	varspc = NULL;
+	eparams->varspc = NULL;
 
     code = *state->pc++;
 
-    type = wc_code(code);
+    eparams->type = wc_code(code);
+    eparams->postassigns = 0;
 
     /* It would be nice if we could use EC_DUPTOK instead of EC_DUP here.
      * But for that we would need to check/change all builtins so that
      * they don't modify their argument strings. */
-    switch (type) {
+    switch (eparams->type) {
     case WC_SIMPLE:
-	args = ecgetlist(state, WC_SIMPLE_ARGC(code), EC_DUP, &htok);
+	eparams->args = ecgetlist(state, WC_SIMPLE_ARGC(code), EC_DUP,
+				  &eparams->htok);
+	eparams->assignspc = NULL;
 	break;
 
     case WC_TYPESET:
-	args = ecgetlist(state, WC_TYPESET_ARGC(code), EC_DUP, &htok);
-	postassigns = *state->pc++;
-	assignspc = state->pc;
-	for (i = 0; i < postassigns; i++) {
+	eparams->args = ecgetlist(state, WC_TYPESET_ARGC(code), EC_DUP,
+				  &eparams->htok);
+	eparams->postassigns = *state->pc++;
+	eparams->assignspc = state->pc;
+	for (i = 0; i < eparams->postassigns; i++) {
 	    code = *state->pc;
 	    DPUTS(wc_code(code) != WC_ASSIGN,
 		  "BUG: miscounted typeset assignments");
@@ -2595,8 +2591,45 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	break;
 
     default:
-	args = NULL;
+	eparams->args = NULL;
+	eparams->assignspc = NULL;
+	eparams->htok = 0;
+	break;
     }
+}
+
+/*
+ * Execute a command at the lowest level of the hierarchy.
+ */
+
+/**/
+static void
+execcmd_exec(Estate state, Execcmd_params eparams,
+	     int input, int output, int how, int last1)
+{
+    HashNode hn = NULL;
+    LinkList filelist = NULL;
+    LinkNode node;
+    Redir fn;
+    struct multio *mfds[10];
+    char *text;
+    int save[10];
+    int fil, dfil, is_cursh, do_exec = 0, redir_err = 0, i;
+    int nullexec = 0, assign = 0, forked = 0;
+    int is_shfunc = 0, is_builtin = 0, is_exec = 0, use_defpath = 0;
+    /* Various flags to the command. */
+    int cflags = 0, orig_cflags = 0, checked = 0, oautocont = -1;
+    FILE *oxtrerr = xtrerr, *newxtrerr = NULL;
+    /*
+     * Retrieve parameters for quick reference (they are unique
+     * to us so we can modify the structure if we want).
+     */
+    LinkList args = eparams->args;
+    LinkList redir = eparams->redir;
+    Wordcode varspc = eparams->varspc;
+    int type = eparams->type;
+
+    doneps4 = 0;
 
     /*
      * If assignment but no command get the status from variable
@@ -2854,7 +2887,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
 
     /* Do prefork substitutions */
     esprefork = (assign || isset(MAGICEQUALSUBST)) ? PREFORK_TYPESET : 0;
-    if (args && htok)
+    if (args && eparams->htok)
 	prefork(args, esprefork, NULL);
 
     if (type == WC_SIMPLE || type == WC_TYPESET) {
@@ -2999,7 +3032,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
     /* Get the text associated with this command. */
     if ((how & Z_ASYNC) ||
 	(!sfcontext && !sourcelevel && (jobbing || (how & Z_TIMED))))
-	text = getjobtext(state->prog, beg);
+	text = getjobtext(state->prog, eparams->beg);
     else
 	text = NULL;
 
@@ -3258,7 +3291,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	    forked = 1;
     }
 
-    if ((esglob = !(cflags & BINF_NOGLOB)) && args && htok) {
+    if ((esglob = !(cflags & BINF_NOGLOB)) && args && eparams->htok) {
 	LinkList oargs = args;
 	globlist(args, 0);
 	args = oargs;
@@ -3572,7 +3605,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	}
 	if (type == WC_FUNCDEF) {
 	    Eprog redir_prog;
-	    if (!redir && wc_code(*beg) == WC_REDIR)  {
+	    if (!redir && wc_code(*eparams->beg) == WC_REDIR)  {
 		/*
 		 * We're not using a redirection from the currently
 		 * parsed environment, which is what we'd do for an
@@ -3582,7 +3615,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		struct estate s;
 
 		s.prog = state->prog;
-		s.pc = beg;
+		s.pc = eparams->beg;
 		s.strs = state->prog->strs;
 
 		/*
@@ -3666,13 +3699,15 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	    } else {
 		/* It's a builtin */
 		LinkList assigns = (LinkList)0;
+		int postassigns = eparams->postassigns;
 		if (forked)
 		    closem(FDT_INTERNAL);
 		if (postassigns) {
 		    Wordcode opc = state->pc;
-		    state->pc = assignspc;
+		    state->pc = eparams->assignspc;
 		    assigns = newlinklist();
 		    while (postassigns--) {
+			int htok;
 			wordcode ac = *state->pc++;
 			char *name = ecgetstr(state, EC_DUPTOK, &htok);
 			Asgment asg;
diff --git a/Src/zsh.h b/Src/zsh.h
index 79747d6..a5d4455 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -489,6 +489,7 @@ typedef struct complist  *Complist;
 typedef struct conddef   *Conddef;
 typedef struct dirsav    *Dirsav;
 typedef struct emulation_options *Emulation_options;
+typedef struct execcmd_params *Execcmd_params;
 typedef struct features  *Features;
 typedef struct feature_enables  *Feature_enables;
 typedef struct funcstack *Funcstack;
@@ -1391,6 +1392,21 @@ struct builtin {
  */
 #define BINF_ASSIGN		(1<<19)
 
+/**
+ * Parameters passed to execcmd().
+ * These are not opaque --- they are also used by the pipeline manager.
+ */
+struct execcmd_params {
+    LinkList args;		/* All command prefixes, arguments & options */
+    LinkList redir;		/* Redirections */
+    Wordcode beg;		/* The code at the start of the command */
+    Wordcode varspc;		/* The code for assignment parsed as such */
+    Wordcode assignspc;		/* The code for assignment parsed as typeset */
+    int type;			/* The WC_* type of the command */
+    int postassigns;		/* The number of assignspc assiguments */
+    int htok;			/* tokens in parameter list */
+};
+
 struct module {
     struct hashnode node;
     union {
diff --git a/Test/A01grammar.ztst b/Test/A01grammar.ztst
index 1ad73c5..0b1085c 100644
--- a/Test/A01grammar.ztst
+++ b/Test/A01grammar.ztst
@@ -757,12 +757,9 @@
 >}
 >Stuff here
 
-## This problem is hard to fix without significant changes to how
-## the shell forks for a pipeline.
-#
-#   x=1
-#   x=2 | echo $x
-#   echo $x
-# 0:Assignment-only current shell commands in LHS of pipelin
-# >1
-# >1
+  x=1
+  x=2 | echo $x
+  echo $x
+0:Assignment-only current shell commands in LHS of pipelin
+>1
+>1

Re: Strange parameter visibility

[Bart Schaefer]

On Sep 30,  2:50pm, Peter Stephenson wrote:
}
} A start at refactoring, which I think is generally beneficial

I commend your bravery and perseverance.