Re: [PATCH] More ERR_EXIT (was Re: Tests RE behavior of ERR_EXIT)

[Philippe Altherr <philippe.altherr@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2885 bytes --]

>
> Here are two additional cases that your patch does not cover.  They
> are not regressions, as stable zsh 5.9 is also affected.


Interesting! And the symptoms are exactly the same as they were with
function calls:

- If you replace "echo done" with "echo done $?" you can see that the
"source" and "eval" statements return the right exit status. They "just"
fail to trigger ERR_EXIT.
- When you drop the "if" statement and only keep "false && true", then
ERR_EXIT is correctly triggered.

It looks like another case where saving and restoring this_noerrexit is
missing. And, indeed, if save and restore this_noerrexit are added just
before and after this execlist
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/exec.c#L1221>
in execode, then your examples work and all tests still pass. The function
execode
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/exec.c#L1193>
is
called from builtin.c in the eval
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/builtin.c#L6067>
function. In that context the saving and restoring looks warranted. But
there are multiple other calls, mainly in exec.c. Although for most it
looks right to save and restore, I can't say confidently that it's indeed
the case for all of them. I also wonder whether other elements of the
execution stack should be saved and restored. For function calls, there are
many more things that are saved and restored.

I also noticed that zsh.h declares a structure execstack
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/zsh.h#L1132>,
which is used in exec.c to implement execsave
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/exec.c#L6325>,
which saves the state of the execution stack, and execrestore
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/exec.c#L6357>,
which restores it. Surprisingly these two methods are only used once in
signals.c (here
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/signals.c#L1339>
and. here
<https://github.com/zsh-users/zsh/blob/b1533066ca7d50c88b37ce72093c12cf19807818/Src/signals.c#L1391>).
And even more surprisingly, even though plenty of variables are saved,
including this_noerrexit, the variable noerrexit is NOT saved, which looks
very suspicious to me.

How should we proceed? I'm 99% sure that my current patch (without the
changes mentioned above) is correct (i.e., it fixes a number of problems
and doesn't introduce new ones). At the moment, I'm much less confident
about changing execode as described above or about adding noerrexit
to execstack. Would it make sense to submit my pacth (once I have fixed
Changlelog and 1-2 other things) and try to solve the other problems in
follow-up patches?

Philippe

[-- Attachment #2: Type: text/html, Size: 3429 bytes --]