From 76d9b20e6afdd41543d3a5922c9d680b2c82ed79 Mon Sep 17 00:00:00 2001 From: aarzilli Date: Thu, 19 Oct 2023 15:57:20 +0200 Subject: [PATCH] proc: remove expr evaluation goroutine from EvalExpressionWithCalls We have used a goroutine to keep track of some of the expression evaluation status across target resumes during call injections. Now that the expression interpreter has been rewritten to use a stack machine we can move what little state is left into the stack machine and get rid of the goroutine-and-channel mechanism. --- pkg/proc/eval.go | 224 ++++++++++++++++----------------- pkg/proc/evalop/evalcompile.go | 46 ++++++- pkg/proc/fncall.go | 128 +++++++------------ 3 files changed, 195 insertions(+), 203 deletions(-) diff --git a/pkg/proc/eval.go b/pkg/proc/eval.go index 6e4edbd972..4418e91497 100644 --- a/pkg/proc/eval.go +++ b/pkg/proc/eval.go @@ -9,7 +9,6 @@ import ( "go/constant" "go/parser" "go/printer" - "go/scanner" "go/token" "reflect" "runtime/debug" @@ -43,17 +42,9 @@ type EvalScope struct { frameOffset int64 // When the following pointer is not nil this EvalScope was created - // by CallFunction and the expression evaluation is executing on a - // different goroutine from the debugger's main goroutine. - // Under this circumstance the expression evaluator can make function - // calls by setting up the runtime.debugCallV1 call and then writing a - // value to the continueRequest channel. - // When a value is written to continueRequest the debugger's main goroutine - // will call Continue, when the runtime in the target process sends us a - // request in the function call protocol the debugger's main goroutine will - // write a value to the continueCompleted channel. - // The goroutine executing the expression evaluation shall signal that the - // evaluation is complete by closing the continueRequest channel. + // by EvalExpressionWithCalls and function call injection are allowed. + // See the top comment in fncall.go for a description of how the call + // injection protocol is handled. callCtx *callContext dictAddr uint64 // dictionary address for instantiated generic functions @@ -183,41 +174,24 @@ func GoroutineScope(t *Target, thread Thread) (*EvalScope, error) { // EvalExpression returns the value of the given expression. func (scope *EvalScope) EvalExpression(expr string, cfg LoadConfig) (*Variable, error) { - if scope.callCtx != nil { - // makes sure that the other goroutine won't wait forever if we make a mistake - defer close(scope.callCtx.continueRequest) - } - t, err := parser.ParseExpr(expr) - if eqOff, isAs := isAssignment(err); scope.callCtx != nil && isAs { - lexpr := expr[:eqOff] - rexpr := expr[eqOff+1:] - err := scope.SetVariable(lexpr, rexpr) - scope.callCtx.doReturn(nil, err) - return nil, err - } + ops, err := evalop.Compile(scopeToEvalLookup{scope}, expr, false) if err != nil { - scope.callCtx.doReturn(nil, err) return nil, err } - ops, err := evalop.Compile(scopeToEvalLookup{scope}, t) - if err != nil { - scope.callCtx.doReturn(nil, err) - return nil, err - } + stack := &evalStack{} scope.loadCfg = &cfg - - ev, err := scope.eval(ops) + stack.eval(scope, ops) + ev, err := stack.result(&cfg) if err != nil { - scope.callCtx.doReturn(nil, err) return nil, err } + ev.loadValue(cfg) if ev.Name == "" { ev.Name = expr } - scope.callCtx.doReturn(ev, nil) return ev, nil } @@ -391,14 +365,6 @@ func (scope *EvalScope) ChanGoroutines(expr string, start, count int) ([]int64, return goids, nil } -func isAssignment(err error) (int, bool) { - el, isScannerErr := err.(scanner.ErrorList) - if isScannerErr && el[0].Msg == "expected '==', found '='" { - return el[0].Pos.Offset, true - } - return 0, false -} - // Locals returns all variables in 'scope'. func (scope *EvalScope) Locals(flags localsFlags) ([]*Variable, error) { if scope.Fn == nil { @@ -611,21 +577,14 @@ func (scope *EvalScope) setValue(dstv, srcv *Variable, srcExpr string) error { // SetVariable sets the value of the named variable func (scope *EvalScope) SetVariable(name, value string) error { - lhe, err := parser.ParseExpr(name) - if err != nil { - return err - } - rhe, err := parser.ParseExpr(value) - if err != nil { - return err - } - - ops, err := evalop.CompileSet(scopeToEvalLookup{scope}, lhe, rhe) + ops, err := evalop.CompileSet(scopeToEvalLookup{scope}, name, value) if err != nil { return err } - _, err = scope.eval(ops) + stack := &evalStack{} + stack.eval(scope, ops) + _, err = stack.result(nil) return err } @@ -782,9 +741,15 @@ func (scope *EvalScope) image() *Image { type evalStack struct { stack []*Variable // current stack of Variable values fncalls []*functionCallState // stack of call injections currently being executed + ops []evalop.Op // program being executed opidx int // program counter for the stack program callInjectionContinue bool // when set program execution suspends and the call injection protocol is executed instead err error + + spoff, bpoff, fboff int64 + scope *EvalScope + curthread Thread + lastRetiredFncall *functionCallState } func (s *evalStack) push(v *Variable) { @@ -820,83 +785,105 @@ func (s *evalStack) pushErr(v *Variable, err error) { s.stack = append(s.stack, v) } -func (scope *EvalScope) eval(ops []evalop.Op) (*Variable, error) { +// eval evaluates ops. When it returns if callInjectionContinue is set the +// target program should be resumed to execute the call injection protocol. +// Otherwise the result of the evaluation can be retrieved using +// stack.result. +func (stack *evalStack) eval(scope *EvalScope, ops []evalop.Op) { if logflags.FnCall() { fncallLog("eval program:\n%s", evalop.Listing(nil, ops)) } - stack := &evalStack{} - var spoff, bpoff, fboff int64 + stack.ops = ops + stack.scope = scope if scope.g != nil { - spoff = int64(scope.Regs.Uint64Val(scope.Regs.SPRegNum)) - int64(scope.g.stack.hi) - bpoff = int64(scope.Regs.Uint64Val(scope.Regs.BPRegNum)) - int64(scope.g.stack.hi) - fboff = scope.Regs.FrameBase - int64(scope.g.stack.hi) + stack.spoff = int64(scope.Regs.Uint64Val(scope.Regs.SPRegNum)) - int64(scope.g.stack.hi) + stack.bpoff = int64(scope.Regs.Uint64Val(scope.Regs.BPRegNum)) - int64(scope.g.stack.hi) + stack.fboff = scope.Regs.FrameBase - int64(scope.g.stack.hi) } - var curthread Thread + if scope.g != nil && scope.g.Thread != nil { - curthread = scope.g.Thread + stack.curthread = scope.g.Thread } - // funcCallSteps executes the call injection protocol until more input from - // the stack program is needed (i.e. until the call injection either needs - // to copy function arguments, terminates or fails. - // Scope and curthread are updated every time the target program stops). - funcCallSteps := func() { - for stack.callInjectionContinue { - scope.callCtx.injectionThread = nil - g := scope.callCtx.doContinue() - // Go 1.15 will move call injection execution to a different goroutine, - // but we want to keep evaluation on the original goroutine. - if g.ID == scope.g.ID { - scope.g = g - } else { - // We are in Go 1.15 and we switched to a new goroutine, the original - // goroutine is now parked and therefore does not have a thread - // associated. - scope.g.Thread = nil - scope.g.Status = Gwaiting - scope.callCtx.injectionThread = g.Thread - } + stack.run() +} - // adjust the value of registers inside scope - pcreg, bpreg, spreg := scope.Regs.Reg(scope.Regs.PCRegNum), scope.Regs.Reg(scope.Regs.BPRegNum), scope.Regs.Reg(scope.Regs.SPRegNum) - scope.Regs.ClearRegisters() - scope.Regs.AddReg(scope.Regs.PCRegNum, pcreg) - scope.Regs.AddReg(scope.Regs.BPRegNum, bpreg) - scope.Regs.AddReg(scope.Regs.SPRegNum, spreg) - scope.Regs.Reg(scope.Regs.SPRegNum).Uint64Val = uint64(spoff + int64(scope.g.stack.hi)) - scope.Regs.Reg(scope.Regs.BPRegNum).Uint64Val = uint64(bpoff + int64(scope.g.stack.hi)) - scope.Regs.FrameBase = fboff + int64(scope.g.stack.hi) - scope.Regs.CFA = scope.frameOffset + int64(scope.g.stack.hi) - curthread = g.Thread - - stack.callInjectionContinue = false - finished := funcCallStep(scope, stack, g.Thread) - if finished { - funcCallFinish(scope, stack) - } - } +// resume resumes evaluation of stack.ops. When it returns if +// callInjectionContinue is set the target program should be resumed to +// execute the call injection protocol. Otherwise the result of the +// evaluation can be retrieved using stack.result. +func (stack *evalStack) resume(g *G) { + stack.callInjectionContinue = false + scope := stack.scope + // Go 1.15 will move call injection execution to a different goroutine, + // but we want to keep evaluation on the original goroutine. + if g.ID == scope.g.ID { + scope.g = g + } else { + // We are in Go 1.15 and we switched to a new goroutine, the original + // goroutine is now parked and therefore does not have a thread + // associated. + scope.g.Thread = nil + scope.g.Status = Gwaiting + scope.callCtx.injectionThread = g.Thread + } + + // adjust the value of registers inside scope + pcreg, bpreg, spreg := scope.Regs.Reg(scope.Regs.PCRegNum), scope.Regs.Reg(scope.Regs.BPRegNum), scope.Regs.Reg(scope.Regs.SPRegNum) + scope.Regs.ClearRegisters() + scope.Regs.AddReg(scope.Regs.PCRegNum, pcreg) + scope.Regs.AddReg(scope.Regs.BPRegNum, bpreg) + scope.Regs.AddReg(scope.Regs.SPRegNum, spreg) + scope.Regs.Reg(scope.Regs.SPRegNum).Uint64Val = uint64(stack.spoff + int64(scope.g.stack.hi)) + scope.Regs.Reg(scope.Regs.BPRegNum).Uint64Val = uint64(stack.bpoff + int64(scope.g.stack.hi)) + scope.Regs.FrameBase = stack.fboff + int64(scope.g.stack.hi) + scope.Regs.CFA = scope.frameOffset + int64(scope.g.stack.hi) + stack.curthread = g.Thread + + finished := funcCallStep(scope, stack, g.Thread) + if finished { + funcCallFinish(scope, stack) + } + + if stack.callInjectionContinue { + // not done with call injection, stay in this mode + stack.scope.callCtx.injectionThread = nil + return } - for stack.opidx < len(ops) && stack.err == nil { + // call injection protocol suspended or concluded, resume normal opcode execution + stack.run() +} + +func (stack *evalStack) run() { + scope, curthread := stack.scope, stack.curthread + for stack.opidx < len(stack.ops) && stack.err == nil { stack.callInjectionContinue = false - scope.executeOp(stack, ops, curthread) + stack.executeOp() // If the instruction we just executed requests the call injection // protocol by setting callInjectionContinue we switch to it. if stack.callInjectionContinue { - funcCallSteps() + scope.callCtx.injectionThread = nil + return } } + if stack.err == nil && len(stack.fncalls) > 0 { + stack.err = fmt.Errorf("internal debugger error: eval program finished without error but %d call injections still active", len(stack.fncalls)) + return + } + // If there is an error we must undo all currently executing call // injections before returning. - if stack.err == nil && len(stack.fncalls) > 0 { - return nil, fmt.Errorf("internal debugger error: eval program finished without error but %d call injections still active", len(stack.fncalls)) - } - for len(stack.fncalls) > 0 { + if len(stack.fncalls) > 0 { fncall := stack.fncallPeek() + if fncall == stack.lastRetiredFncall { + stack.err = fmt.Errorf("internal debugger error: could not undo injected call during error recovery, original error: %v", stack.err) + return + } if fncall.undoInjection != nil { // setTargetExecuted is set if evalop.CallInjectionSetTarget has been // executed but evalop.CallInjectionComplete hasn't, we must undo the callOP @@ -913,14 +900,15 @@ func (scope *EvalScope) eval(ops []evalop.Op) (*Variable, error) { panic("not implemented") } } + stack.lastRetiredFncall = fncall + // Resume target to undo one call stack.callInjectionContinue = true - prevlen := len(stack.fncalls) - funcCallSteps() - if len(stack.fncalls) == prevlen { - return nil, fmt.Errorf("internal debugger error: could not undo injected call during error recovery, original error: %v", stack.err) - } + scope.callCtx.injectionThread = nil + return } +} +func (stack *evalStack) result(cfg *LoadConfig) (*Variable, error) { var r *Variable switch len(stack.stack) { case 0: @@ -932,11 +920,15 @@ func (scope *EvalScope) eval(ops []evalop.Op) (*Variable, error) { stack.err = fmt.Errorf("internal debugger error: wrong stack size at end %d", len(stack.stack)) } } + if r != nil && cfg != nil && stack.err == nil { + r.loadValue(*cfg) + } return r, stack.err } -// executeOp executes the opcode at ops[stack.opidx] and increments stack.opidx. -func (scope *EvalScope) executeOp(stack *evalStack, ops []evalop.Op, curthread Thread) { +// executeOp executes the opcode at stack.ops[stack.opidx] and increments stack.opidx. +func (stack *evalStack) executeOp() { + scope, ops, curthread := stack.scope, stack.ops, stack.curthread defer func() { err := recover() if err != nil { @@ -1121,11 +1113,13 @@ func (scope *EvalScope) executeOp(stack *evalStack, ops []evalop.Op, curthread T } func (scope *EvalScope) evalAST(t ast.Expr) (*Variable, error) { - ops, err := evalop.Compile(scopeToEvalLookup{scope}, t) + ops, err := evalop.CompileAST(scopeToEvalLookup{scope}, t) if err != nil { return nil, err } - return scope.eval(ops) + stack := &evalStack{} + stack.eval(scope, ops) + return stack.result(nil) } func exprToString(t ast.Expr) string { diff --git a/pkg/proc/evalop/evalcompile.go b/pkg/proc/evalop/evalcompile.go index 5b6519aa5b..5b7907e7c0 100644 --- a/pkg/proc/evalop/evalcompile.go +++ b/pkg/proc/evalop/evalcompile.go @@ -6,7 +6,9 @@ import ( "fmt" "go/ast" "go/constant" + "go/parser" "go/printer" + "go/scanner" "go/token" "strconv" "strings" @@ -34,8 +36,8 @@ type evalLookup interface { LookupRegisterName(string) (int, bool) } -// Compile compiles the expression t into a list of instructions. -func Compile(lookup evalLookup, t ast.Expr) ([]Op, error) { +// CompileAST compiles the expression t into a list of instructions. +func CompileAST(lookup evalLookup, t ast.Expr) ([]Op, error) { ctx := &compileCtx{evalLookup: lookup, allowCalls: true} err := ctx.compileAST(t) if err != nil { @@ -49,11 +51,44 @@ func Compile(lookup evalLookup, t ast.Expr) ([]Op, error) { return ctx.ops, nil } -// CompileSet compiles the expression setting lhe to rhe into a list of +// Compile compiles the expression expr into a list of instructions. +// If canSet is true expressions like "x = y" are also accepted. +func Compile(lookup evalLookup, expr string, canSet bool) ([]Op, error) { + t, err := parser.ParseExpr(expr) + if err != nil { + if canSet { + eqOff, isAs := isAssignment(err) + if isAs { + return CompileSet(lookup, expr[:eqOff], expr[eqOff+1:]) + } + } + return nil, err + } + return CompileAST(lookup, t) +} + +func isAssignment(err error) (int, bool) { + el, isScannerErr := err.(scanner.ErrorList) + if isScannerErr && el[0].Msg == "expected '==', found '='" { + return el[0].Pos.Offset, true + } + return 0, false +} + +// CompileSet compiles the expression setting lhexpr to rhexpr into a list of // instructions. -func CompileSet(lookup evalLookup, lhe, rhe ast.Expr) ([]Op, error) { +func CompileSet(lookup evalLookup, lhexpr, rhexpr string) ([]Op, error) { + lhe, err := parser.ParseExpr(lhexpr) + if err != nil { + return nil, err + } + rhe, err := parser.ParseExpr(rhexpr) + if err != nil { + return nil, err + } + ctx := &compileCtx{evalLookup: lookup, allowCalls: true} - err := ctx.compileAST(rhe) + err = ctx.compileAST(rhe) if err != nil { return nil, err } @@ -74,7 +109,6 @@ func CompileSet(lookup evalLookup, lhe, rhe ast.Expr) ([]Op, error) { return ctx.ops, err } return ctx.ops, nil - } func (ctx *compileCtx) compileAllocLiteralString() { diff --git a/pkg/proc/fncall.go b/pkg/proc/fncall.go index 0fbbcddc27..d256810422 100644 --- a/pkg/proc/fncall.go +++ b/pkg/proc/fncall.go @@ -27,15 +27,14 @@ import ( // The protocol is described in $GOROOT/src/runtime/asm_amd64.s in the // comments for function runtime·debugCallVn. // -// The main entry point is EvalExpressionWithCalls which will start a goroutine to -// evaluate the provided expression. -// This goroutine can either return immediately, if no function calls were -// needed, or write a continue request to the scope.callCtx.continueRequest -// channel. When this happens EvalExpressionWithCalls will call Continue and -// return. +// The main entry point is EvalExpressionWithCalls which will set up an +// evalStack object to evaluate the provided expression. +// This object can either finish immediately, if no function calls were +// needed, or return with callInjectionContinue set. When this happens +// EvalExpressionWithCalls will call Continue and return. // -// The Continue loop will write to scope.callCtx.continueCompleted when it -// hits a breakpoint in the call injection protocol. +// The Continue loop will call evalStack.resume when it hits a breakpoint in +// the call injection protocol. // // The work of setting up the function call and executing the protocol is // done by: @@ -110,15 +109,6 @@ type callContext struct { // retLoadCfg is the load configuration used to load return values retLoadCfg LoadConfig - // Write to continueRequest to request a call to Continue from the - // debugger's main goroutine. - // Read from continueCompleted to wait for the target process to stop at - // one of the interaction point of the function call protocol. - // To signal that evaluation is completed a value will be written to - // continueRequest having cont == false and the return values in ret. - continueRequest chan<- continueRequest - continueCompleted <-chan *G - // injectionThread is the thread to use for nested call injections if the // original injection goroutine isn't running (because we are in Go 1.15) injectionThread Thread @@ -128,32 +118,10 @@ type callContext struct { stacks []stack } -type continueRequest struct { - cont bool - err error - ret *Variable -} - type callInjection struct { - // if continueCompleted is not nil it means we are in the process of - // executing an injected function call, see comments throughout - // pkg/proc/fncall.go for a description of how this works. - continueCompleted chan<- *G - continueRequest <-chan continueRequest - startThreadID int - endCallInjection func() -} - -func (callCtx *callContext) doContinue() *G { - callCtx.continueRequest <- continueRequest{cont: true} - return <-callCtx.continueCompleted -} - -func (callCtx *callContext) doReturn(ret *Variable, err error) { - if callCtx == nil { - return - } - callCtx.continueRequest <- continueRequest{cont: false, ret: ret, err: err} + evalStack *evalStack + startThreadID int + endCallInjection func() } // EvalExpressionWithCalls is like EvalExpression but allows function calls in 'expr'. @@ -178,7 +146,7 @@ func EvalExpressionWithCalls(grp *TargetGroup, g *G, expr string, retLoadCfg Loa return errGoroutineNotRunning } - if callinj := t.fncallForG[g.ID]; callinj != nil && callinj.continueCompleted != nil { + if callinj := t.fncallForG[g.ID]; callinj != nil && callinj.evalStack != nil { return errFuncCallInProgress } @@ -192,72 +160,69 @@ func EvalExpressionWithCalls(grp *TargetGroup, g *G, expr string, retLoadCfg Loa return err } - continueRequest := make(chan continueRequest) - continueCompleted := make(chan *G) - scope.callCtx = &callContext{ - grp: grp, - p: t, - checkEscape: checkEscape, - retLoadCfg: retLoadCfg, - continueRequest: continueRequest, - continueCompleted: continueCompleted, + grp: grp, + p: t, + checkEscape: checkEscape, + retLoadCfg: retLoadCfg, } + scope.loadCfg = &retLoadCfg endCallInjection, err := t.proc.StartCallInjection() if err != nil { return err } - t.fncallForG[g.ID] = &callInjection{ - continueCompleted: continueCompleted, - continueRequest: continueRequest, - startThreadID: 0, - endCallInjection: endCallInjection, + ops, err := evalop.Compile(scopeToEvalLookup{scope}, expr, true) + if err != nil { + return err } - go scope.EvalExpression(expr, retLoadCfg) + stack := &evalStack{} + + t.fncallForG[g.ID] = &callInjection{ + evalStack: stack, + startThreadID: 0, + endCallInjection: endCallInjection, + } - contReq, ok := <-continueRequest - if contReq.cont { + stack.eval(scope, ops) + if stack.callInjectionContinue { return grp.Continue() } - return finishEvalExpressionWithCalls(t, g, contReq, ok) + return finishEvalExpressionWithCalls(t, g, stack) } -func finishEvalExpressionWithCalls(t *Target, g *G, contReq continueRequest, ok bool) error { +func finishEvalExpressionWithCalls(t *Target, g *G, stack *evalStack) error { fncallLog("stashing return values for %d in thread=%d", g.ID, g.Thread.ThreadID()) g.Thread.Common().CallReturn = true - var err error - if !ok { - err = errors.New("internal error EvalExpressionWithCalls didn't return anything") - } else if contReq.err != nil { - if fpe, ispanic := contReq.err.(fncallPanicErr); ispanic { + ret, err := stack.result(&stack.scope.callCtx.retLoadCfg) + if err != nil { + if fpe, ispanic := stack.err.(fncallPanicErr); ispanic { + err = nil g.Thread.Common().returnValues = []*Variable{fpe.panicVar} - } else { - err = contReq.err } - } else if contReq.ret == nil { + } else if ret == nil { g.Thread.Common().returnValues = nil - } else if contReq.ret.Addr == 0 && contReq.ret.DwarfType == nil && contReq.ret.Kind == reflect.Invalid { + } else if ret.Addr == 0 && ret.DwarfType == nil && ret.Kind == reflect.Invalid { // this is a variable returned by a function call with multiple return values - r := make([]*Variable, len(contReq.ret.Children)) - for i := range contReq.ret.Children { - r[i] = &contReq.ret.Children[i] + r := make([]*Variable, len(ret.Children)) + for i := range ret.Children { + r[i] = &ret.Children[i] } g.Thread.Common().returnValues = r } else { - g.Thread.Common().returnValues = []*Variable{contReq.ret} + g.Thread.Common().returnValues = []*Variable{ret} } - close(t.fncallForG[g.ID].continueCompleted) callinj := t.fncallForG[g.ID] for goid := range t.fncallForG { if t.fncallForG[goid] == callinj { delete(t.fncallForG, goid) } } + callinj.evalStack = nil callinj.endCallInjection() return err } @@ -1163,10 +1128,9 @@ func callInjectionProtocol(t *Target, threads []Thread) (done bool, err error) { fncallLog("step for injection on goroutine %d (current) thread=%d (location %s)", g.ID, thread.ThreadID(), loc.Fn.Name) t.currentThread = thread - callinj.continueCompleted <- g - contReq, ok := <-callinj.continueRequest - if !contReq.cont { - err := finishEvalExpressionWithCalls(t, g, contReq, ok) + callinj.evalStack.resume(g) + if !callinj.evalStack.callInjectionContinue { + err := finishEvalExpressionWithCalls(t, g, callinj.evalStack) if err != nil { return done, err } @@ -1187,7 +1151,7 @@ func findCallInjectionStateForThread(t *Target, thread Thread) (*G, *callInjecti } callinj := t.fncallForG[g.ID] if callinj != nil { - if callinj.continueCompleted == nil { + if callinj.evalStack == nil { return nil, nil, notfound() } return g, callinj, nil @@ -1199,7 +1163,7 @@ func findCallInjectionStateForThread(t *Target, thread Thread) (*G, *callInjecti // thread. for goid, callinj := range t.fncallForG { - if callinj != nil && callinj.continueCompleted != nil && callinj.startThreadID != 0 && callinj.startThreadID == thread.ThreadID() { + if callinj != nil && callinj.evalStack != nil && callinj.startThreadID != 0 && callinj.startThreadID == thread.ThreadID() { t.fncallForG[g.ID] = callinj fncallLog("goroutine %d is the goroutine executing the call injection started in goroutine %d", g.ID, goid) return g, callinj, nil