From 7276c02b4193edb19bc0d2d36a786238564db03f Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Thu, 12 Sep 2013 14:00:16 -0400 Subject: runtime, cmd/gc, cmd/ld: ignore method wrappers in recover Bug #1: Issue 5406 identified an interesting case: defer iface.M() may end up calling a wrapper that copies an indirect receiver from the iface value and then calls the real M method. That's two calls down, not just one, and so recover() == nil always in the real M method, even during a panic. [For the purposes of this entire discussion, a wrapper's implementation is a function containing an ordinary call, not the optimized tail call form that is somtimes possible. The tail call does not create a second frame, so it is already handled correctly.] Fix this bug by introducing g->panicwrap, which counts the number of bytes on current stack segment that are due to wrapper calls that should not count against the recover check. All wrapper functions must now adjust g->panicwrap up on entry and back down on exit. This adds slightly to their expense; on the x86 it is a single instruction at entry and exit; on the ARM it is three. However, the alternative is to make a call to recover depend on being able to walk the stack, which I very much want to avoid. We have enough problems walking the stack for garbage collection and profiling. Also, if performance is critical in a specific case, it is already faster to use a pointer receiver and avoid this kind of wrapper entirely. Bug #2: The old code, which did not consider the possibility of two calls, already contained a check to see if the call had split its stack and so the panic-created segment was one behind the current segment. In the wrapper case, both of the two calls might split their stacks, so the panic-created segment can be two behind the current segment. Fix this by propagating the Stktop.panic flag forward during stack splits instead of looking backward during recover. Fixes #5406. R=golang-dev, iant CC=golang-dev https://golang.org/cl/13367052 --- src/cmd/5l/noop.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++ src/cmd/6l/pass.c | 32 ++++++++++++++++++++++++---- src/cmd/8l/pass.c | 32 ++++++++++++++++++++++++---- src/cmd/gc/go.h | 1 + src/cmd/gc/pgen.c | 13 +++++++++++- src/cmd/gc/subr.c | 1 + src/cmd/ld/textflag.h | 2 ++ 7 files changed, 131 insertions(+), 9 deletions(-) (limited to 'src/cmd') diff --git a/src/cmd/5l/noop.c b/src/cmd/5l/noop.c index 7b63aa7156..0bd76040d3 100644 --- a/src/cmd/5l/noop.c +++ b/src/cmd/5l/noop.c @@ -271,6 +271,35 @@ noops(void) p->to.offset = -autosize; p->to.reg = REGSP; p->spadj = autosize; + + if(cursym->text->reg & WRAPPER) { + // g->panicwrap += autosize; + // MOVW panicwrap_offset(g), R3 + // ADD $autosize, R3 + // MOVW R3 panicwrap_offset(g) + p = appendp(p); + p->as = AMOVW; + p->from.type = D_OREG; + p->from.reg = REGG; + p->from.offset = 2*PtrSize; + p->to.type = D_REG; + p->to.reg = 3; + + p = appendp(p); + p->as = AADD; + p->from.type = D_CONST; + p->from.offset = autosize; + p->to.type = D_REG; + p->to.reg = 3; + + p = appendp(p); + p->as = AMOVW; + p->from.type = D_REG; + p->from.reg = 3; + p->to.type = D_OREG; + p->to.reg = REGG; + p->to.offset = 2*PtrSize; + } break; case ARET: @@ -290,6 +319,36 @@ noops(void) break; } } + + if(cursym->text->reg & WRAPPER) { + // g->panicwrap -= autosize; + // MOVW panicwrap_offset(g), R3 + // SUB $autosize, R3 + // MOVW R3 panicwrap_offset(g) + p->as = AMOVW; + p->from.type = D_OREG; + p->from.reg = REGG; + p->from.offset = 2*PtrSize; + p->to.type = D_REG; + p->to.reg = 3; + p = appendp(p); + + p->as = ASUB; + p->from.type = D_CONST; + p->from.offset = autosize; + p->to.type = D_REG; + p->to.reg = 3; + p = appendp(p); + + p->as = AMOVW; + p->from.type = D_REG; + p->from.reg = 3; + p->to.type = D_OREG; + p->to.reg = REGG; + p->to.offset = 2*PtrSize; + p = appendp(p); + } + p->as = AMOVW; p->scond |= C_PBIT; p->from.type = D_OREG; diff --git a/src/cmd/6l/pass.c b/src/cmd/6l/pass.c index d24672432f..1be3c18fe2 100644 --- a/src/cmd/6l/pass.c +++ b/src/cmd/6l/pass.c @@ -511,11 +511,12 @@ dostkoff(void) diag("nosplit func likely to overflow stack"); q = P; - if(!(p->from.scale & NOSPLIT)) { + if(!(p->from.scale & NOSPLIT) || (p->from.scale & WRAPPER)) { p = appendp(p); p = load_g_cx(p); // load g into CX - p = stacksplit(p, autoffset, &q); // emit split check } + if(!(cursym->text->from.scale & NOSPLIT)) + p = stacksplit(p, autoffset, &q); // emit split check if(autoffset) { p = appendp(p); @@ -523,8 +524,6 @@ dostkoff(void) p->from.type = D_CONST; p->from.offset = autoffset; p->spadj = autoffset; - if(q != P) - q->pcond = p; } else { // zero-byte stack adjustment. // Insert a fake non-zero adjustment so that stkcheck can @@ -536,7 +535,19 @@ dostkoff(void) p->as = ANOP; p->spadj = PtrSize; } + if(q != P) + q->pcond = p; deltasp = autoffset; + + if(cursym->text->from.scale & WRAPPER) { + // g->panicwrap += autoffset + PtrSize; + p = appendp(p); + p->as = AADDL; + p->from.type = D_CONST; + p->from.offset = autoffset + PtrSize; + p->to.type = D_INDIR+D_CX; + p->to.offset = 2*PtrSize; + } if(debug['K'] > 1 && autoffset) { // 6l -KK means double-check for stack overflow @@ -654,6 +665,19 @@ dostkoff(void) if(autoffset != deltasp) diag("unbalanced PUSH/POP"); + + if(cursym->text->from.scale & WRAPPER) { + p = load_g_cx(p); + p = appendp(p); + // g->panicwrap -= autoffset + PtrSize; + p->as = ASUBL; + p->from.type = D_CONST; + p->from.offset = autoffset + PtrSize; + p->to.type = D_INDIR+D_CX; + p->to.offset = 2*PtrSize; + p = appendp(p); + p->as = ARET; + } if(autoffset) { p->as = AADJSP; diff --git a/src/cmd/8l/pass.c b/src/cmd/8l/pass.c index b558ffaa9d..1eaf78fe0a 100644 --- a/src/cmd/8l/pass.c +++ b/src/cmd/8l/pass.c @@ -444,11 +444,12 @@ dostkoff(void) q = P; - if(!(p->from.scale & NOSPLIT)) { + if(!(p->from.scale & NOSPLIT) || (p->from.scale & WRAPPER)) { p = appendp(p); p = load_g_cx(p); // load g into CX - p = stacksplit(p, autoffset, &q); // emit split check } + if(!(cursym->text->from.scale & NOSPLIT)) + p = stacksplit(p, autoffset, &q); // emit split check if(autoffset) { p = appendp(p); @@ -456,8 +457,6 @@ dostkoff(void) p->from.type = D_CONST; p->from.offset = autoffset; p->spadj = autoffset; - if(q != P) - q->pcond = p; } else { // zero-byte stack adjustment. // Insert a fake non-zero adjustment so that stkcheck can @@ -469,8 +468,20 @@ dostkoff(void) p->as = ANOP; p->spadj = PtrSize; } + if(q != P) + q->pcond = p; deltasp = autoffset; + if(cursym->text->from.scale & WRAPPER) { + // g->panicwrap += autoffset + PtrSize; + p = appendp(p); + p->as = AADDL; + p->from.type = D_CONST; + p->from.offset = autoffset + PtrSize; + p->to.type = D_INDIR+D_CX; + p->to.offset = 2*PtrSize; + } + if(debug['Z'] && autoffset && !(cursym->text->from.scale&NOSPLIT)) { // 8l -Z means zero the stack frame on entry. // This slows down function calls but can help avoid @@ -540,6 +551,19 @@ dostkoff(void) if(autoffset != deltasp) diag("unbalanced PUSH/POP"); + + if(cursym->text->from.scale & WRAPPER) { + p = load_g_cx(p); + p = appendp(p); + // g->panicwrap -= autoffset + PtrSize; + p->as = ASUBL; + p->from.type = D_CONST; + p->from.offset = autoffset + PtrSize; + p->to.type = D_INDIR+D_CX; + p->to.offset = 2*PtrSize; + p = appendp(p); + p->as = ARET; + } if(autoffset) { p->as = AADJSP; diff --git a/src/cmd/gc/go.h b/src/cmd/gc/go.h index 103aedb41e..ba73508b8f 100644 --- a/src/cmd/gc/go.h +++ b/src/cmd/gc/go.h @@ -272,6 +272,7 @@ struct Node uchar implicit; uchar addrtaken; // address taken, even if not moved to heap uchar dupok; // duplicate definitions ok (for func) + uchar wrapper; // is method wrapper (for func) schar likely; // likeliness of if statement uchar hasbreak; // has break statement uchar needzero; // if it contains pointers, needs to be zeroed on function entry diff --git a/src/cmd/gc/pgen.c b/src/cmd/gc/pgen.c index d649fc49da..0fbac84cf6 100644 --- a/src/cmd/gc/pgen.c +++ b/src/cmd/gc/pgen.c @@ -95,7 +95,18 @@ compile(Node *fn) nodconst(&nod1, types[TINT32], 0); ptxt = gins(ATEXT, isblank(curfn->nname) ? N : curfn->nname, &nod1); if(fn->dupok) - ptxt->TEXTFLAG = DUPOK; + ptxt->TEXTFLAG |= DUPOK; + if(fn->wrapper) + ptxt->TEXTFLAG |= WRAPPER; + + // Clumsy but important. + // See test/recover.go for test cases and src/pkg/reflect/value.go + // for the actual functions being considered. + if(myimportpath != nil && strcmp(myimportpath, "reflect") == 0) { + if(strcmp(curfn->nname->sym->name, "callReflect") == 0 || strcmp(curfn->nname->sym->name, "callMethod") == 0) + ptxt->TEXTFLAG |= WRAPPER; + } + afunclit(&ptxt->from, curfn->nname); ginit(); diff --git a/src/cmd/gc/subr.c b/src/cmd/gc/subr.c index 54fddbb909..3b3b576312 100644 --- a/src/cmd/gc/subr.c +++ b/src/cmd/gc/subr.c @@ -2588,6 +2588,7 @@ genwrapper(Type *rcvr, Type *method, Sym *newnam, int iface) n->left = newname(methodsym(method->sym, methodrcvr, 0)); fn->nbody = list(fn->nbody, n); } else { + fn->wrapper = 1; // ignore frame for panic+recover matching call = nod(OCALL, dot, N); call->list = args; call->isddd = isddd; diff --git a/src/cmd/ld/textflag.h b/src/cmd/ld/textflag.h index 64ae647fb6..1d62db7368 100644 --- a/src/cmd/ld/textflag.h +++ b/src/cmd/ld/textflag.h @@ -17,3 +17,5 @@ #define RODATA 8 // This data contains no pointers. #define NOPTR 16 +// This is a wrapper function and should not count as disabling 'recover'. +#define WRAPPER 32 -- cgit v1.3