Hello, i am not subscribed, please cc me. in this bugreport https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=908818 i reported an issue with Zsh 5.6. Debian maintainers asked to report directly. So here it goes. The shortest test case i could come with is documented in https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=908818#66 Basically its a setup for startup profiling which did work up to 5.5.1 but since 5.6 instead of displaying profiling data zsh says: zsh: suspended (tty output) i then have to 'fg' first to see the actual profiling data. kind regards, Thilo
[-- Attachment #1: Type: text/plain, Size: 738 bytes --] 5.6.2 has been announced, with a fix for this. On Sat, Sep 15, 2018, 10:42 AM TS <debts@xk2c.de> wrote: > Hello, > > i am not subscribed, please cc me. > > in this bugreport > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=908818 > > i reported an issue with Zsh 5.6. Debian maintainers asked to report > directly. So here it goes. > > The shortest test case i could come with is documented in > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=908818#66 > > Basically its a setup for startup profiling which did work up to 5.5.1 > but since 5.6 instead of displaying profiling data zsh says: > > zsh: suspended (tty output) > > i then have to 'fg' first to see the actual profiling data. > > > > > kind regards, > > Thilo > >
Hello,
Bart Schaefer schrieb/wrote:
> 5.6.2 has been announced, with a fix for this.
Thanks for your reply. Sorry for not being verbose enough.
Well actually i did this with 5.6.2 and that version still
shows of that issue.
Downgrading to 5.5.1 and older fixes it for me.
kind regards,
Thilo
TS schrieb/wrote: > Hello, > > Bart Schaefer schrieb/wrote: >> 5.6.2 has been announced, with a fix for this. > > Thanks for your reply. Sorry for not being verbose enough. > Well actually i did this with 5.6.2 and that version still > shows of that issue. > Downgrading to 5.5.1 and older fixes it for me. Test case, tested with 5.6.2: https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=908818;filename=.zshrc;msg=66 result for me: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=908818#66 kind regards, Thilo
On 2018-09-15 21:06:56 +0200, TS wrote: > Test case, tested with 5.6.2: > https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=908818;filename=.zshrc;msg=66 > > result for me: > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=908818#66 I think I have noticed issues with "less" using zsh-5.6.1-10-g24bb465 but I didn't remember what I did exactly and I couldn't reproduce them. I needed to do a fg. -- Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/> 100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/> Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
[-- Attachment #1: Type: text/plain, Size: 1137 bytes --] Hello Vincent Lefevre schrieb/wrote: > On 2018-09-15 21:06:56 +0200, TS wrote: >> Test case, tested with 5.6.2: >> https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=908818;filename=.zshrc;msg=66 >> >> result for me: >> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=908818#66 > > I think I have noticed issues with "less" using zsh-5.6.1-10-g24bb465 > but I didn't remember what I did exactly and I couldn't reproduce > them. I needed to do a fg. I have now tried different iterations of that minimal .zshrc. It seems only when less is wrapped inside a function i get this "zsh: suspended (tty output)". But when less is wrapped inside a function like this, it is rather reliable replicable. steps to show the issue: 1) ~/.zshrc as attached 2) exec zsh or login e.g.: % exec zsh zsh: suspended (tty output) i have a testuser for such things, therefore i "su -l" to him first. But according to my tests, the "su -l" part is irrelevant for the issue. Only the "less wrapped in function" part seem to matter. I earlier versions of Zsh that did not make a difference. Thanks for taking a look. kind regards, Thilo [-- Attachment #2: .zshrc --] [-- Type: text/plain, Size: 66 bytes --] builtin zmodload zsh/zprof lessx() { =less } zprof |& lessx
On Sun, 16 Sep 2018 17:40:48 +0200 TS <debts@xk2c.de> wrote: > I have now tried different iterations of that minimal .zshrc. > It seems only when less is wrapped inside a function i get this > "zsh: suspended (tty output)". Thanks for that --- it does help to have all the reproduction detail inline (or attached) in a post on the list. > But when less is wrapped inside a function like this, it is rather reliable > replicable. All the previous forms of this bug show this when inside a sourced file, so e.g. testfile containing echo | { cat | more; } followed by ". ./testfile" shows this up. I've rebased job_control_debug2 on top of all the latest fixes to track this down. (Just the sort of thing I like doing in my hotel room on holiday.) The issue is that the list_pipe_job mechanism is being applied twice, once for the "." and once for the nested code (a function in your case, a nested pipeline in the above case). Regular readers will know one application is quite enough --- quite whether doing this twice is a good idea is anybody's guess. Anyway, the first job is marked as being in control of the terminal; then the second job gets the terminal. The first job exits and the shell erroneously thinks it was still in control of the terminal and swipes it back from under the feet of the second. We need to keep track of which job's process group last got the terminal. I think the combination of recording this in attachtty() and also in the recently added code that fixes up the records in the main shell from what the new subshell just did should do the trick --- we're still protected by the STAT_NOSTTY check with the additional safety. However, I'm not sure we want to rush this fix out to all and sundry. The trouble is, clearly no one's testing things until they're released (which is why the releases are broken). If this is likely to come unstuck anywhere, I'd suspect use of ^Z and fg which is applying the most additional stress to the foreground terminal machinations, so if you have things you do with ^Z and complex shell constructs please try them out. I've also tightened up in entersubsh() so that if the process is asynchronous it doesn't pass up the status --- that's all the exec.c hunks are doing. diff --git a/Src/exec.c b/Src/exec.c index b9af9ea63..a667b078d 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -1036,7 +1036,7 @@ entersubsh(int flags, struct entersubsh_ret *retp) if (!(flags & ESUB_ASYNC)) attachtty(jobtab[thisjob].gleader); } - if (retp) { + if (retp && !(flags & ESUB_ASYNC)) { retp->gleader = jobtab[list_pipe_job].gleader; retp->list_pipe_job = list_pipe_job; } @@ -1058,12 +1058,13 @@ entersubsh(int flags, struct entersubsh_ret *retp) !jobtab[list_pipe_job].gleader) jobtab[list_pipe_job].gleader = jobtab[thisjob].gleader; setpgrp(0L, jobtab[thisjob].gleader); - if (!(flags & ESUB_ASYNC)) + if (!(flags & ESUB_ASYNC)) { attachtty(jobtab[thisjob].gleader); - if (retp) { - retp->gleader = jobtab[thisjob].gleader; - if (list_pipe_job != thisjob) - retp->list_pipe_job = list_pipe_job; + if (retp) { + retp->gleader = jobtab[thisjob].gleader; + if (list_pipe_job != thisjob) + retp->list_pipe_job = list_pipe_job; + } } } } diff --git a/Src/jobs.c b/Src/jobs.c index db2e87ec1..2d58319a8 100644 --- a/Src/jobs.c +++ b/Src/jobs.c @@ -40,6 +40,11 @@ mod_export pid_t origpgrp; /**/ mod_export pid_t mypgrp; + +/* the last process group to attach to the terminal */ + +/**/ +pid_t last_attached_pgrp; /* the job we are working on */ @@ -1405,6 +1410,11 @@ addproc(pid_t pid, char *text, int aux, struct timeval *bgtime, jobtab[thisjob].gleader = gleader; if (list_pipe_job_used != -1) jobtab[list_pipe_job_used].gleader = gleader; + /* + * Record here this is the latest process group to grab the + * terminal as attachtty() was run in the subshell. + */ + last_attached_pgrp = gleader; } else if (!jobtab[thisjob].gleader) jobtab[thisjob].gleader = pid; /* attach this process to end of process list of current job */ diff --git a/Src/signals.c b/Src/signals.c index 99aad0fab..26d88abc2 100644 --- a/Src/signals.c +++ b/Src/signals.c @@ -540,8 +540,8 @@ wait_for_processes(void) if (WIFEXITED(status) && pn->pid == jn->gleader && killpg(pn->pid, 0) == -1) { - jn->gleader = 0; - if (!(jn->stat & STAT_NOSTTY)) { + if (last_attached_pgrp == jn->gleader && + !(jn->stat & STAT_NOSTTY)) { /* * This PID was in control of the terminal; * reclaim terminal now it has exited. @@ -552,6 +552,7 @@ wait_for_processes(void) attachtty(mypgrp); adjustwinsize(0); } + jn->gleader = 0; } } update_job(jn); diff --git a/Src/utils.c b/Src/utils.c index 075d27241..5a9fbdd32 100644 --- a/Src/utils.c +++ b/Src/utils.c @@ -4670,6 +4670,10 @@ attachtty(pid_t pgrp) ep = 1; } } + else + { + last_attached_pgrp = pgrp; + } } }
Hello Allow me to do this broken record writing, but wouldn't it be nice to have Valgrind automatic tests prepared for a handy run while doing the `suspended tty' investigation and patching? The VATS (Valgrind Automatic Tests Suite) can be still added to upstream. It is just a copy of Test directory, I know this doesn't sound nice, but also: definition-files with non-harmful, already known Valgrind errors, and a Zsh script that parses Valgrind output doing some hiding, formatting, etc. On Sat, 15 Sep 2018 at 19:42, TS <debts@xk2c.de> wrote: > > Hello, > > i am not subscribed, please cc me. > > in this bugreport > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=908818 > > i reported an issue with Zsh 5.6. Debian maintainers asked to report > directly. So here it goes. > > The shortest test case i could come with is documented in > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=908818#66 > > Basically its a setup for startup profiling which did work up to 5.5.1 > but since 5.6 instead of displaying profiling data zsh says: > > zsh: suspended (tty output) > > i then have to 'fg' first to see the actual profiling data. > > > > > kind regards, > > Thilo > -- Sebastian Gniazdowski News: https://twitter.com/ZdharmaI IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin Blog: http://zdharma.org
Sebastian Gniazdowski wrote on Mon, 17 Sep 2018 16:01 +0200:
> Hello
> Allow me to do this broken record writing, but wouldn't it be nice to
> have Valgrind automatic tests prepared for a handy run while doing the
> `suspended tty' investigation and patching? The VATS (Valgrind
> Automatic Tests Suite) can be still added to upstream. It is just a
> copy of Test directory, I know this doesn't sound nice, but also:
> definition-files with non-harmful, already known Valgrind errors, and
> a Zsh script that parses Valgrind output doing some hiding,
> formatting, etc.
Yes, it would be a good thing to have support in the build system to run
the test suite under valgrind, complete with developer documentation
(such as what configure and config.modules settings to use).
However, if the proposal means having two copies of the Test/ directory,
then it's a non-starter.
Can you describe what changes would be involved? Would the test
suite remain as portable as it should be?
Cheers,
Daniel
Hello Peter, > Thanks for that --- it does help to have all the reproduction detail inline > (or attached) in a post on the list. I take a note about this for next time. Thanks back to you also, for having a look at it. > (Just the sort of thing I like doing in my hotel room on holiday.) Send me your bank connection via private mail. I will transfer you a amount of money for having {1..3} drinks of your liking. ;) Uups, almost forgot the patch you send did fix it. Thanks! kind regards, Thilo
On 2018-09-16 10:40:20 +0200, Vincent Lefevre wrote: > On 2018-09-15 21:06:56 +0200, TS wrote: > > Test case, tested with 5.6.2: > > https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=908818;filename=.zshrc;msg=66 > > > > result for me: > > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=908818#66 > > I think I have noticed issues with "less" using zsh-5.6.1-10-g24bb465 > but I didn't remember what I did exactly and I couldn't reproduce > them. I needed to do a fg. The session in the terminal looks like that: zira:~/software/mpfr> emacs <3:13:29 ^Zzsh: exit 148 zsh: suspended eclient zira:~/software/mpfr[TSTP]> bg <3:13:33 [1] + continued eclient zira:~/software/mpfr> less src/Makefile <3:13:38 zsh: suspended (tty output) less src/Makefile zira:~/software/mpfr[TTOU]> fg <3:13:42 [2] + continued less src/Makefile zira:~/software/mpfr> <3:13:42 In short: 1. Run emacs[*] (with its X interface). 2. Type Ctrl-Z in the terminal to stop Emacs and get the prompt. 3. Type "bg". 4. View a file with "less"[*]. 5. Quit Emacs (while "less" is still running). 6. Quit "less" (with 'q'). This yields a TTOU on "less" (see above). Note that 2+3 cannot be replaced by the use of "emacs &". [*] This is actually a shell function: eclient () { emulate -LR zsh local display i if [[ -n $GNUCLIENT_ENABLED ]] then display="$DISPLAY" for i in "$@" do [[ "$i" == "-nw" ]] && unset display done [[ -n $display ]] && gnuclient "$@" 2> /dev/null || \emacs "$@" else \emacs "$@" fi } with GNUCLIENT_ENABLED not defined. -- Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/> 100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/> Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
On Mon, 17 Sep 2018 at 16:50, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > Can you describe what changes would be involved? Would the test > suite remain as portable as it should be? I was thinking little different back then when creating VATS, it appeared reasonable to have separate tests aimed at stressing Zsh in terms of memory usage, etc. I now see that the main Zsh tests are putting much stress on Zsh, so VATS should just add vtest.conf, custom runtests.zsh, zsh-valgrind-parse.cmd, custom ztst.zsh (I'm not sure if the two custom files really have to be different), from here: https://github.com/zdharma/VATS-zsh/tree/master/VATS. The readme of this project is unnecessarily very long, the thing is very simple, I need some time to approach this again with original Test/ in mind. I've found asciinema about VATS: https://asciinema.org/a/125035 (asciinema seems down at the moment). -- Sebastian Gniazdowski News: https://twitter.com/ZdharmaI IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin Blog: http://zdharma.org
Sebastian Gniazdowski wrote on Tue, 18 Sep 2018 07:21 +0200: > On Mon, 17 Sep 2018 at 16:50, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > > Can you describe what changes would be involved? Would the test > > suite remain as portable as it should be? > > I was thinking little different back then when creating VATS, it > appeared reasonable to have separate tests aimed at stressing Zsh in > terms of memory usage, etc. I now see that the main Zsh tests are > putting much stress on Zsh, so VATS should just add vtest.conf, custom > runtests.zsh, zsh-valgrind-parse.cmd, custom ztst.zsh (I'm not sure if > the two custom files really have to be different), from here: > https://github.com/zdharma/VATS-zsh/tree/master/VATS. You haven't actually answered my question. Can you, please, explain what changes would need to be made to zsh in order to implement your upthread proposal? It's fine to link to external sources for details but the gist should be self-contained and understandable by readers familiar with zsh and its build system but nothing else. > The readme of this project is unnecessarily very long, the thing is > very simple, I need some time to approach this again with original > Test/ in mind. For avoidance of doubt, I am simply asking for information on the proposal. I am not yet sold on it.
On Tue, 18 Sep 2018 03:16:45 +0200
Vincent Lefevre <vincent@vinc17.net> wrote:
> 1. Run emacs[*] (with its X interface).
> 2. Type Ctrl-Z in the terminal to stop Emacs and get the prompt.
> 3. Type "bg".
> 4. View a file with "less"[*].
> 5. Quit Emacs (while "less" is still running).
> 6. Quit "less" (with 'q').
>
> This yields a TTOU on "less" (see above). Note that 2+3 cannot be
> replaced by the use of "emacs &".
This appears to be the same bug as the last one I've recently fixed.
When emacs exits, having previously been in control of the terminal
(hence the need to start in the foreground), the shell thinks it still
is and swipes the terminal from less. The shell now remembers the
actual last process group to be in control.
pws
On Mon, 17 Sep 2018 22:13:40 +0200 TS <debts@xk2c.de> wrote: > Send me your bank connection via private mail. I will transfer you a amount of > money for having {1..3} drinks of your liking. ;) That's OK, but thank for the thought. > Uups, almost forgot the patch you send did fix it. Thanks! Good --- there's one thing about it I don't understand yet but it shouldn't affect the fix. pws
Hello Vincent,
please refine your bugreport. I am not able to reproduce your report.
basically: emacs != eclient
% su -l heinb
Password:
tosh% emacs
zsh: suspended emacs
tosh% bg
[1] + continued emacs
tosh%
[1] + suspended (tty output) emacs
tosh%
i now undestand upsteam.
this is with zsh git tag 5.6.2
kind regards,
Thilo
Vincent Lefevre schrieb/wrote:
> On 2018-09-16 10:40:20 +0200, Vincent Lefevre wrote:
>> On 2018-09-15 21:06:56 +0200, TS wrote:
>>> Test case, tested with 5.6.2:
>>> https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=908818;filename=.zshrc;msg=66
>>>
>>> result for me:
>>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=908818#66
>>
>> I think I have noticed issues with "less" using zsh-5.6.1-10-g24bb465
>> but I didn't remember what I did exactly and I couldn't reproduce
>> them. I needed to do a fg.
>
> The session in the terminal looks like that:
>
> zira:~/software/mpfr> emacs <3:13:29
> ^Zzsh: exit 148
> zsh: suspended eclient
> zira:~/software/mpfr[TSTP]> bg <3:13:33
> [1] + continued eclient
> zira:~/software/mpfr> less src/Makefile <3:13:38
> zsh: suspended (tty output) less src/Makefile
> zira:~/software/mpfr[TTOU]> fg <3:13:42
> [2] + continued less src/Makefile
> zira:~/software/mpfr> <3:13:42
>
> In short:
>
> 1. Run emacs[*] (with its X interface).
> 2. Type Ctrl-Z in the terminal to stop Emacs and get the prompt.
> 3. Type "bg".
> 4. View a file with "less"[*].
> 5. Quit Emacs (while "less" is still running).
> 6. Quit "less" (with 'q').
>
> This yields a TTOU on "less" (see above). Note that 2+3 cannot be
> replaced by the use of "emacs &".
>
> [*] This is actually a shell function:
>
> eclient () {
> emulate -LR zsh
> local display i
> if [[ -n $GNUCLIENT_ENABLED ]]
> then
> display="$DISPLAY"
> for i in "$@"
> do
> [[ "$i" == "-nw" ]] && unset display
> done
> [[ -n $display ]] && gnuclient "$@" 2> /dev/null || \emacs "$@"
> else
> \emacs "$@"
> fi
> }
>
> with GNUCLIENT_ENABLED not defined.
>
[-- Attachment #1: Type: text/plain, Size: 3282 bytes --] Hello Peter, when emacs is wrapped inside function, then emacs is dead when: ^Z bg Does not happen when emacs called w/o function. See minimal .zshrc attached. % su -l heinb Password: tosh% emacs zsh: suspended emacs tosh% bg [1] + continued emacs tosh% [1] + suspended (tty output) emacs tosh% jobs -l [1] + 20708 suspended (tty output) emacs tosh% fg %1 [1] + continued emacs tosh% eclient zsh: suspended eclient tosh% jobs -l [1] + 20718 suspended (signal) eclient tosh% bg [1] + continued eclient tosh% jobs -l [1] - 20718 running eclient tosh% fg %1 [1] - running eclient <<<#### here terminal is broken This is still even after 551ff842721d6ca83727dbe6cd40178f46cc8201 aka "43464: Another attachtty() fix." HTH kind regards, Thilo TS schrieb/wrote: > Hello Vincent, > > please refine your bugreport. I am not able to reproduce your report. > basically: emacs != eclient > > % su -l heinb > Password: > tosh% emacs > > zsh: suspended emacs > tosh% bg > [1] + continued emacs > tosh% > [1] + suspended (tty output) emacs > tosh% > > i now undestand upsteam. > > this is with zsh git tag 5.6.2 > > > > kind regards, > > Thilo > > Vincent Lefevre schrieb/wrote: >> On 2018-09-16 10:40:20 +0200, Vincent Lefevre wrote: >>> On 2018-09-15 21:06:56 +0200, TS wrote: >>>> Test case, tested with 5.6.2: >>>> https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=908818;filename=.zshrc;msg=66 >>>> >>>> result for me: >>>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=908818#66 >>> >>> I think I have noticed issues with "less" using zsh-5.6.1-10-g24bb465 >>> but I didn't remember what I did exactly and I couldn't reproduce >>> them. I needed to do a fg. >> >> The session in the terminal looks like that: >> >> zira:~/software/mpfr> emacs <3:13:29 >> ^Zzsh: exit 148 >> zsh: suspended eclient >> zira:~/software/mpfr[TSTP]> bg <3:13:33 >> [1] + continued eclient >> zira:~/software/mpfr> less src/Makefile <3:13:38 >> zsh: suspended (tty output) less src/Makefile >> zira:~/software/mpfr[TTOU]> fg <3:13:42 >> [2] + continued less src/Makefile >> zira:~/software/mpfr> <3:13:42 >> >> In short: >> >> 1. Run emacs[*] (with its X interface). >> 2. Type Ctrl-Z in the terminal to stop Emacs and get the prompt. >> 3. Type "bg". >> 4. View a file with "less"[*]. >> 5. Quit Emacs (while "less" is still running). >> 6. Quit "less" (with 'q'). >> >> This yields a TTOU on "less" (see above). Note that 2+3 cannot be >> replaced by the use of "emacs &". >> >> [*] This is actually a shell function: >> >> eclient () { >> emulate -LR zsh >> local display i >> if [[ -n $GNUCLIENT_ENABLED ]] >> then >> display="$DISPLAY" >> for i in "$@" >> do >> [[ "$i" == "-nw" ]] && unset display >> done >> [[ -n $display ]] && gnuclient "$@" 2> /dev/null || \emacs "$@" >> else >> \emacs "$@" >> fi >> } >> >> with GNUCLIENT_ENABLED not defined. >> > [-- Attachment #2: .zshrc --] [-- Type: text/plain, Size: 51 bytes --] setopt NO_globalrcs eclient () { =emacs "$@" }
[-- Attachment #1: Type: text/plain, Size: 472 bytes --] Wt., 18.09.2018, 17:55 użytkownik Daniel Shahaf <d.s@daniel.shahaf.name> napisał: Sebastian Gniazdowski wrote on Tue, 18 Sep 2018 07:21 > so VATS should just add vtest.conf, custom > runtests.zsh, zsh-valgrind-parse.cmd, custom ztst.zsh (I'm not sure if > the two custom files really have to be different), from here: > https://github.com/zdharma/VATS-zsh/tree/master/VATS. I'm answering from phone. The changes are described above, max 4 files addition.
es
On Wed, Sep 19, 2018 at 05:38:11AM +0200, TS wrote:
> Hello Peter,
>
> when emacs is wrapped inside function,
> then emacs is dead when:
>
> ^Z
> bg
>
> Does not happen when emacs called w/o function.
> See minimal .zshrc attached.
>
>
> % su -l heinb
> Password:
> tosh% emacs
>
> zsh: suspended emacs
> tosh% bg
> [1] + continued emacs
> tosh%
> [1] + suspended (tty output) emacs
> tosh% jobs -l
> [1] + 20708 suspended (tty output) emacs
> tosh% fg %1
> [1] + continued emacs
> tosh% eclient
> zsh: suspended eclient
> tosh% jobs -l
> [1] + 20718 suspended (signal) eclient
> tosh% bg
> [1] + continued eclient
> tosh% jobs -l
> [1] - 20718 running eclient
> tosh% fg %1
> [1] - running eclient
> <<<#### here terminal is broken
>
>
> setopt NO_globalrcs
> eclient () {
> =emacs "$@"
> }
>
>
This is not a new phenomenon. I can reproduce it with zsh 5.3. ps
shows one difference between the function and the normal emacs case:
there is another zsh instance:
function case after issuing bg:
$ ps -o pid,ppid,state,command
80367 80366 Ss /bin/zsh
80900 80367 T+ /opt/local/bin/emacs
80902 80367 T /bin/zsh
normal case after issuing bg:
$ ps -o pid,ppid,state,command
81266 81265 Ss /bin/zsh
81297 81266 T /opt/local/bin/emacs
After fg %1, I can revive the function case by sending SIGCONT
directly to the emacs process.
--
Best,
Daniel
On Wed, 19 Sep 2018 05:38:11 +0200
TS <debts@xk2c.de> wrote:
> when emacs is wrapped inside function,
> then emacs is dead when:
>
> ^Z
> bg
>
> Does not happen when emacs called w/o function.
> See minimal .zshrc attached.
>
> % su -l heinb
> Password:
> tosh% emacs
>
> zsh: suspended emacs
> tosh% bg
> [1] + continued emacs
> tosh%
> [1] + suspended (tty output) emacs
I'm not quite sure what you're doing here, but it seems to involve emacs
being suspended due to terminal output when it goes into the
*background*, which is entirely sensible as it's not even supposed to be
attached to the terminal then. Quite when it attempts to write to the
terminal during this period is not up to the shell. I'm not sure what
effect the function is having but I don't see how it can be affecting
writes from emacs. My best guess would be it's adding some kind of
delay which is affecting how Emacs starts, but the actual interactions
going on are obscure.
I'm not actually seeing this behaviour myself anyway, but it sounds like
a race so that's probably not surprising.
This doesn't appear to be the problem Vincent was reporting, which I
can't get to happen with that minimal eclient().
pws
Hello, Peter Stephenson schrieb/wrote: >> when emacs is wrapped inside function, >> then emacs is dead when: >> >> ^Z >> bg >> >> Does not happen when emacs called w/o function. >> See minimal .zshrc attached. >> >> % su -l heinb >> Password: >> tosh% emacs >> >> zsh: suspended emacs >> tosh% bg >> [1] + continued emacs >> tosh% >> [1] + suspended (tty output) emacs > > I'm not quite sure what you're doing here, Ok will write down as detailed as possible: First i have an user with absolute no configuration apart from the .zshrc given before. That is to make sure i test in as clean as possible environment. For that i now moved the "setopt NO_globalrcs" from .zshrc to .zshenv to make even cleaner. commented lines are, well comments: ## I then login as that user: % su -l heinb Password: ## I start emacs and press ^z tosh% emacs zsh: suspended emacs ## enter bg at prompt tosh% bg [1] + continued emacs tosh% [1] + suspended (tty output) emacs ## verify backgrounded jobs tosh% jobs -l [1] + 3179 suspended (tty output) emacs ## foreground emacs again and normal quit aka ^x^c tosh% fg %1 [1] + continued emacs tosh% ## now the same sequence with function: tosh% which eclient eclient () { =emacs "$@" } ## I start eclient and press ^z tosh% eclient zsh: suspended eclient ## enter bg at prompt tosh% bg [1] + continued eclient ## verify backgrounded jobs tosh% jobs -l [1] - 3289 running eclient ## foreground eclient again tosh% fg %1 [1] - running eclient ^C^C^C^C^C^C^C^C^C^C^C^C^C^C^C After that emacs does not come back and terminal is not useable anymore, prompt does not appear and ^c does not help to return. If can help by testing or other informations just let me know. In case of trace or so i will need detailed instructions what to do but will happily help. > This doesn't appear to be the problem Vincent was reporting, which I > can't get to happen with that minimal eclient(). In this case sorry Vicent for the complaint. Actually by trying to figuring out an easy way to reproduce your issue we potential found an other one. I am confidant that is half way to fix that, too. Which is good IMHO. kind regards, Thilo
On 2018-09-18 18:58:21 +0100, Peter Stephenson wrote: > On Tue, 18 Sep 2018 03:16:45 +0200 > Vincent Lefevre <vincent@vinc17.net> wrote: > > 1. Run emacs[*] (with its X interface). > > 2. Type Ctrl-Z in the terminal to stop Emacs and get the prompt. > > 3. Type "bg". > > 4. View a file with "less"[*]. > > 5. Quit Emacs (while "less" is still running). > > 6. Quit "less" (with 'q'). > > > > This yields a TTOU on "less" (see above). Note that 2+3 cannot be > > replaced by the use of "emacs &". > > This appears to be the same bug as the last one I've recently fixed. [...] This bug no longer occurs with Debian's zsh 5.6.2-3 package. This is probably this fix: [ Daniel Shahaf ] * [3d8f4eee] Cherry-pick 551ff842 from upstream: Another attachtty() fix. (Closes: #908818) -- Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/> 100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/> Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
Vincent Lefevre wrote on Thu, Sep 20, 2018 at 14:01:06 +0200:
> This bug no longer occurs with Debian's zsh 5.6.2-3 package.
> This is probably this fix:
>
> [ Daniel Shahaf ]
> * [3d8f4eee] Cherry-pick 551ff842 from upstream: Another attachtty()
> fix. (Closes: #908818)
For clarity, this is an excerpt from the changelog of the Debian package
which names me as the person who backported Peter's patch, which had
been committed after the 5.6.2 tag, into the Debian package of 5.6.2.
This is simply the default format for Debian package changelogs. (I
have my disagreements with it.)
Cheers,
Daniel
On Tue, 18 Sep 2018 at 17:55, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > Sebastian Gniazdowski wrote on Tue, 18 Sep 2018 07:21 +0200: > > On Mon, 17 Sep 2018 at 16:50, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > > > > Can you describe what changes would be involved? Would the test > > > suite remain as portable as it should be? What's needed is currently only this +-2 patch. It makes passing VALGRIND=1 to the tests' make run the tests through VATS/valgrind. It creates a conditional variable VLGRND which has a value of "v" in such case, making the tests go throguh vruntests.zsh and not runtests.zsh: @@ -35,6 +35,7 @@ VPATH = @srcdir@ sdir = @srcdir@ sdir_top = @top_srcdir@ INSTALL = @INSTALL@ +VLGRND = $(VALGRIND:1=v) @DEFS_MK@ @@ -49,7 +50,7 @@ check test: do echo $$f; done`" \ ZTST_srcdir="$(sdir)" \ ZTST_exe=$(dir_top)/Src/zsh@EXEEXT@ \ - $(dir_top)/Src/zsh@EXEEXT@ +Z -f $(sdir)/runtests.zsh; then \ + $(dir_top)/Src/zsh@EXEEXT@ +Z -f $(sdir)/$(VLGRND)runtests.zsh; then \ stat=0; \ else \ stat=1; \ > > The readme of this project is unnecessarily very long, the thing is > > very simple, I need some time to approach this again with original > > Test/ in mind. > > For avoidance of doubt, I am simply asking for information on the > proposal. I am not yet sold on it. I hope the valgrind tests will be added. I've spend 2 days on carefully building the errors database on OS X. I think that on Linux there will be no such problem, as it seems that it's OS X that embraces the fact that freed memory is actually never returned to the OS (so it apparently does allocations here and there, without any code to release the memory). The tests apparently reveal one memory leak currently: ==52847== 79 (24 direct, 55 indirect) bytes in 1 blocks are definitely lost in loss record 349 of 550 ==52847== at 0x10017B545: malloc (vg_replace_malloc.c:302) ==52847== by 0x10004899E: zalloc (mem.c:966) ==52847== by 0x10004316D: znewlinklist (linklist.c:120) ==52847== by 0x10003989D: addfilelist (jobs.c:1297) ==52847== by 0x100017AC0: getoutputfile (exec.c:4796) ==52847== by 0x100078474: stringsubst (subst.c:254) ==52847== by 0x100077E46: prefork (subst.c:142) ==52847== by 0x10001A24F: execfuncdef (exec.c:2567) ==52847== by 0x10001DC23: execcmd_exec (exec.c:3896) ==52847== by 0x10001AAF6: execpline2 (exec.c:1927) ==52847== by 0x100015938: execpline (exec.c:1658) ==52847== by 0x1000150E2: execlist (exec.c:1413) The full procedure to add the valgrind tests to Zsh is: 1. Apply the patch 2. Add files: vruntests.zsh, vtest.conf, zsh-valgrind-parse.cmd to the Test subdirectory After this running make VALGRIND=1 inside the Test subdirectory will run the valgrind tests. The files and additional information are available at: https://github.com/zdharma/VATS-zsh -- Sebastian Gniazdowski News: https://twitter.com/ZdharmaI IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin Blog: http://zdharma.org
Ah, also: using the ztst.zsh from the repo will cause the descriptions of all tests to be printed before executing – to allow matching up the valgrind reports against concrete Zsh code. The change to the file is very simple: diff --git a/Test/ztst.zsh b/Test/ztst.zsh index d835e0edb..d20893f53 100755 --- a/Test/ztst.zsh +++ b/Test/ztst.zsh @@ -402,6 +402,7 @@ $ZTST_curline" ZTST_xstatus=$match[1] ZTST_flags=$match[2] ZTST_message=${match[3]:+${match[3][2,-1]}} + [[ $VLGRND_TEST_DESC = 1 ]] && print -- "VATS-test-desc: $ZTST_message" else ZTST_testfailed "expecting test status at: On Thu, 4 Jul 2019 at 01:29, Sebastian Gniazdowski <sgniazdowski@gmail.com> wrote: > > On Tue, 18 Sep 2018 at 17:55, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > > > Sebastian Gniazdowski wrote on Tue, 18 Sep 2018 07:21 +0200: > > > On Mon, 17 Sep 2018 at 16:50, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > > > > > > Can you describe what changes would be involved? Would the test > > > > suite remain as portable as it should be? > > What's needed is currently only this +-2 patch. It makes passing > VALGRIND=1 to the tests' make run the tests through VATS/valgrind. It > creates a conditional variable VLGRND which has a value of "v" in such > case, making the tests go throguh vruntests.zsh and not runtests.zsh: > > @@ -35,6 +35,7 @@ VPATH = @srcdir@ > sdir = @srcdir@ > sdir_top = @top_srcdir@ > INSTALL = @INSTALL@ > +VLGRND = $(VALGRIND:1=v) > > @DEFS_MK@ > > @@ -49,7 +50,7 @@ check test: > do echo $$f; done`" \ > ZTST_srcdir="$(sdir)" \ > ZTST_exe=$(dir_top)/Src/zsh@EXEEXT@ \ > - $(dir_top)/Src/zsh@EXEEXT@ +Z -f $(sdir)/runtests.zsh; then \ > + $(dir_top)/Src/zsh@EXEEXT@ +Z -f $(sdir)/$(VLGRND)runtests.zsh; then \ > stat=0; \ > else \ > stat=1; \ > > > > The readme of this project is unnecessarily very long, the thing is > > > very simple, I need some time to approach this again with original > > > Test/ in mind. > > > > For avoidance of doubt, I am simply asking for information on the > > proposal. I am not yet sold on it. > > I hope the valgrind tests will be added. I've spend 2 days on > carefully building the errors database on OS X. I think that on Linux > there will be no such problem, as it seems that it's OS X that > embraces the fact that freed memory is actually never returned to the > OS (so it apparently does allocations here and there, without any code > to release the memory). The tests apparently reveal one memory leak > currently: > > ==52847== 79 (24 direct, 55 indirect) bytes in 1 blocks are definitely > lost in loss record 349 of 550 > ==52847== at 0x10017B545: malloc (vg_replace_malloc.c:302) > ==52847== by 0x10004899E: zalloc (mem.c:966) > ==52847== by 0x10004316D: znewlinklist (linklist.c:120) > ==52847== by 0x10003989D: addfilelist (jobs.c:1297) > ==52847== by 0x100017AC0: getoutputfile (exec.c:4796) > ==52847== by 0x100078474: stringsubst (subst.c:254) > ==52847== by 0x100077E46: prefork (subst.c:142) > ==52847== by 0x10001A24F: execfuncdef (exec.c:2567) > ==52847== by 0x10001DC23: execcmd_exec (exec.c:3896) > ==52847== by 0x10001AAF6: execpline2 (exec.c:1927) > ==52847== by 0x100015938: execpline (exec.c:1658) > ==52847== by 0x1000150E2: execlist (exec.c:1413) > > The full procedure to add the valgrind tests to Zsh is: > 1. Apply the patch > 2. Add files: vruntests.zsh, vtest.conf, zsh-valgrind-parse.cmd to the > Test subdirectory > > After this running make VALGRIND=1 inside the Test subdirectory will > run the valgrind tests. > > The files and additional information are available at: > https://github.com/zdharma/VATS-zsh > > -- > Sebastian Gniazdowski > News: https://twitter.com/ZdharmaI > IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin > Blog: http://zdharma.org -- Sebastian Gniazdowski News: https://twitter.com/ZdharmaI IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin Blog: http://zdharma.org
I've made a 20 seconds asciinema to show the new test-descriptions feature: https://asciinema.org/a/255371 On Thu, 4 Jul 2019 at 01:31, Sebastian Gniazdowski <sgniazdowski@gmail.com> wrote: > > Ah, also: using the ztst.zsh from the repo will cause the descriptions > of all tests to be printed before executing – to allow matching up the > valgrind reports against concrete Zsh code. The change to the file is > very simple: > > diff --git a/Test/ztst.zsh b/Test/ztst.zsh > index d835e0edb..d20893f53 100755 > --- a/Test/ztst.zsh > +++ b/Test/ztst.zsh > @@ -402,6 +402,7 @@ $ZTST_curline" > ZTST_xstatus=$match[1] > ZTST_flags=$match[2] > ZTST_message=${match[3]:+${match[3][2,-1]}} > + [[ $VLGRND_TEST_DESC = 1 ]] && print -- "VATS-test-desc: > $ZTST_message" > else > ZTST_testfailed "expecting test status at: > > On Thu, 4 Jul 2019 at 01:29, Sebastian Gniazdowski > <sgniazdowski@gmail.com> wrote: > > > > On Tue, 18 Sep 2018 at 17:55, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > > > > > Sebastian Gniazdowski wrote on Tue, 18 Sep 2018 07:21 +0200: > > > > On Mon, 17 Sep 2018 at 16:50, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > > > > > > > > Can you describe what changes would be involved? Would the test > > > > > suite remain as portable as it should be? > > > > What's needed is currently only this +-2 patch. It makes passing > > VALGRIND=1 to the tests' make run the tests through VATS/valgrind. It > > creates a conditional variable VLGRND which has a value of "v" in such > > case, making the tests go throguh vruntests.zsh and not runtests.zsh: > > > > @@ -35,6 +35,7 @@ VPATH = @srcdir@ > > sdir = @srcdir@ > > sdir_top = @top_srcdir@ > > INSTALL = @INSTALL@ > > +VLGRND = $(VALGRIND:1=v) > > > > @DEFS_MK@ > > > > @@ -49,7 +50,7 @@ check test: > > do echo $$f; done`" \ > > ZTST_srcdir="$(sdir)" \ > > ZTST_exe=$(dir_top)/Src/zsh@EXEEXT@ \ > > - $(dir_top)/Src/zsh@EXEEXT@ +Z -f $(sdir)/runtests.zsh; then \ > > + $(dir_top)/Src/zsh@EXEEXT@ +Z -f $(sdir)/$(VLGRND)runtests.zsh; then \ > > stat=0; \ > > else \ > > stat=1; \ > > > > > > The readme of this project is unnecessarily very long, the thing is > > > > very simple, I need some time to approach this again with original > > > > Test/ in mind. > > > > > > For avoidance of doubt, I am simply asking for information on the > > > proposal. I am not yet sold on it. > > > > I hope the valgrind tests will be added. I've spend 2 days on > > carefully building the errors database on OS X. I think that on Linux > > there will be no such problem, as it seems that it's OS X that > > embraces the fact that freed memory is actually never returned to the > > OS (so it apparently does allocations here and there, without any code > > to release the memory). The tests apparently reveal one memory leak > > currently: > > > > ==52847== 79 (24 direct, 55 indirect) bytes in 1 blocks are definitely > > lost in loss record 349 of 550 > > ==52847== at 0x10017B545: malloc (vg_replace_malloc.c:302) > > ==52847== by 0x10004899E: zalloc (mem.c:966) > > ==52847== by 0x10004316D: znewlinklist (linklist.c:120) > > ==52847== by 0x10003989D: addfilelist (jobs.c:1297) > > ==52847== by 0x100017AC0: getoutputfile (exec.c:4796) > > ==52847== by 0x100078474: stringsubst (subst.c:254) > > ==52847== by 0x100077E46: prefork (subst.c:142) > > ==52847== by 0x10001A24F: execfuncdef (exec.c:2567) > > ==52847== by 0x10001DC23: execcmd_exec (exec.c:3896) > > ==52847== by 0x10001AAF6: execpline2 (exec.c:1927) > > ==52847== by 0x100015938: execpline (exec.c:1658) > > ==52847== by 0x1000150E2: execlist (exec.c:1413) > > > > The full procedure to add the valgrind tests to Zsh is: > > 1. Apply the patch > > 2. Add files: vruntests.zsh, vtest.conf, zsh-valgrind-parse.cmd to the > > Test subdirectory > > > > After this running make VALGRIND=1 inside the Test subdirectory will > > run the valgrind tests. > > > > The files and additional information are available at: > > https://github.com/zdharma/VATS-zsh > > > > -- > > Sebastian Gniazdowski > > News: https://twitter.com/ZdharmaI > > IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin > > Blog: http://zdharma.org > > > > -- > Sebastian Gniazdowski > News: https://twitter.com/ZdharmaI > IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin > Blog: http://zdharma.org -- Sebastian Gniazdowski News: https://twitter.com/ZdharmaI IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin Blog: http://zdharma.org
Hello, Daniel seems to be currently away or uninterested – could someone else take a look at this? On Thu, 4 Jul 2019 at 01:45, Sebastian Gniazdowski <sgniazdowski@gmail.com> wrote: > > I've made a 20 seconds asciinema to show the new test-descriptions > feature: https://asciinema.org/a/255371 > > On Thu, 4 Jul 2019 at 01:31, Sebastian Gniazdowski > <sgniazdowski@gmail.com> wrote: > > > > Ah, also: using the ztst.zsh from the repo will cause the descriptions > > of all tests to be printed before executing – to allow matching up the > > valgrind reports against concrete Zsh code. The change to the file is > > very simple: > > > > diff --git a/Test/ztst.zsh b/Test/ztst.zsh > > index d835e0edb..d20893f53 100755 > > --- a/Test/ztst.zsh > > +++ b/Test/ztst.zsh > > @@ -402,6 +402,7 @@ $ZTST_curline" > > ZTST_xstatus=$match[1] > > ZTST_flags=$match[2] > > ZTST_message=${match[3]:+${match[3][2,-1]}} > > + [[ $VLGRND_TEST_DESC = 1 ]] && print -- "VATS-test-desc: > > $ZTST_message" > > else > > ZTST_testfailed "expecting test status at: > > > > On Thu, 4 Jul 2019 at 01:29, Sebastian Gniazdowski > > <sgniazdowski@gmail.com> wrote: > > > > > > On Tue, 18 Sep 2018 at 17:55, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > > > > > > > Sebastian Gniazdowski wrote on Tue, 18 Sep 2018 07:21 +0200: > > > > > On Mon, 17 Sep 2018 at 16:50, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > > > > > > > > > > Can you describe what changes would be involved? Would the test > > > > > > suite remain as portable as it should be? > > > > > > What's needed is currently only this +-2 patch. It makes passing > > > VALGRIND=1 to the tests' make run the tests through VATS/valgrind. It > > > creates a conditional variable VLGRND which has a value of "v" in such > > > case, making the tests go throguh vruntests.zsh and not runtests.zsh: > > > > > > @@ -35,6 +35,7 @@ VPATH = @srcdir@ > > > sdir = @srcdir@ > > > sdir_top = @top_srcdir@ > > > INSTALL = @INSTALL@ > > > +VLGRND = $(VALGRIND:1=v) > > > > > > @DEFS_MK@ > > > > > > @@ -49,7 +50,7 @@ check test: > > > do echo $$f; done`" \ > > > ZTST_srcdir="$(sdir)" \ > > > ZTST_exe=$(dir_top)/Src/zsh@EXEEXT@ \ > > > - $(dir_top)/Src/zsh@EXEEXT@ +Z -f $(sdir)/runtests.zsh; then \ > > > + $(dir_top)/Src/zsh@EXEEXT@ +Z -f $(sdir)/$(VLGRND)runtests.zsh; then \ > > > stat=0; \ > > > else \ > > > stat=1; \ > > > > > > > > The readme of this project is unnecessarily very long, the thing is > > > > > very simple, I need some time to approach this again with original > > > > > Test/ in mind. > > > > > > > > For avoidance of doubt, I am simply asking for information on the > > > > proposal. I am not yet sold on it. > > > > > > I hope the valgrind tests will be added. I've spend 2 days on > > > carefully building the errors database on OS X. I think that on Linux > > > there will be no such problem, as it seems that it's OS X that > > > embraces the fact that freed memory is actually never returned to the > > > OS (so it apparently does allocations here and there, without any code > > > to release the memory). The tests apparently reveal one memory leak > > > currently: > > > > > > ==52847== 79 (24 direct, 55 indirect) bytes in 1 blocks are definitely > > > lost in loss record 349 of 550 > > > ==52847== at 0x10017B545: malloc (vg_replace_malloc.c:302) > > > ==52847== by 0x10004899E: zalloc (mem.c:966) > > > ==52847== by 0x10004316D: znewlinklist (linklist.c:120) > > > ==52847== by 0x10003989D: addfilelist (jobs.c:1297) > > > ==52847== by 0x100017AC0: getoutputfile (exec.c:4796) > > > ==52847== by 0x100078474: stringsubst (subst.c:254) > > > ==52847== by 0x100077E46: prefork (subst.c:142) > > > ==52847== by 0x10001A24F: execfuncdef (exec.c:2567) > > > ==52847== by 0x10001DC23: execcmd_exec (exec.c:3896) > > > ==52847== by 0x10001AAF6: execpline2 (exec.c:1927) > > > ==52847== by 0x100015938: execpline (exec.c:1658) > > > ==52847== by 0x1000150E2: execlist (exec.c:1413) > > > > > > The full procedure to add the valgrind tests to Zsh is: > > > 1. Apply the patch > > > 2. Add files: vruntests.zsh, vtest.conf, zsh-valgrind-parse.cmd to the > > > Test subdirectory > > > > > > After this running make VALGRIND=1 inside the Test subdirectory will > > > run the valgrind tests. > > > > > > The files and additional information are available at: > > > https://github.com/zdharma/VATS-zsh > > > > > > -- > > > Sebastian Gniazdowski > > > News: https://twitter.com/ZdharmaI > > > IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin > > > Blog: http://zdharma.org > > > > > > > > -- > > Sebastian Gniazdowski > > News: https://twitter.com/ZdharmaI > > IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin > > Blog: http://zdharma.org > > > > -- > Sebastian Gniazdowski > News: https://twitter.com/ZdharmaI > IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin > Blog: http://zdharma.org -- Sebastian Gniazdowski News: https://twitter.com/ZdharmaI IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin Blog: http://zdharma.org
Sebastian Gniazdowski wrote on Sat, Jul 13, 2019 at 16:31:10 +0200:
> Daniel seems to be currently away or uninterested – could someone else
> take a look at this?
You're not entitled to having me review your patch, nor did I at any
point commit myself to doing so. Do not insinuate otherwise.
As to your actual patch, no, I do not presently have time to review it.
Daniel
On Sun, 14 Jul 2019 at 08:39, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > Sebastian Gniazdowski wrote on Sat, Jul 13, 2019 at 16:31:10 +0200: > > Daniel seems to be currently away or uninterested – could someone else > > take a look at this? > > You're not entitled to having me review your patch, nor did I at any > point commit myself to doing so. Do not insinuate otherwise. I wasn't leveraging any "entitlement" but just sincerely following your shown interest to the Valgrind tests. Sorry if it turned out to sound differently. > As to your actual patch, no, I do not presently have time to review it. Ok. I've waited 9 days and I'm afraid that the patch will get lost somewhere. I just want to say, that the thing isn't actually a patch. It's a tool that makes the current tests go through Valgrind. The patch that adds the 'VALGRIND=1' variable to the tests' Makefile is a +-2 lines patch. Therefore could someone else take a look? There's not much to review. -- Sebastian Gniazdowski News: https://twitter.com/ZdharmaI IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin Blog: http://zdharma.org
On Mon, 2019-07-15 at 00:17 +0200, Sebastian Gniazdowski wrote: > On Sun, 14 Jul 2019 at 08:39, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > Ok. I've waited 9 days and I'm afraid that the patch will get lost > somewhere. I just want to say, that the thing isn't actually a patch. > It's a tool that makes the current tests go through Valgrind. The > patch that adds the 'VALGRIND=1' variable to the tests' Makefile is a > +-2 lines patch. Therefore could someone else take a look? There's not > much to review. I don't mind the handler code changing, to make it easy to run tests slightly different, but I'd prefer to add a more general mechanism that's more like how the current system works... This would mean running make ZTST_handler=vruntests.zsh or you can simply set that in the environment. pws diff --git a/Test/Makefile.in b/Test/Makefile.in index 083df4942..09f37bf53 100644 --- a/Test/Makefile.in +++ b/Test/Makefile.in @@ -40,16 +40,21 @@ INSTALL = @INSTALL@ # ========== DEPENDENCIES FOR TESTING ========== +# If ZTST_handler is not empty, run that instead of runtests.zsh. +# It's assumed to be found in the source directory. check test: if test -n "$(DLLD)"; then \ cd $(dir_top) && DESTDIR= \ $(MAKE) MODDIR=`pwd`/$(subdir)/Modules install.modules > /dev/null; \ fi + if test -z "$$ZTST_handler"; then \ + ZTST_handler=runtests.zsh; \ + fi; \ if ZTST_testlist="`for f in $(sdir)/$(TESTNUM)*.ztst; \ do echo $$f; done`" \ ZTST_srcdir="$(sdir)" \ ZTST_exe=$(dir_top)/Src/zsh@EXEEXT@ \ - $(dir_top)/Src/zsh@EXEEXT@ +Z -f $(sdir)/runtests.zsh; then \ + $(dir_top)/Src/zsh@EXEEXT@ +Z -f $(sdir)/$$ZTST_handler; then \ stat=0; \ else \ stat=1; \
On Thu, 2019-07-04 at 01:29 +0200, Sebastian Gniazdowski wrote:
> The tests apparently reveal one memory leak currently:
>
> ==52847== 79 (24 direct, 55 indirect) bytes in 1 blocks are definitely
> lost in loss record 349 of 550
> ==52847== at 0x10017B545: malloc (vg_replace_malloc.c:302)
> ==52847== by 0x10004899E: zalloc (mem.c:966)
> ==52847== by 0x10004316D: znewlinklist (linklist.c:120)
> ==52847== by 0x10003989D: addfilelist (jobs.c:1297)
> ==52847== by 0x100017AC0: getoutputfile (exec.c:4796)
> ==52847== by 0x100078474: stringsubst (subst.c:254)
> ==52847== by 0x100077E46: prefork (subst.c:142)
> ==52847== by 0x10001A24F: execfuncdef (exec.c:2567)
> ==52847== by 0x10001DC23: execcmd_exec (exec.c:3896)
> ==52847== by 0x10001AAF6: execpline2 (exec.c:1927)
> ==52847== by 0x100015938: execpline (exec.c:1658)
> ==52847== by 0x1000150E2: execlist (exec.c:1413)
By the way, I think these are benign --- addfilelist() adds a file to be
tidied up when a job exits, but these appear to happening in subshells.
This means there's no job clear-up --- but the shell simply exits. So
the memory isn't recovered in the subshell but that doesn't cause a
long-term build-up.
I haven't followed every single one of this type, though.
pws
On Mon, 15 Jul 2019 at 11:30, Peter Stephenson <p.stephenson@samsung.com> wrote: > but these appear to happening in subshells. > This means there's no job clear-up --- but the shell simply exits. So > the memory isn't recovered in the subshell but that doesn't cause a > long-term build-up. Yes that makes sense. > I haven't followed every single one of this type, though. So have you run the tests? On which OS? I'm wondering if the errors database was good / enough complete.