From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1158 invoked by alias); 3 Oct 2017 14:46:47 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 41804 Received: (qmail 17708 invoked by uid 1010); 3 Oct 2017 14:46:47 -0000 X-Qmail-Scanner-Diagnostics: from mx1.redhat.com by f.primenet.com.au (envelope-from , uid 7791) with qmail-scanner-2.11 (clamdscan: 0.99.2/21882. spamassassin: 3.4.1. Clear:RC:0(209.132.183.28):SA:0(-6.9/5.0):. Processed in 2.799277 secs); 03 Oct 2017 14:46:47 -0000 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_HI, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham autolearn_force=no version=3.4.1 X-Envelope-From: kdudka@redhat.com X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 34C5C3E2C8 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=kdudka@redhat.com From: Kamil Dudka To: Peter Stephenson Cc: zsh-workers@zsh.org Subject: Re: Crash when completion script call itself. Date: Tue, 03 Oct 2017 16:45:56 +0200 Message-ID: <2309766.fAIUzOSDAY@kdudka-nb> In-Reply-To: <20171002164018.699d03d4@pwslap01u.europe.root.pri> References: <20170929151614.56fd9cff@pwslap01u.europe.root.pri> <20171002164018.699d03d4@pwslap01u.europe.root.pri> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Tue, 03 Oct 2017 14:46:40 +0000 (UTC) On Monday, October 2, 2017 5:40:18 PM CEST Peter Stephenson wrote: > On Fri, 29 Sep 2017 15:16:14 +0100 > > Peter Stephenson wrote: > > I see this flag introduces a heuristic based on frame size, so tweaking > > memory management internally might have a similar but more controllable > > effect; on the other hand, it simultaneously removes any easy trade-off > > you can make directly using compiler flags, and I'm not keen on > > introducing #ifdef's where one branch would be underused and rot. > > This looks like low-hanging fruit. Allocating memory to save over a > shell function happens just at the point where we've created the new > heap for the function, which expires immediately after the call. The > allocation itself should take very little time and on most architectures > accessing the structure should have a similar overhead to accessing the > stack. I was experimenting with a similar, slightly smaller, patch in April: http://www.zsh.org/mla/workers/2017/msg00630.html Now I tried to run the following command on my system: % FUNCNEST=4096 Src/zsh -c 'i=0; fn() { print $(( ++i )); fn; }; fn' With the current upstream HEAD, it counted to 1180. With my old patch, it counted to 1224. With your current patch, it counted to 1242. The difference becomes slightly more significant when zsh is compiled with -fconserve-stack. Then it counts to 2682 with the current version and 3024 with your patch applied. Kamil > I had to tweak the debug in parse.c to avoid a message if I increased > FUNCSAVE to see where there was a crash. > > pws > > diff --git a/Src/exec.c b/Src/exec.c > index dfb50c3..16f9f16 100644 > --- a/Src/exec.c > +++ b/Src/exec.c > @@ -41,6 +41,20 @@ enum { > ADDVAR_RESTORE = 1 << 2 > }; > > +/* Structure in which to save values around shell function call */ > + > +struct funcsave { > + char opts[OPT_SIZE]; > + char *argv0; > + int zoptind, lastval, optcind, numpipestats; > + int *pipestats; > + char *scriptname; > + int breaks, contflag, loops, emulation, noerrexit, oflags, > restore_sticky; + Emulation_options sticky; > + struct funcstack fstack; > +}; > +typedef struct funcsave *Funcsave; > + > /* > * used to suppress ERREXIT and trapping of SIGZERR, SIGEXIT. > * Bits from noerrexit_bits. > @@ -5495,34 +5509,31 @@ int sticky_emulation_differs(Emulation_options > sticky2) mod_export int > doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval) > { > - char **pptab, **x, *oargv0; > - int oldzoptind, oldlastval, oldoptcind, oldnumpipestats, ret; > - int *oldpipestats = NULL; > - char saveopts[OPT_SIZE], *oldscriptname = scriptname; > + char **pptab, **x; > + int ret; > char *name = shfunc->node.nam; > - int flags = shfunc->node.flags, ooflags; > - int savnoerrexit; > + int flags = shfunc->node.flags; > char *fname = dupstring(name); > - int obreaks, ocontflag, oloops, saveemulation, restore_sticky; > Eprog prog; > - struct funcstack fstack; > static int oflags; > - Emulation_options save_sticky = NULL; > static int funcdepth; > Heap funcheap; > > queue_signals(); /* Lots of memory and global state changes coming */ > > NEWHEAPS(funcheap) { > - oargv0 = NULL; > - obreaks = breaks; > - ocontflag = contflag; > - oloops = loops; > + Funcsave funcsave = zhalloc(sizeof(struct funcsave)); > + funcsave->scriptname = scriptname; > + funcsave->argv0 = NULL; > + funcsave->breaks = breaks; > + funcsave->contflag = contflag; > + funcsave->loops = loops; > + funcsave->lastval = lastval; > + funcsave->pipestats = NULL; > + funcsave->numpipestats = numpipestats; > + funcsave->noerrexit = noerrexit; > if (trap_state == TRAP_STATE_PRIMED) > trap_return--; > - oldlastval = lastval; > - oldnumpipestats = numpipestats; > - savnoerrexit = noerrexit; > /* > * Suppression of ERR_RETURN is turned off in function scope. > */ > @@ -5533,8 +5544,8 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) * immediately by a pushheap/popheap pair. > */ > size_t bytes = sizeof(int)*numpipestats; > - oldpipestats = (int *)zhalloc(bytes); > - memcpy(oldpipestats, pipestats, bytes); > + funcsave->pipestats = (int *)zhalloc(bytes); > + memcpy(funcsave->pipestats, pipestats, bytes); > } > > starttrapscope(); > @@ -5543,8 +5554,8 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) pptab = pparams; > if (!(flags & PM_UNDEFINED)) > scriptname = dupstring(name); > - oldzoptind = zoptind; > - oldoptcind = optcind; > + funcsave->zoptind = zoptind; > + funcsave->optcind = optcind; > if (!isset(POSIXBUILTINS)) { > zoptind = 1; > optcind = 0; > @@ -5553,9 +5564,9 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) /* We need to save the current options even if LOCALOPTIONS is > * * not currently set. That's because if it gets set in the * * > function we need to restore the original options on exit. */ > - memcpy(saveopts, opts, sizeof(opts)); > - saveemulation = emulation; > - save_sticky = sticky; > + memcpy(funcsave->opts, opts, sizeof(opts)); > + funcsave->emulation = emulation; > + funcsave->sticky = sticky; > > if (sticky_emulation_differs(shfunc->sticky)) { > /* > @@ -5572,7 +5583,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) */ > sticky = sticky_emulation_dup(shfunc->sticky, 1); > emulation = sticky->emulation; > - restore_sticky = 1; > + funcsave->restore_sticky = 1; > installemulation(emulation, opts); > if (sticky->n_on_opts) { > OptIndex *onptr; > @@ -5591,7 +5602,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) /* All emulations start with pattern disables clear */ > clearpatterndisables(); > } else > - restore_sticky = 0; > + funcsave->restore_sticky = 0; > > if (flags & (PM_TAGGED|PM_TAGGED_LOCAL)) > opts[XTRACE] = 1; > @@ -5609,11 +5620,11 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) else > opts[WARNNESTEDVAR] = 0; > } > - ooflags = oflags; > + funcsave->oflags = oflags; > /* > * oflags is static, because we compare it on the next recursive > - * call. Hence also we maintain ooflags for restoring the previous > - * value of oflags after the call. > + * call. Hence also we maintain a saved version for restoring > + * the previous value of oflags after the call. > */ > oflags = flags; > opts[PRINTEXITVALUE] = 0; > @@ -5624,7 +5635,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) pparams = x = (char **) zshcalloc(((sizeof *x) * > (1 + countlinknodes(doshargs)))); > if (isset(FUNCTIONARGZERO)) { > - oargv0 = argzero; > + funcsave->argv0 = argzero; > argzero = ztrdup(getdata(node)); > } > /* first node contains name regardless of option */ > @@ -5634,7 +5645,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) } else { > pparams = (char **) zshcalloc(sizeof *pparams); > if (isset(FUNCTIONARGZERO)) { > - oargv0 = argzero; > + funcsave->argv0 = argzero; > argzero = ztrdup(argzero); > } > } > @@ -5644,21 +5655,21 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) lastval = 1; > goto undoshfunc; > } > - fstack.name = dupstring(name); > + funcsave->fstack.name = dupstring(name); > /* > * The caller is whatever is immediately before on the stack, > * unless we're at the top, in which case it's the script > * or interactive shell name. > */ > - fstack.caller = funcstack ? funcstack->name : > - dupstring(oargv0 ? oargv0 : argzero); > - fstack.lineno = lineno; > - fstack.prev = funcstack; > - fstack.tp = FS_FUNC; > - funcstack = &fstack; > + funcsave->fstack.caller = funcstack ? funcstack->name : > + dupstring(funcsave->argv0 ? funcsave->argv0 : argzero); > + funcsave->fstack.lineno = lineno; > + funcsave->fstack.prev = funcstack; > + funcsave->fstack.tp = FS_FUNC; > + funcstack = &funcsave->fstack; > > - fstack.flineno = shfunc->lineno; > - fstack.filename = getshfuncfile(shfunc); > + funcsave->fstack.flineno = shfunc->lineno; > + funcsave->fstack.filename = getshfuncfile(shfunc); > > prog = shfunc->funcdef; > if (prog->flags & EF_RUN) { > @@ -5666,7 +5677,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) > > prog->flags &= ~EF_RUN; > > - runshfunc(prog, NULL, fstack.name); > + runshfunc(prog, NULL, funcsave->fstack.name); > > if (!(shf = (Shfunc) shfunctab->getnode(shfunctab, > (name = fname)))) { > @@ -5679,52 +5690,52 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) } > prog = shf->funcdef; > } > - runshfunc(prog, wrappers, fstack.name); > + runshfunc(prog, wrappers, funcsave->fstack.name); > doneshfunc: > - funcstack = fstack.prev; > + funcstack = funcsave->fstack.prev; > undoshfunc: > --funcdepth; > if (retflag) { > retflag = 0; > - breaks = obreaks; > + breaks = funcsave->breaks; > } > freearray(pparams); > - if (oargv0) { > + if (funcsave->argv0) { > zsfree(argzero); > - argzero = oargv0; > + argzero = funcsave->argv0; > } > pparams = pptab; > if (!isset(POSIXBUILTINS)) { > - zoptind = oldzoptind; > - optcind = oldoptcind; > + zoptind = funcsave->zoptind; > + optcind = funcsave->optcind; > } > - scriptname = oldscriptname; > - oflags = ooflags; > + scriptname = funcsave->scriptname; > + oflags = funcsave->oflags; > > endpatternscope(); /* before restoring old LOCALPATTERNS */ > > - if (restore_sticky) { > + if (funcsave->restore_sticky) { > /* > * If we switched to an emulation environment just for > * this function, we interpret the option and emulation > * switch as being a firewall between environments. > */ > - memcpy(opts, saveopts, sizeof(opts)); > - emulation = saveemulation; > - sticky = save_sticky; > + memcpy(opts, funcsave->opts, sizeof(opts)); > + emulation = funcsave->emulation; > + sticky = funcsave->sticky; > } else if (isset(LOCALOPTIONS)) { > /* restore all shell options except PRIVILEGED and RESTRICTED */ > - saveopts[PRIVILEGED] = opts[PRIVILEGED]; > - saveopts[RESTRICTED] = opts[RESTRICTED]; > - memcpy(opts, saveopts, sizeof(opts)); > - emulation = saveemulation; > + funcsave->opts[PRIVILEGED] = opts[PRIVILEGED]; > + funcsave->opts[RESTRICTED] = opts[RESTRICTED]; > + memcpy(opts, funcsave->opts, sizeof(opts)); > + emulation = funcsave->emulation; > } else { > /* just restore a couple. */ > - opts[XTRACE] = saveopts[XTRACE]; > - opts[PRINTEXITVALUE] = saveopts[PRINTEXITVALUE]; > - opts[LOCALOPTIONS] = saveopts[LOCALOPTIONS]; > - opts[LOCALLOOPS] = saveopts[LOCALLOOPS]; > - opts[WARNNESTEDVAR] = saveopts[WARNNESTEDVAR]; > + opts[XTRACE] = funcsave->opts[XTRACE]; > + opts[PRINTEXITVALUE] = funcsave->opts[PRINTEXITVALUE]; > + opts[LOCALOPTIONS] = funcsave->opts[LOCALOPTIONS]; > + opts[LOCALLOOPS] = funcsave->opts[LOCALLOOPS]; > + opts[WARNNESTEDVAR] = funcsave->opts[WARNNESTEDVAR]; > } > > if (opts[LOCALLOOPS]) { > @@ -5732,9 +5743,9 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) zwarn("`continue' active at end of function scope"); > if (breaks) > zwarn("`break' active at end of function scope"); > - breaks = obreaks; > - contflag = ocontflag; > - loops = oloops; > + breaks = funcsave->breaks; > + contflag = funcsave->contflag; > + loops = funcsave->loops; > } > > endtrapscope(); > @@ -5742,11 +5753,11 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int > noreturnval) if (trap_state == TRAP_STATE_PRIMED) > trap_return++; > ret = lastval; > - noerrexit = savnoerrexit; > + noerrexit = funcsave->noerrexit; > if (noreturnval) { > - lastval = oldlastval; > - numpipestats = oldnumpipestats; > - memcpy(pipestats, oldpipestats, sizeof(int)*numpipestats); > + lastval = funcsave->lastval; > + numpipestats = funcsave->numpipestats; > + memcpy(pipestats, funcsave->pipestats, sizeof(int)*numpipestats); > } > } OLDHEAPS; > > diff --git a/Src/parse.c b/Src/parse.c > index 6e0856b..a6a3b09 100644 > --- a/Src/parse.c > +++ b/Src/parse.c > @@ -2742,7 +2742,7 @@ freeeprog(Eprog p) > DPUTS(p->nref < 0 && !(p->flags & EF_HEAP), "Real EPROG has nref < 0"); > DPUTS(p->nref < -1, "Uninitialised EPROG nref"); > #ifdef MAX_FUNCTION_DEPTH > - DPUTS(p->nref > MAX_FUNCTION_DEPTH + 10, "Overlarge EPROG nref"); > + DPUTS(p->nref > zsh_funcnest + 10, "Overlarge EPROG nref"); > #endif > if (p->nref > 0 && !--p->nref) { > for (i = p->npats, pp = p->pats; i--; pp++)