From 08433760186ee0bac71db940d1cae298c45bc587 Mon Sep 17 00:00:00 2001 From: Alessandro Arzilli Date: Mon, 9 Nov 2020 20:28:40 +0100 Subject: [PATCH] proc/*: remove proc.Thread.Blocked, refactor memory access (#2206) On linux we can not read memory if the thread we use to do it is occupied doing certain system calls. The exact conditions when this happens have never been clear. This problem was worked around by using the Blocked method which recognized the most common circumstances where this would happen. However this is a hack: Blocked returning true doesn't mean that the problem will manifest and Blocked returning false doesn't necessarily mean the problem will not manifest. A side effect of this is issue #2151 where sometimes we can't read the memory of a thread and find its associated goroutine. This commit fixes this problem by always reading memory using a thread we know to be good for this, specifically the one returned by ContinueOnce. In particular the changes are as follows: 1. Remove (ProcessInternal).CurrentThread and (ProcessInternal).SetCurrentThread, the "current thread" becomes a field of Target, CurrentThread becomes a (*Target) method and (*Target).SwitchThread basically just sets a field Target. 2. The backends keep track of their own internal idea of what the current thread is, to use it to read memory, this is the thread they return from ContinueOnce as trapthread 3. The current thread in the backend and the current thread in Target only ever get synchronized in two places: when the backend creates a Target object the currentThread field of Target is initialized with the backend's current thread and when (*Target).Restart gets called (when a recording is rewound the currentThread used by Target might not exist anymore). 4. We remove the MemoryReadWriter interface embedded in Thread and instead add a Memory method to Process that returns a MemoryReadWriter. The backends will return something here that will read memory using the current thread saved by the backend. 5. The Thread.Blocked method is removed One possible problem with this change is processes that have threads with different memory maps. As far as I can determine this could happen on old versions of linux but this option was removed in linux 2.5. Fixes #2151 --- pkg/proc/breakpoints.go | 8 ++-- pkg/proc/core/core.go | 36 ++++++++--------- pkg/proc/core/core_test.go | 4 +- pkg/proc/core/linux_core.go | 34 +++++++++-------- pkg/proc/core/windows_amd64_minidump.go | 15 ++++---- pkg/proc/disasm.go | 2 +- pkg/proc/eval.go | 13 ++----- pkg/proc/fncall.go | 4 +- pkg/proc/gdbserial/gdbserver.go | 45 +++++++++++----------- pkg/proc/interface.go | 8 ++-- pkg/proc/linutil/dynamic.go | 6 +-- pkg/proc/native/nonative_darwin.go | 5 --- pkg/proc/native/proc.go | 26 ++++++------- pkg/proc/native/proc_darwin.go | 8 ++-- pkg/proc/native/proc_freebsd.go | 4 +- pkg/proc/native/proc_linux.go | 4 +- pkg/proc/native/proc_windows.go | 4 +- pkg/proc/native/threads.go | 5 +++ pkg/proc/native/threads_darwin.go | 19 --------- pkg/proc/native/threads_freebsd.go | 11 ------ pkg/proc/native/threads_linux.go | 13 ------- pkg/proc/native/threads_windows.go | 20 ---------- pkg/proc/proc_test.go | 30 ++++----------- pkg/proc/stack.go | 4 +- pkg/proc/target.go | 30 ++++++++++----- pkg/proc/target_exec.go | 33 ++++++---------- pkg/proc/threads.go | 16 +------- pkg/proc/variables.go | 11 ++---- pkg/terminal/command_test.go | 51 ++----------------------- service/debugger/debugger.go | 11 +++--- service/test/integration1_test.go | 16 +------- service/test/integration2_test.go | 29 +++++--------- service/test/variables_test.go | 4 +- 33 files changed, 181 insertions(+), 348 deletions(-) diff --git a/pkg/proc/breakpoints.go b/pkg/proc/breakpoints.go index bcd7cb27..443f54b9 100644 --- a/pkg/proc/breakpoints.go +++ b/pkg/proc/breakpoints.go @@ -422,11 +422,11 @@ func (rbpi *returnBreakpointInfo) Collect(thread Thread) []*Variable { g, err := GetG(thread) if err != nil { - return returnInfoError("could not get g", err, thread) + return returnInfoError("could not get g", err, thread.ProcessMemory()) } scope, err := GoroutineScope(thread) if err != nil { - return returnInfoError("could not get scope", err, thread) + return returnInfoError("could not get scope", err, thread.ProcessMemory()) } v, err := scope.evalAST(rbpi.retFrameCond) if err != nil || v.Unreadable != nil || v.Kind != reflect.Bool { @@ -444,12 +444,12 @@ func (rbpi *returnBreakpointInfo) Collect(thread Thread) []*Variable { oldSP := uint64(rbpi.spOffset + int64(g.stack.hi)) err = fakeFunctionEntryScope(scope, rbpi.fn, oldFrameOffset, oldSP) if err != nil { - return returnInfoError("could not read function entry", err, thread) + return returnInfoError("could not read function entry", err, thread.ProcessMemory()) } vars, err := scope.Locals() if err != nil { - return returnInfoError("could not evaluate return variables", err, thread) + return returnInfoError("could not evaluate return variables", err, thread.ProcessMemory()) } vars = filterVariables(vars, func(v *Variable) bool { return (v.Flags & VariableReturnArgument) != 0 diff --git a/pkg/proc/core/core.go b/pkg/proc/core/core.go index abb05b78..93bb277d 100644 --- a/pkg/proc/core/core.go +++ b/pkg/proc/core/core.go @@ -154,9 +154,8 @@ type process struct { entryPoint uint64 - bi *proc.BinaryInfo - breakpoints proc.BreakpointMap - currentThread *thread + bi *proc.BinaryInfo + breakpoints proc.BreakpointMap } var _ proc.ProcessInternal = &process{} @@ -188,7 +187,7 @@ var ( ErrChangeRegisterCore = errors.New("can not change register values of core process") ) -type openFn func(string, string) (*process, error) +type openFn func(string, string) (*process, proc.Thread, error) var openFns = []openFn{readLinuxCore, readAMD64Minidump} @@ -201,9 +200,10 @@ var ErrUnrecognizedFormat = errors.New("unrecognized core format") // for external debug files in the directories passed in. func OpenCore(corePath, exePath string, debugInfoDirs []string) (*proc.Target, error) { var p *process + var currentThread proc.Thread var err error for _, openFn := range openFns { - p, err = openFn(corePath, exePath) + p, currentThread, err = openFn(corePath, exePath) if err != ErrUnrecognizedFormat { break } @@ -212,7 +212,7 @@ func OpenCore(corePath, exePath string, debugInfoDirs []string) (*proc.Target, e return nil, err } - return proc.NewTarget(p, proc.NewTargetConfig{ + return proc.NewTarget(p, currentThread, proc.NewTargetConfig{ Path: exePath, DebugInfoDirs: debugInfoDirs, DisableAsyncPreempt: false, @@ -239,7 +239,7 @@ func (p *process) WriteBreakpoint(addr uint64) (file string, line int, fn *proc. func (p *process) Recorded() (bool, string) { return true, "" } // Restart will only return an error for core files, as they are not executing. -func (p *process) Restart(string) error { return ErrContinueCore } +func (p *process) Restart(string) (proc.Thread, error) { return nil, ErrContinueCore } // ChangeDirection will only return an error as you cannot continue a core process. func (p *process) ChangeDirection(proc.Direction) error { return ErrContinueCore } @@ -262,8 +262,8 @@ func (p *process) ClearCheckpoint(int) error { return errors.New("checkpoint not // ReadMemory will return memory from the core file at the specified location and put the // read memory into `data`, returning the length read, and returning an error if // the length read is shorter than the length of the `data` buffer. -func (t *thread) ReadMemory(data []byte, addr uint64) (n int, err error) { - n, err = t.p.mem.ReadMemory(data, addr) +func (p *process) ReadMemory(data []byte, addr uint64) (n int, err error) { + n, err = p.mem.ReadMemory(data, addr) if err == nil && n != len(data) { err = ErrShortRead } @@ -272,10 +272,15 @@ func (t *thread) ReadMemory(data []byte, addr uint64) (n int, err error) { // WriteMemory will only return an error for core files, you cannot write // to the memory of a core process. -func (t *thread) WriteMemory(addr uint64, data []byte) (int, error) { +func (p *process) WriteMemory(addr uint64, data []byte) (int, error) { return 0, ErrWriteCore } +// ProcessMemory returns the memory of this thread's process. +func (t *thread) ProcessMemory() proc.MemoryReadWriter { + return t.p +} + // Location returns the location of this thread based on // the value of the instruction pointer register. func (t *thread) Location() (*proc.Location, error) { @@ -400,9 +405,9 @@ func (p *process) CheckAndClearManualStopRequest() bool { return false } -// CurrentThread returns the current active thread. -func (p *process) CurrentThread() proc.Thread { - return p.currentThread +// Memory returns the process memory. +func (p *process) Memory() proc.MemoryReadWriter { + return p } // Detach will always return nil and have no @@ -442,8 +447,3 @@ func (p *process) FindThread(threadID int) (proc.Thread, bool) { t, ok := p.Threads[threadID] return t, ok } - -// SetCurrentThread is used internally by proc.Target to change the current thread. -func (p *process) SetCurrentThread(th proc.Thread) { - p.currentThread = th.(*thread) -} diff --git a/pkg/proc/core/core_test.go b/pkg/proc/core/core_test.go index 43e1ebcd..5a0dca5c 100644 --- a/pkg/proc/core/core_test.go +++ b/pkg/proc/core/core_test.go @@ -244,7 +244,7 @@ func TestCore(t *testing.T) { if mainFrame == nil { t.Fatalf("Couldn't find main in stack %v", panickingStack) } - msg, err := proc.FrameToScope(p.BinInfo(), p.CurrentThread(), nil, *mainFrame).EvalVariable("msg", proc.LoadConfig{MaxStringLen: 64}) + msg, err := proc.FrameToScope(p.BinInfo(), p.Memory(), nil, *mainFrame).EvalVariable("msg", proc.LoadConfig{MaxStringLen: 64}) if err != nil { t.Fatalf("Couldn't EvalVariable(msg, ...): %v", err) } @@ -371,7 +371,7 @@ mainSearch: t.Fatal("could not find main.main frame") } - scope := proc.FrameToScope(p.BinInfo(), p.CurrentThread(), nil, *mainFrame) + scope := proc.FrameToScope(p.BinInfo(), p.Memory(), nil, *mainFrame) loadConfig := proc.LoadConfig{FollowPointers: true, MaxVariableRecurse: 1, MaxStringLen: 64, MaxArrayValues: 64, MaxStructFields: -1} v1, err := scope.EvalVariable("t", loadConfig) assertNoError(err, t, "EvalVariable(t)") diff --git a/pkg/proc/core/linux_core.go b/pkg/proc/core/linux_core.go index 77c5e108..6861451a 100644 --- a/pkg/proc/core/linux_core.go +++ b/pkg/proc/core/linux_core.go @@ -42,7 +42,8 @@ const ( const elfErrorBadMagicNumber = "bad magic number" -func linuxThreadsFromNotes(p *process, notes []*note, machineType elf.Machine) { +func linuxThreadsFromNotes(p *process, notes []*note, machineType elf.Machine) proc.Thread { + var currentThread proc.Thread var lastThreadAMD *linuxAMD64Thread var lastThreadARM *linuxARM64Thread for _, note := range notes { @@ -52,15 +53,15 @@ func linuxThreadsFromNotes(p *process, notes []*note, machineType elf.Machine) { t := note.Desc.(*linuxPrStatusAMD64) lastThreadAMD = &linuxAMD64Thread{linutil.AMD64Registers{Regs: &t.Reg}, t} p.Threads[int(t.Pid)] = &thread{lastThreadAMD, p, proc.CommonThread{}} - if p.currentThread == nil { - p.currentThread = p.Threads[int(t.Pid)] + if currentThread == nil { + currentThread = p.Threads[int(t.Pid)] } } else if machineType == _EM_AARCH64 { t := note.Desc.(*linuxPrStatusARM64) lastThreadARM = &linuxARM64Thread{linutil.ARM64Registers{Regs: &t.Reg}, t} p.Threads[int(t.Pid)] = &thread{lastThreadARM, p, proc.CommonThread{}} - if p.currentThread == nil { - p.currentThread = p.Threads[int(t.Pid)] + if currentThread == nil { + currentThread = p.Threads[int(t.Pid)] } } case _NT_FPREGSET: @@ -79,6 +80,7 @@ func linuxThreadsFromNotes(p *process, notes []*note, machineType elf.Machine) { p.pid = int(note.Desc.(*linuxPrPsInfo).Pid) } } + return currentThread } // readLinuxCore reads a core file from corePath corresponding to the executable at @@ -87,35 +89,35 @@ func linuxThreadsFromNotes(p *process, notes []*note, machineType elf.Machine) { // http://uhlo.blogspot.fr/2012/05/brief-look-into-core-dumps.html, // elf_core_dump in http://lxr.free-electrons.com/source/fs/binfmt_elf.c, // and, if absolutely desperate, readelf.c from the binutils source. -func readLinuxCore(corePath, exePath string) (*process, error) { +func readLinuxCore(corePath, exePath string) (*process, proc.Thread, error) { coreFile, err := elf.Open(corePath) if err != nil { if _, isfmterr := err.(*elf.FormatError); isfmterr && (strings.Contains(err.Error(), elfErrorBadMagicNumber) || strings.Contains(err.Error(), " at offset 0x0: too short")) { // Go >=1.11 and <1.11 produce different errors when reading a non-elf file. - return nil, ErrUnrecognizedFormat + return nil, nil, ErrUnrecognizedFormat } - return nil, err + return nil, nil, err } exe, err := os.Open(exePath) if err != nil { - return nil, err + return nil, nil, err } exeELF, err := elf.NewFile(exe) if err != nil { - return nil, err + return nil, nil, err } if coreFile.Type != elf.ET_CORE { - return nil, fmt.Errorf("%v is not a core file", coreFile) + return nil, nil, fmt.Errorf("%v is not a core file", coreFile) } if exeELF.Type != elf.ET_EXEC && exeELF.Type != elf.ET_DYN { - return nil, fmt.Errorf("%v is not an exe file", exeELF) + return nil, nil, fmt.Errorf("%v is not an exe file", exeELF) } machineType := exeELF.Machine notes, err := readNotes(coreFile, machineType) if err != nil { - return nil, err + return nil, nil, err } memory := buildMemory(coreFile, exeELF, exe, notes) @@ -127,7 +129,7 @@ func readLinuxCore(corePath, exePath string) (*process, error) { case _EM_AARCH64: bi = proc.NewBinaryInfo("linux", "arm64") default: - return nil, fmt.Errorf("unsupported machine type") + return nil, nil, fmt.Errorf("unsupported machine type") } entryPoint := findEntryPoint(notes, bi.Arch.PtrSize()) @@ -140,8 +142,8 @@ func readLinuxCore(corePath, exePath string) (*process, error) { breakpoints: proc.NewBreakpointMap(), } - linuxThreadsFromNotes(p, notes, machineType) - return p, nil + currentThread := linuxThreadsFromNotes(p, notes, machineType) + return p, currentThread, nil } type linuxAMD64Thread struct { diff --git a/pkg/proc/core/windows_amd64_minidump.go b/pkg/proc/core/windows_amd64_minidump.go index 554a66f4..aeadd11a 100644 --- a/pkg/proc/core/windows_amd64_minidump.go +++ b/pkg/proc/core/windows_amd64_minidump.go @@ -7,7 +7,7 @@ import ( "github.com/go-delve/delve/pkg/proc/winutil" ) -func readAMD64Minidump(minidumpPath, exePath string) (*process, error) { +func readAMD64Minidump(minidumpPath, exePath string) (*process, proc.Thread, error) { var logfn func(string, ...interface{}) if logflags.Minidump() { logfn = logflags.MinidumpLogger().Infof @@ -16,9 +16,9 @@ func readAMD64Minidump(minidumpPath, exePath string) (*process, error) { mdmp, err := minidump.Open(minidumpPath, logfn) if err != nil { if _, isNotAMinidump := err.(minidump.ErrNotAMinidump); isNotAMinidump { - return nil, ErrUnrecognizedFormat + return nil, nil, ErrUnrecognizedFormat } - return nil, err + return nil, nil, err } memory := &splicedMemory{} @@ -45,11 +45,12 @@ func readAMD64Minidump(minidumpPath, exePath string) (*process, error) { for i := range mdmp.Threads { th := &mdmp.Threads[i] p.Threads[int(th.ID)] = &thread{&windowsAMD64Thread{th}, p, proc.CommonThread{}} - if p.currentThread == nil { - p.currentThread = p.Threads[int(th.ID)] - } } - return p, nil + var currentThread proc.Thread + if len(mdmp.Threads) > 0 { + currentThread = p.Threads[int(mdmp.Threads[0].ID)] + } + return p, currentThread, nil } type windowsAMD64Thread struct { diff --git a/pkg/proc/disasm.go b/pkg/proc/disasm.go index 808a11e9..8d5d50b9 100644 --- a/pkg/proc/disasm.go +++ b/pkg/proc/disasm.go @@ -71,7 +71,7 @@ type opcodeSeq []uint64 // If sameline is set firstPCAfterPrologueDisassembly will always return an // address associated with the same line as fn.Entry func firstPCAfterPrologueDisassembly(p Process, fn *Function, sameline bool) (uint64, error) { - var mem MemoryReadWriter = p.CurrentThread() + mem := p.Memory() breakpoints := p.Breakpoints() bi := p.BinInfo() text, err := disassemble(mem, nil, breakpoints, bi, fn.Entry, fn.End, false) diff --git a/pkg/proc/eval.go b/pkg/proc/eval.go index 6563e275..07c1608d 100644 --- a/pkg/proc/eval.go +++ b/pkg/proc/eval.go @@ -74,13 +74,6 @@ func ConvertEvalScope(dbp *Target, gid, frame, deferCall int) (*EvalScope, error return ThreadScope(ct) } - var thread MemoryReadWriter - if g.Thread == nil { - thread = ct - } else { - thread = g.Thread - } - var opts StacktraceOptions if deferCall > 0 { opts = StacktraceReadDefers @@ -108,7 +101,7 @@ func ConvertEvalScope(dbp *Target, gid, frame, deferCall int) (*EvalScope, error return d.EvalScope(ct) } - return FrameToScope(dbp.BinInfo(), thread, g, locs[frame:]...), nil + return FrameToScope(dbp.BinInfo(), dbp.Memory(), g, locs[frame:]...), nil } // FrameToScope returns a new EvalScope for frames[0]. @@ -145,7 +138,7 @@ func ThreadScope(thread Thread) (*EvalScope, error) { if len(locations) < 1 { return nil, errors.New("could not decode first frame") } - return FrameToScope(thread.BinInfo(), thread, nil, locations...), nil + return FrameToScope(thread.BinInfo(), thread.ProcessMemory(), nil, locations...), nil } // GoroutineScope returns an EvalScope for the goroutine running on the given thread. @@ -161,7 +154,7 @@ func GoroutineScope(thread Thread) (*EvalScope, error) { if err != nil { return nil, err } - return FrameToScope(thread.BinInfo(), thread, g, locations...), nil + return FrameToScope(thread.BinInfo(), thread.ProcessMemory(), g, locations...), nil } // EvalExpression returns the value of the given expression. diff --git a/pkg/proc/fncall.go b/pkg/proc/fncall.go index da7157c2..7ce7ee29 100644 --- a/pkg/proc/fncall.go +++ b/pkg/proc/fncall.go @@ -309,7 +309,7 @@ func evalFunctionCall(scope *EvalScope, node *ast.CallExpr) (*Variable, error) { return nil, err } // write the desired argument frame size at SP-(2*pointer_size) (the extra pointer is the saved PC) - if err := writePointer(bi, thread, regs.SP()-3*uint64(bi.Arch.PtrSize()), uint64(fncall.argFrameSize)); err != nil { + if err := writePointer(bi, scope.Mem, regs.SP()-3*uint64(bi.Arch.PtrSize()), uint64(fncall.argFrameSize)); err != nil { return nil, err } @@ -422,7 +422,7 @@ func callOP(bi *BinaryInfo, thread Thread, regs Registers, callAddr uint64) erro if err := thread.SetSP(sp); err != nil { return err } - if err := writePointer(bi, thread, sp, regs.PC()); err != nil { + if err := writePointer(bi, thread.ProcessMemory(), sp, regs.PC()); err != nil { return err } return thread.SetPC(callAddr) diff --git a/pkg/proc/gdbserial/gdbserver.go b/pkg/proc/gdbserial/gdbserver.go index b1937b86..843e0a30 100644 --- a/pkg/proc/gdbserial/gdbserver.go +++ b/pkg/proc/gdbserial/gdbserver.go @@ -576,7 +576,7 @@ func (p *gdbProcess) initialize(path string, debugInfoDirs []string, stopReason return nil, err } } - tgt, err := proc.NewTarget(p, proc.NewTargetConfig{ + tgt, err := proc.NewTarget(p, p.currentThread, proc.NewTargetConfig{ Path: path, DebugInfoDirs: debugInfoDirs, DisableAsyncPreempt: runtime.GOOS == "darwin", @@ -649,15 +649,9 @@ func (p *gdbProcess) ThreadList() []proc.Thread { return r } -// CurrentThread returns the current active -// selected thread. -func (p *gdbProcess) CurrentThread() proc.Thread { - return p.currentThread -} - -// SetCurrentThread is used internally by proc.Target to change the current thread. -func (p *gdbProcess) SetCurrentThread(th proc.Thread) { - p.currentThread = th.(*gdbThread) +// Memory returns the process memory. +func (p *gdbProcess) Memory() proc.MemoryReadWriter { + return p } const ( @@ -775,6 +769,7 @@ continueLoop: // the signals that are reported here can not be propagated back to the target process. trapthread.sig = 0 } + p.currentThread = trapthread return trapthread, stopReason, err } @@ -937,9 +932,9 @@ func (p *gdbProcess) Detach(kill bool) error { } // Restart will restart the process from the given position. -func (p *gdbProcess) Restart(pos string) error { +func (p *gdbProcess) Restart(pos string) (proc.Thread, error) { if p.tracedir == "" { - return proc.ErrNotRecorded + return nil, proc.ErrNotRecorded } p.exited = false @@ -952,19 +947,19 @@ func (p *gdbProcess) Restart(pos string) error { err := p.conn.restart(pos) if err != nil { - return err + return nil, err } // for some reason we have to send a vCont;c after a vRun to make rr behave // properly, because that's what gdb does. _, _, err = p.conn.resume(nil, nil) if err != nil { - return err + return nil, err } err = p.updateThreadList(&threadUpdater{p: p}) if err != nil { - return err + return nil, err } p.clearThreadSignals() p.clearThreadRegisters() @@ -973,7 +968,7 @@ func (p *gdbProcess) Restart(pos string) error { p.conn.setBreakpoint(addr) } - return p.setCurrentBreakpoints() + return p.currentThread, p.setCurrentBreakpoints() } // When executes the 'when' command for the Mozilla RR backend. @@ -1265,8 +1260,8 @@ func (p *gdbProcess) setCurrentBreakpoints() error { } // ReadMemory will read into 'data' memory at the address provided. -func (t *gdbThread) ReadMemory(data []byte, addr uint64) (n int, err error) { - err = t.p.conn.readMemory(data, addr) +func (p *gdbProcess) ReadMemory(data []byte, addr uint64) (n int, err error) { + err = p.conn.readMemory(data, addr) if err != nil { return 0, err } @@ -1274,8 +1269,12 @@ func (t *gdbThread) ReadMemory(data []byte, addr uint64) (n int, err error) { } // WriteMemory will write into the memory at 'addr' the data provided. -func (t *gdbThread) WriteMemory(addr uint64, data []byte) (written int, err error) { - return t.p.conn.writeMemory(addr, data) +func (p *gdbProcess) WriteMemory(addr uint64, data []byte) (written int, err error) { + return p.conn.writeMemory(addr, data) +} + +func (t *gdbThread) ProcessMemory() proc.MemoryReadWriter { + return t.p } // Location returns the current location of this thread. @@ -1522,18 +1521,18 @@ func (t *gdbThread) reloadGAtPC() error { } savedcode := make([]byte, len(movinstr)) - _, err := t.ReadMemory(savedcode, pc) + _, err := t.p.ReadMemory(savedcode, pc) if err != nil { return err } - _, err = t.WriteMemory(pc, movinstr) + _, err = t.p.WriteMemory(pc, movinstr) if err != nil { return err } defer func() { - _, err0 := t.WriteMemory(pc, savedcode) + _, err0 := t.p.WriteMemory(pc, savedcode) if err == nil { err = err0 } diff --git a/pkg/proc/interface.go b/pkg/proc/interface.go index cd1ad0c8..f785480f 100644 --- a/pkg/proc/interface.go +++ b/pkg/proc/interface.go @@ -13,6 +13,9 @@ type Process interface { RecordingManipulation Breakpoints() *BreakpointMap + + // Memory returns a memory read/writer for this process's memory. + Memory() MemoryReadWriter } // ProcessInternal holds a set of methods that are not meant to be called by @@ -21,12 +24,12 @@ type Process interface { // the `proc` package. // This is temporary and in support of an ongoing refactor. type ProcessInternal interface { - SetCurrentThread(Thread) // Restart restarts the recording from the specified position, or from the // last checkpoint if pos == "". // If pos starts with 'c' it's a checkpoint ID, otherwise it's an event // number. - Restart(pos string) error + // Returns the new current thread after the restart has completed. + Restart(pos string) (Thread, error) Detach(bool) error ContinueOnce() (trapthread Thread, stopReason StopReason, err error) @@ -91,7 +94,6 @@ type Info interface { type ThreadInfo interface { FindThread(threadID int) (Thread, bool) ThreadList() []Thread - CurrentThread() Thread } // ProcessManipulation is an interface for changing the execution state of a process. diff --git a/pkg/proc/linutil/dynamic.go b/pkg/proc/linutil/dynamic.go index 46eab4db..2e02a774 100644 --- a/pkg/proc/linutil/dynamic.go +++ b/pkg/proc/linutil/dynamic.go @@ -44,7 +44,7 @@ func readUintRaw(reader io.Reader, order binary.ByteOrder, ptrSize int) (uint64, // dynamicSearchDebug searches for the DT_DEBUG entry in the .dynamic section func dynamicSearchDebug(p proc.Process) (uint64, error) { bi := p.BinInfo() - mem := p.CurrentThread() + mem := p.Memory() dynbuf := make([]byte, bi.ElfDynamicSection.Size) _, err := mem.ReadMemory(dynbuf, bi.ElfDynamicSection.Addr) @@ -73,7 +73,7 @@ func dynamicSearchDebug(p proc.Process) (uint64, error) { func readPtr(p proc.Process, addr uint64) (uint64, error) { ptrbuf := make([]byte, p.BinInfo().Arch.PtrSize()) - _, err := p.CurrentThread().ReadMemory(ptrbuf, addr) + _, err := p.Memory().ReadMemory(ptrbuf, addr) if err != nil { return 0, err } @@ -115,7 +115,7 @@ func readCString(p proc.Process, addr uint64) (string, error) { if addr == 0 { return "", nil } - mem := p.CurrentThread() + mem := p.Memory() buf := make([]byte, 1) r := []byte{} for { diff --git a/pkg/proc/native/nonative_darwin.go b/pkg/proc/native/nonative_darwin.go index 06009a27..9531239c 100644 --- a/pkg/proc/native/nonative_darwin.go +++ b/pkg/proc/native/nonative_darwin.go @@ -81,11 +81,6 @@ func (dbp *nativeProcess) EntryPoint() (uint64, error) { panic(ErrNativeBackendDisabled) } -// Blocked returns true if the thread is blocked -func (t *nativeThread) Blocked() bool { - panic(ErrNativeBackendDisabled) -} - // SetPC sets the value of the PC register. func (t *nativeThread) SetPC(pc uint64) error { panic(ErrNativeBackendDisabled) diff --git a/pkg/proc/native/proc.go b/pkg/proc/native/proc.go index fdd72061..6509bf54 100644 --- a/pkg/proc/native/proc.go +++ b/pkg/proc/native/proc.go @@ -22,8 +22,8 @@ type nativeProcess struct { // List of threads mapped as such: pid -> *Thread threads map[int]*nativeThread - // Active thread - currentThread *nativeThread + // Thread used to read and write memory + memthread *nativeThread os *osProcessDetails firstStart bool @@ -72,7 +72,7 @@ func (dbp *nativeProcess) Recorded() (bool, string) { return false, "" } // Restart will always return an error in the native proc backend, only for // recorded traces. -func (dbp *nativeProcess) Restart(string) error { return proc.ErrNotRecorded } +func (dbp *nativeProcess) Restart(string) (proc.Thread, error) { return nil, proc.ErrNotRecorded } // ChangeDirection will always return an error in the native proc backend, only for // recorded traces. @@ -166,14 +166,9 @@ func (dbp *nativeProcess) FindThread(threadID int) (proc.Thread, bool) { return th, ok } -// CurrentThread returns the current selected, active thread. -func (dbp *nativeProcess) CurrentThread() proc.Thread { - return dbp.currentThread -} - -// SetCurrentThread is used internally by proc.Target to change the current thread. -func (p *nativeProcess) SetCurrentThread(th proc.Thread) { - p.currentThread = th.(*nativeThread) +// Memory returns the process memory. +func (dbp *nativeProcess) Memory() proc.MemoryReadWriter { + return dbp.memthread } // Breakpoints returns a list of breakpoints currently set. @@ -209,11 +204,11 @@ func (dbp *nativeProcess) WriteBreakpoint(addr uint64) (string, int, *proc.Funct f, l, fn := dbp.bi.PCToLine(uint64(addr)) originalData := make([]byte, dbp.bi.Arch.BreakpointSize()) - _, err := dbp.currentThread.ReadMemory(originalData, addr) + _, err := dbp.memthread.ReadMemory(originalData, addr) if err != nil { return "", 0, nil, nil, err } - if err := dbp.writeSoftwareBreakpoint(dbp.currentThread, addr); err != nil { + if err := dbp.writeSoftwareBreakpoint(dbp.memthread, addr); err != nil { return "", 0, nil, nil, err } @@ -221,7 +216,7 @@ func (dbp *nativeProcess) WriteBreakpoint(addr uint64) (string, int, *proc.Funct } func (dbp *nativeProcess) EraseBreakpoint(bp *proc.Breakpoint) error { - return dbp.currentThread.ClearBreakpoint(bp) + return dbp.memthread.ClearBreakpoint(bp) } // ContinueOnce will continue the target until it stops. @@ -255,6 +250,7 @@ func (dbp *nativeProcess) ContinueOnce() (proc.Thread, proc.StopReason, error) { return nil, proc.StopUnknown, err } if trapthread != nil { + dbp.memthread = trapthread return trapthread, proc.StopUnknown, nil } } @@ -288,7 +284,7 @@ func (dbp *nativeProcess) initialize(path string, debugInfoDirs []string) (*proc if !dbp.childProcess { stopReason = proc.StopAttached } - return proc.NewTarget(dbp, proc.NewTargetConfig{ + return proc.NewTarget(dbp, dbp.memthread, proc.NewTargetConfig{ Path: path, DebugInfoDirs: debugInfoDirs, DisableAsyncPreempt: runtime.GOOS == "windows" || runtime.GOOS == "freebsd", diff --git a/pkg/proc/native/proc_darwin.go b/pkg/proc/native/proc_darwin.go index 3df1522e..f44aa312 100644 --- a/pkg/proc/native/proc_darwin.go +++ b/pkg/proc/native/proc_darwin.go @@ -118,7 +118,7 @@ func Launch(cmd []string, wd string, flags proc.LaunchFlags, _ []string, _ strin } dbp.os.initialized = true - dbp.currentThread = trapthread + dbp.memthread = trapthread tgt, err := dbp.initialize(argv0Go, []string{}) if err != nil { @@ -188,7 +188,7 @@ func (dbp *nativeProcess) kill() (err error) { func (dbp *nativeProcess) requestManualStop() (err error) { var ( task = C.mach_port_t(dbp.os.task) - thread = C.mach_port_t(dbp.currentThread.os.threadAct) + thread = C.mach_port_t(dbp.memthread.os.threadAct) exceptionPort = C.mach_port_t(dbp.os.exceptionPort) ) dbp.os.halt = true @@ -267,8 +267,8 @@ func (dbp *nativeProcess) addThread(port int, attach bool) (*nativeThread, error } dbp.threads[port] = thread thread.os.threadAct = C.thread_act_t(port) - if dbp.currentThread == nil { - dbp.currentThread = thread + if dbp.memthread == nil { + dbp.memthread = thread } return thread, nil } diff --git a/pkg/proc/native/proc_freebsd.go b/pkg/proc/native/proc_freebsd.go index 7f5a854f..a2a3ab16 100644 --- a/pkg/proc/native/proc_freebsd.go +++ b/pkg/proc/native/proc_freebsd.go @@ -180,8 +180,8 @@ func (dbp *nativeProcess) addThread(tid int, attach bool) (*nativeThread, error) os: new(osSpecificDetails), } - if dbp.currentThread == nil { - dbp.currentThread = dbp.threads[tid] + if dbp.memthread == nil { + dbp.memthread = dbp.threads[tid] } return dbp.threads[tid], nil diff --git a/pkg/proc/native/proc_linux.go b/pkg/proc/native/proc_linux.go index d85a992a..40255276 100644 --- a/pkg/proc/native/proc_linux.go +++ b/pkg/proc/native/proc_linux.go @@ -253,8 +253,8 @@ func (dbp *nativeProcess) addThread(tid int, attach bool) (*nativeThread, error) dbp: dbp, os: new(osSpecificDetails), } - if dbp.currentThread == nil { - dbp.currentThread = dbp.threads[tid] + if dbp.memthread == nil { + dbp.memthread = dbp.threads[tid] } return dbp.threads[tid], nil } diff --git a/pkg/proc/native/proc_windows.go b/pkg/proc/native/proc_windows.go index f3f86420..d9ee5999 100644 --- a/pkg/proc/native/proc_windows.go +++ b/pkg/proc/native/proc_windows.go @@ -206,8 +206,8 @@ func (dbp *nativeProcess) addThread(hThread syscall.Handle, threadID int, attach } thread.os.hThread = hThread dbp.threads[threadID] = thread - if dbp.currentThread == nil { - dbp.currentThread = dbp.threads[threadID] + if dbp.memthread == nil { + dbp.memthread = dbp.threads[threadID] } if suspendNewThreads { _, err := _SuspendThread(thread.os.hThread) diff --git a/pkg/proc/native/threads.go b/pkg/proc/native/threads.go index 65b5ab75..c15d63bd 100644 --- a/pkg/proc/native/threads.go +++ b/pkg/proc/native/threads.go @@ -175,3 +175,8 @@ func (t *nativeThread) PC() (uint64, error) { } return regs.PC(), nil } + +// ProcessMemory returns this thread's process memory. +func (t *nativeThread) ProcessMemory() proc.MemoryReadWriter { + return t.dbp.Memory() +} diff --git a/pkg/proc/native/threads_darwin.go b/pkg/proc/native/threads_darwin.go index 183ddff9..fe12bede 100644 --- a/pkg/proc/native/threads_darwin.go +++ b/pkg/proc/native/threads_darwin.go @@ -85,25 +85,6 @@ func (t *nativeThread) resume() error { return nil } -func (t *nativeThread) Blocked() bool { - // TODO(dp) cache the func pc to remove this lookup - regs, err := t.Registers() - if err != nil { - return false - } - pc := regs.PC() - fn := t.BinInfo().PCToFunc(pc) - if fn == nil { - return false - } - switch fn.Name { - case "runtime.kevent", "runtime.mach_semaphore_wait", "runtime.usleep", "runtime.mach_semaphore_timedwait": - return true - default: - return false - } -} - // Stopped returns whether the thread is stopped at // the operating system level. func (t *nativeThread) Stopped() bool { diff --git a/pkg/proc/native/threads_freebsd.go b/pkg/proc/native/threads_freebsd.go index 6e7cf427..4af26e1b 100644 --- a/pkg/proc/native/threads_freebsd.go +++ b/pkg/proc/native/threads_freebsd.go @@ -75,17 +75,6 @@ func (t *nativeThread) singleStep() (err error) { return nil } -func (t *nativeThread) Blocked() bool { - loc, err := t.Location() - if err != nil { - return false - } - if loc.Fn != nil && ((loc.Fn.Name == "runtime.futex") || (loc.Fn.Name == "runtime.usleep") || (loc.Fn.Name == "runtime.clone")) { - return true - } - return false -} - func (t *nativeThread) restoreRegisters(savedRegs proc.Registers) error { sr := savedRegs.(*fbsdutil.AMD64Registers) diff --git a/pkg/proc/native/threads_linux.go b/pkg/proc/native/threads_linux.go index 90af3240..d0e978c0 100644 --- a/pkg/proc/native/threads_linux.go +++ b/pkg/proc/native/threads_linux.go @@ -71,19 +71,6 @@ func (t *nativeThread) singleStep() (err error) { } } -func (t *nativeThread) Blocked() bool { - regs, err := t.Registers() - if err != nil { - return false - } - pc := regs.PC() - fn := t.BinInfo().PCToFunc(pc) - if fn != nil && ((fn.Name == "runtime.futex") || (fn.Name == "runtime.usleep") || (fn.Name == "runtime.clone")) { - return true - } - return false -} - func (t *nativeThread) WriteMemory(addr uint64, data []byte) (written int, err error) { if t.dbp.exited { return 0, proc.ErrProcessExited{Pid: t.dbp.pid} diff --git a/pkg/proc/native/threads_windows.go b/pkg/proc/native/threads_windows.go index 610b75c4..887a639d 100644 --- a/pkg/proc/native/threads_windows.go +++ b/pkg/proc/native/threads_windows.go @@ -111,26 +111,6 @@ func (t *nativeThread) resume() error { return err } -func (t *nativeThread) Blocked() bool { - // TODO: Probably incorrect - what are the runtime functions that - // indicate blocking on Windows? - regs, err := t.Registers() - if err != nil { - return false - } - pc := regs.PC() - fn := t.BinInfo().PCToFunc(pc) - if fn == nil { - return false - } - switch fn.Name { - case "runtime.kevent", "runtime.usleep": - return true - default: - return false - } -} - // Stopped returns whether the thread is stopped at the operating system // level. On windows this always returns true. func (t *nativeThread) Stopped() bool { diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go index ad2720b9..f33ddac3 100644 --- a/pkg/proc/proc_test.go +++ b/pkg/proc/proc_test.go @@ -359,7 +359,7 @@ func TestClearBreakpointBreakpoint(t *testing.T) { _, err := p.ClearBreakpoint(bp.Addr) assertNoError(err, t, "ClearBreakpoint()") - data, err := dataAtAddr(p.CurrentThread(), bp.Addr) + data, err := dataAtAddr(p.Memory(), bp.Addr) assertNoError(err, t, "dataAtAddr") int3 := []byte{0xcc} @@ -949,19 +949,6 @@ func TestStacktraceGoroutine(t *testing.T) { {{10, "main.agoroutine"}}, } - tgtAgoroutineCount := 10 - - if goversion.VersionAfterOrEqual(runtime.Version(), 1, 14) { - // We try to make sure that all goroutines are stopped at a sensible place - // before reading their stacktrace, but due to the nature of the test - // program there is no guarantee that we always find them in a reasonable - // state. - // Asynchronous preemption in Go 1.14 exacerbates this problem, to avoid - // unnecessary flakiness reduce the target count to 9, allowing one - // goroutine to be in a bad state. - tgtAgoroutineCount = 9 - } - protest.AllowRecording(t) withTestProcess("goroutinestackprog", t, func(p *proc.Target, fixture protest.Fixture) { bp := setFunctionBreakpoint(p, t, "main.stacktraceme") @@ -1011,7 +998,7 @@ func TestStacktraceGoroutine(t *testing.T) { t.Fatalf("Main goroutine stack not found %d", mainCount) } - if agoroutineCount < tgtAgoroutineCount { + if agoroutineCount < 10 { t.Fatalf("Goroutine stacks not found (%d)", agoroutineCount) } @@ -1154,7 +1141,7 @@ func evalVariableOrError(p *proc.Target, symbol string) (*proc.Variable, error) var frame proc.Stackframe frame, err = findFirstNonRuntimeFrame(p) if err == nil { - scope = proc.FrameToScope(p.BinInfo(), p.CurrentThread(), nil, frame) + scope = proc.FrameToScope(p.BinInfo(), p.Memory(), nil, frame) } } else { scope, err = proc.GoroutineScope(p.CurrentThread()) @@ -2531,7 +2518,7 @@ func TestStepOnCallPtrInstr(t *testing.T) { regs, err := p.CurrentThread().Registers() assertNoError(err, t, "Registers()") pc := regs.PC() - text, err := proc.Disassemble(p.CurrentThread(), regs, p.Breakpoints(), p.BinInfo(), pc, pc+uint64(p.BinInfo().Arch.MaxInstructionLength())) + text, err := proc.Disassemble(p.Memory(), regs, p.Breakpoints(), p.BinInfo(), pc, pc+uint64(p.BinInfo().Arch.MaxInstructionLength())) assertNoError(err, t, "Disassemble()") if text[0].IsCall() { found = true @@ -3008,9 +2995,6 @@ func TestIssue893(t *testing.T) { if _, ok := err.(*frame.ErrNoFDEForPC); ok { return } - if _, ok := err.(proc.ErrThreadBlocked); ok { - return - } if _, ok := err.(*proc.ErrNoSourceForPC); ok { return } @@ -3041,7 +3025,7 @@ func TestIssue871(t *testing.T) { var frame proc.Stackframe frame, err = findFirstNonRuntimeFrame(p) if err == nil { - scope = proc.FrameToScope(p.BinInfo(), p.CurrentThread(), nil, frame) + scope = proc.FrameToScope(p.BinInfo(), p.Memory(), nil, frame) } } else { scope, err = proc.GoroutineScope(p.CurrentThread()) @@ -3488,7 +3472,7 @@ func TestIssue1034(t *testing.T) { assertNoError(p.Continue(), t, "Continue()") frames, err := p.SelectedGoroutine().Stacktrace(10, 0) assertNoError(err, t, "Stacktrace") - scope := proc.FrameToScope(p.BinInfo(), p.CurrentThread(), nil, frames[2:]...) + scope := proc.FrameToScope(p.BinInfo(), p.Memory(), nil, frames[2:]...) args, _ := scope.FunctionArguments(normalLoadConfig) assertNoError(err, t, "FunctionArguments()") if len(args) > 0 { @@ -3663,7 +3647,7 @@ func TestDisassembleGlobalVars(t *testing.T) { withTestProcess("teststepconcurrent", t, func(p *proc.Target, fixture protest.Fixture) { mainfn := p.BinInfo().LookupFunc["main.main"] regs, _ := p.CurrentThread().Registers() - text, err := proc.Disassemble(p.CurrentThread(), regs, p.Breakpoints(), p.BinInfo(), mainfn.Entry, mainfn.End) + text, err := proc.Disassemble(p.Memory(), regs, p.Breakpoints(), p.BinInfo(), mainfn.Entry, mainfn.End) assertNoError(err, t, "Disassemble") found := false for i := range text { diff --git a/pkg/proc/stack.go b/pkg/proc/stack.go index a6d17ade..4bc454ea 100644 --- a/pkg/proc/stack.go +++ b/pkg/proc/stack.go @@ -100,7 +100,7 @@ func ThreadStacktrace(thread Thread, depth int) ([]Stackframe, error) { return nil, err } so := thread.BinInfo().PCToImage(regs.PC()) - it := newStackIterator(thread.BinInfo(), thread, thread.BinInfo().Arch.RegistersToDwarfRegisters(so.StaticBase, regs), 0, nil, -1, nil, 0) + it := newStackIterator(thread.BinInfo(), thread.ProcessMemory(), thread.BinInfo().Arch.RegistersToDwarfRegisters(so.StaticBase, regs), 0, nil, -1, nil, 0) return it.stacktrace(depth) } return g.Stacktrace(depth, 0) @@ -120,7 +120,7 @@ func (g *G) stackIterator(opts StacktraceOptions) (*stackIterator, error) { } so := bi.PCToImage(regs.PC()) return newStackIterator( - bi, g.Thread, + bi, g.variable.mem, bi.Arch.RegistersToDwarfRegisters(so.StaticBase, regs), g.stack.hi, stkbar, g.stkbarPos, g, opts), nil } diff --git a/pkg/proc/target.go b/pkg/proc/target.go index 1eafc43c..59b91fe7 100644 --- a/pkg/proc/target.go +++ b/pkg/proc/target.go @@ -41,6 +41,9 @@ type Target struct { // case only one will be reported. StopReason StopReason + // currentThread is the thread that will be used by next/step/stepout and to evaluate variables if no goroutine is selected. + currentThread Thread + // Goroutine that will be used by default to set breakpoint, eval variables, etc... // Normally selectedGoroutine is currentThread.GetG, it will not be only if SwitchGoroutine is called with a goroutine that isn't attached to a thread selectedGoroutine *G @@ -108,7 +111,7 @@ func DisableAsyncPreemptEnv() []string { } // NewTarget returns an initialized Target object. -func NewTarget(p Process, cfg NewTargetConfig) (*Target, error) { +func NewTarget(p Process, currentThread Thread, cfg NewTargetConfig) (*Target, error) { entryPoint, err := p.EntryPoint() if err != nil { return nil, err @@ -125,13 +128,14 @@ func NewTarget(p Process, cfg NewTargetConfig) (*Target, error) { } t := &Target{ - Process: p, - proc: p.(ProcessInternal), - fncallForG: make(map[int]*callInjection), - StopReason: cfg.StopReason, + Process: p, + proc: p.(ProcessInternal), + fncallForG: make(map[int]*callInjection), + StopReason: cfg.StopReason, + currentThread: currentThread, } - g, _ := GetG(p.CurrentThread()) + g, _ := GetG(currentThread) t.selectedGoroutine = g t.createUnrecoveredPanicBreakpoint() @@ -171,10 +175,11 @@ func (t *Target) ClearAllGCache() { // Restarting of a normal process happens at a higher level (debugger.Restart). func (t *Target) Restart(from string) error { t.ClearAllGCache() - err := t.proc.Restart(from) + currentThread, err := t.proc.Restart(from) if err != nil { return err } + t.currentThread = currentThread t.selectedGoroutine, _ = GetG(t.CurrentThread()) if from != "" { t.StopReason = StopManual @@ -210,7 +215,7 @@ func (p *Target) SwitchThread(tid int) error { return err } if th, ok := p.FindThread(tid); ok { - p.proc.SetCurrentThread(th) + p.currentThread = th p.selectedGoroutine, _ = GetG(p.CurrentThread()) return nil } @@ -247,7 +252,7 @@ func setAsyncPreemptOff(p *Target, v int64) { return } logger := p.BinInfo().logger - scope := globalScope(p.BinInfo(), p.BinInfo().Images[0], p.CurrentThread()) + scope := globalScope(p.BinInfo(), p.BinInfo().Images[0], p.Memory()) debugv, err := scope.findGlobal("runtime", "debug") if err != nil || debugv.Unreadable != nil { logger.Warnf("could not find runtime/debug variable (or unreadable): %v %v", err, debugv.Unreadable) @@ -295,3 +300,10 @@ func (t *Target) createFatalThrowBreakpoint() { } } } + +// CurrentThread returns the currently selected thread which will be used +// for next/step/stepout and for reading variables, unless a goroutine is +// selected. +func (t *Target) CurrentThread() Thread { + return t.currentThread +} diff --git a/pkg/proc/target_exec.go b/pkg/proc/target_exec.go index 30102819..9aad77c5 100644 --- a/pkg/proc/target_exec.go +++ b/pkg/proc/target_exec.go @@ -142,7 +142,7 @@ func (dbp *Target) Continue() error { if !arch.BreakInstrMovesPC() { bpsize := arch.BreakpointSize() bp := make([]byte, bpsize) - _, err = dbp.CurrentThread().ReadMemory(bp, loc.PC) + _, err = dbp.Memory().ReadMemory(bp, loc.PC) if bytes.Equal(bp, arch.BreakpointInstruction()) { curthread.SetPC(loc.PC + uint64(bpsize)) } @@ -254,7 +254,7 @@ func disassembleCurrentInstruction(p Process, thread Thread, off int64) ([]AsmIn return nil, err } pc := regs.PC() + uint64(off) - return disassemble(thread, regs, p.Breakpoints(), p.BinInfo(), pc, pc+uint64(p.BinInfo().Arch.MaxInstructionLength()), true) + return disassemble(p.Memory(), regs, p.Breakpoints(), p.BinInfo(), pc, pc+uint64(p.BinInfo().Arch.MaxInstructionLength()), true) } // stepInstructionOut repeatedly calls StepInstruction until the current @@ -290,12 +290,8 @@ func (dbp *Target) Step() (err error) { } if err = next(dbp, true, false); err != nil { - switch err.(type) { - case ErrThreadBlocked: // Noop - default: - dbp.ClearInternalBreakpoints() - return - } + _ = dbp.ClearInternalBreakpoints() + return err } if bp := dbp.CurrentThread().Breakpoint().Breakpoint; bp != nil && bp.Kind == StepBreakpoint && dbp.GetDirection() == Backward { @@ -489,10 +485,8 @@ func next(dbp *Target, stepInto, inlinedStepOut bool) error { ext := filepath.Ext(topframe.Current.File) csource := ext != ".go" && ext != ".s" - var thread MemoryReadWriter = curthread var regs Registers if selg != nil && selg.Thread != nil { - thread = selg.Thread regs, err = selg.Thread.Registers() if err != nil { return err @@ -518,13 +512,13 @@ func next(dbp *Target, stepInto, inlinedStepOut bool) error { return nil } - topframe.Ret, err = findCallInstrForRet(dbp, thread, topframe.Ret, retframe.Current.Fn) + topframe.Ret, err = findCallInstrForRet(dbp, dbp.Memory(), topframe.Ret, retframe.Current.Fn) if err != nil { return err } } - text, err := disassemble(thread, regs, dbp.Breakpoints(), dbp.BinInfo(), topframe.Current.Fn.Entry, topframe.Current.Fn.End, false) + text, err := disassemble(dbp.Memory(), regs, dbp.Breakpoints(), dbp.BinInfo(), topframe.Current.Fn.Entry, topframe.Current.Fn.End, false) if err != nil && stepInto { return err } @@ -812,7 +806,7 @@ func skipAutogeneratedWrappersIn(p Process, startfn *Function, startpc uint64) ( // can't exit Go return startfn, startpc } - text, err := Disassemble(p.CurrentThread(), nil, p.Breakpoints(), p.BinInfo(), fn.Entry, fn.End) + text, err := Disassemble(p.Memory(), nil, p.Breakpoints(), p.BinInfo(), fn.Entry, fn.End) if err != nil { break } @@ -876,9 +870,6 @@ func skipAutogeneratedWrappersOut(g *G, thread Thread, startTopframe, startRetfr var err error var frames []Stackframe if g == nil { - if thread.Blocked() { - return - } frames, err = ThreadStacktrace(thread, maxSkipAutogeneratedWrappers) } else { frames, err = g.Stacktrace(maxSkipAutogeneratedWrappers, 0) @@ -961,7 +952,7 @@ func stepOutReverse(p *Target, topframe, retframe Stackframe, sameGCond ast.Expr curthread = selg.Thread } - callerText, err := disassemble(curthread, nil, p.Breakpoints(), p.BinInfo(), retframe.Current.Fn.Entry, retframe.Current.Fn.End, false) + callerText, err := disassemble(p.Memory(), nil, p.Breakpoints(), p.BinInfo(), retframe.Current.Fn.Entry, retframe.Current.Fn.End, false) if err != nil { return err } @@ -969,9 +960,7 @@ func stepOutReverse(p *Target, topframe, retframe Stackframe, sameGCond ast.Expr var frames []Stackframe if selg == nil { - if !curthread.Blocked() { - frames, err = ThreadStacktrace(curthread, 3) - } + frames, err = ThreadStacktrace(curthread, 3) } else { frames, err = selg.Stacktrace(3, 0) } @@ -985,14 +974,14 @@ func stepOutReverse(p *Target, topframe, retframe Stackframe, sameGCond ast.Expr if len(frames) < 4 || frames[3].Current.Fn == nil { return &ErrNoSourceForPC{frames[2].Current.PC} } - callpc, err = findCallInstrForRet(p, curthread, frames[2].Ret, frames[3].Current.Fn) + callpc, err = findCallInstrForRet(p, p.Memory(), frames[2].Ret, frames[3].Current.Fn) if err != nil { return err } } else if ok, pc := isDeferReturnCall(frames, deferReturns); ok { callpc = pc } else { - callpc, err = findCallInstrForRet(p, curthread, topframe.Ret, retframe.Current.Fn) + callpc, err = findCallInstrForRet(p, p.Memory(), topframe.Ret, retframe.Current.Fn) if err != nil { return err } diff --git a/pkg/proc/threads.go b/pkg/proc/threads.go index 8ca17ff3..e08697c2 100644 --- a/pkg/proc/threads.go +++ b/pkg/proc/threads.go @@ -6,7 +6,6 @@ import ( // Thread represents a thread. type Thread interface { - MemoryReadWriter Location() (*Location, error) // Breakpoint will return the breakpoint that this thread is stopped at or // nil if the thread is not stopped at any breakpoint. @@ -24,9 +23,9 @@ type Thread interface { // RestoreRegisters restores saved registers RestoreRegisters(Registers) error BinInfo() *BinaryInfo + // ProcessMemory returns the process memory. + ProcessMemory() MemoryReadWriter StepInstruction() error - // Blocked returns true if the thread is blocked - Blocked() bool // SetCurrentBreakpoint updates the current breakpoint of this thread, if adjustPC is true also checks for breakpoints that were just hit (this should only be passed true after a thread resume) SetCurrentBreakpoint(adjustPC bool) error // Common returns the CommonThread structure for this thread @@ -47,14 +46,6 @@ type Location struct { Fn *Function } -// ErrThreadBlocked is returned when the thread -// is blocked in the scheduler. -type ErrThreadBlocked struct{} - -func (tbe ErrThreadBlocked) Error() string { - return "thread blocked" -} - // CommonThread contains fields used by this package, common to all // implementations of the Thread interface. type CommonThread struct { @@ -75,9 +66,6 @@ func topframe(g *G, thread Thread) (Stackframe, Stackframe, error) { var err error if g == nil { - if thread.Blocked() { - return Stackframe{}, Stackframe{}, ErrThreadBlocked{} - } frames, err = ThreadStacktrace(thread, 1) } else { frames, err = g.Stacktrace(1, StacktraceReadDefers) diff --git a/pkg/proc/variables.go b/pkg/proc/variables.go index d6ecd576..4caf6d72 100644 --- a/pkg/proc/variables.go +++ b/pkg/proc/variables.go @@ -310,16 +310,13 @@ func GoroutinesInfo(dbp *Target, start, count int) ([]*G, int, error) { threads := dbp.ThreadList() for _, th := range threads { - if th.Blocked() { - continue - } g, _ := GetG(th) if g != nil { threadg[g.ID] = g } } - allgptr, allglen, err := dbp.gcache.getRuntimeAllg(dbp.BinInfo(), dbp.CurrentThread()) + allgptr, allglen, err := dbp.gcache.getRuntimeAllg(dbp.BinInfo(), dbp.Memory()) if err != nil { return nil, -1, err } @@ -434,7 +431,7 @@ func getGVariable(thread Thread) (*Variable, error) { gaddr, hasgaddr := regs.GAddr() if !hasgaddr { var err error - gaddr, err = readUintRaw(thread, regs.TLS()+thread.BinInfo().GStructOffset(), int64(thread.BinInfo().Arch.PtrSize())) + gaddr, err = readUintRaw(thread.ProcessMemory(), regs.TLS()+thread.BinInfo().GStructOffset(), int64(thread.BinInfo().Arch.PtrSize())) if err != nil { return nil, err } @@ -568,7 +565,7 @@ func globalScope(bi *BinaryInfo, image *Image, mem MemoryReadWriter) *EvalScope } func newVariableFromThread(t Thread, name string, addr uint64, dwarfType godwarf.Type) *Variable { - return newVariable(name, addr, dwarfType, t.BinInfo(), t) + return newVariable(name, addr, dwarfType, t.BinInfo(), t.ProcessMemory()) } func (v *Variable) newVariable(name string, addr uint64, dwarfType godwarf.Type, mem MemoryReadWriter) *Variable { @@ -922,7 +919,7 @@ var errTracebackAncestorsDisabled = errors.New("tracebackancestors is disabled") // Ancestors returns the list of ancestors for g. func Ancestors(p Process, g *G, n int) ([]Ancestor, error) { - scope := globalScope(p.BinInfo(), p.BinInfo().Images[0], p.CurrentThread()) + scope := globalScope(p.BinInfo(), p.BinInfo().Images[0], p.Memory()) tbav, err := scope.EvalExpression("runtime.debug.tracebackancestors", loadSingleValue) if err == nil && tbav.Unreadable == nil && tbav.Kind == reflect.Int { tba, _ := constant.Int64Val(tbav.Value) diff --git a/pkg/terminal/command_test.go b/pkg/terminal/command_test.go index 61efd3b0..e4adcb8b 100644 --- a/pkg/terminal/command_test.go +++ b/pkg/terminal/command_test.go @@ -342,19 +342,6 @@ func TestScopePrefix(t *testing.T) { const goroutinesCurLinePrefix = "* Goroutine " test.AllowRecording(t) - tgtAgoroutineCount := 10 - - if goversion.VersionAfterOrEqual(runtime.Version(), 1, 14) { - // We try to make sure that all goroutines are stopped at a sensible place - // before reading their stacktrace, but due to the nature of the test - // program there is no guarantee that we always find them in a reasonable - // state. - // Asynchronous preemption in Go 1.14 exacerbates this problem, to avoid - // unnecessary flakiness reduce the target count to 9, allowing one - // goroutine to be in a bad state. - tgtAgoroutineCount = 9 - } - withTestTerminal("goroutinestackprog", t, func(term *FakeTerminal) { term.MustExec("b stacktraceme") term.MustExec("continue") @@ -407,7 +394,7 @@ func TestScopePrefix(t *testing.T) { } } } - if len(agoroutines)+extraAgoroutines < tgtAgoroutineCount { + if len(agoroutines)+extraAgoroutines < 10 { t.Fatalf("Output of goroutines did not have 10 goroutines stopped on main.agoroutine (%d+%d found): %q", len(agoroutines), extraAgoroutines, goroutinesOut) } } @@ -451,12 +438,8 @@ func TestScopePrefix(t *testing.T) { seen[ival] = true } - firsterr := tgtAgoroutineCount != 10 - for i := range seen { - if firsterr { - firsterr = false - } else if !seen[i] { + if !seen[i] { t.Fatalf("goroutine %d not found", i) } } @@ -536,21 +519,8 @@ func TestOnPrefix(t *testing.T) { } } - firsterr := false - if goversion.VersionAfterOrEqual(runtime.Version(), 1, 14) { - // We try to make sure that all goroutines are stopped at a sensible place - // before reading their stacktrace, but due to the nature of the test - // program there is no guarantee that we always find them in a reasonable - // state. - // Asynchronous preemption in Go 1.14 exacerbates this problem, to avoid - // unnecessary flakiness allow a single goroutine to be in a bad state. - firsterr = true - } - for i := range seen { - if firsterr { - firsterr = false - } else if !seen[i] { + if !seen[i] { t.Fatalf("Goroutine %d not seen\n", i) } } @@ -608,21 +578,8 @@ func TestOnPrefixLocals(t *testing.T) { } } - firsterr := false - if goversion.VersionAfterOrEqual(runtime.Version(), 1, 14) { - // We try to make sure that all goroutines are stopped at a sensible place - // before reading their stacktrace, but due to the nature of the test - // program there is no guarantee that we always find them in a reasonable - // state. - // Asynchronous preemption in Go 1.14 exacerbates this problem, to avoid - // unnecessary flakiness allow a single goroutine to be in a bad state. - firsterr = true - } - for i := range seen { - if firsterr { - firsterr = false - } else if !seen[i] { + if !seen[i] { t.Fatalf("Goroutine %d not seen\n", i) } } diff --git a/service/debugger/debugger.go b/service/debugger/debugger.go index b4c8398f..a94a20f5 100644 --- a/service/debugger/debugger.go +++ b/service/debugger/debugger.go @@ -381,9 +381,8 @@ func (d *Debugger) FunctionReturnLocations(fnName string) ([]uint64, error) { } var regs proc.Registers - var mem proc.MemoryReadWriter = p.CurrentThread() + mem := p.Memory() if g != nil && g.Thread != nil { - mem = g.Thread regs, _ = g.Thread.Registers() } instructions, err := proc.Disassemble(mem, regs, p.Breakpoints(), p.BinInfo(), fn.Entry, fn.End) @@ -1374,7 +1373,7 @@ func (d *Debugger) convertStacktrace(rawlocs []proc.Stackframe, cfg *proc.LoadCo } if cfg != nil && rawlocs[i].Current.Fn != nil { var err error - scope := proc.FrameToScope(d.target.BinInfo(), d.target.CurrentThread(), nil, rawlocs[i:]...) + scope := proc.FrameToScope(d.target.BinInfo(), d.target.Memory(), nil, rawlocs[i:]...) locals, err := scope.LocalVariables(*cfg) if err != nil { return nil, err @@ -1502,7 +1501,7 @@ func (d *Debugger) Disassemble(goroutineID int, addr1, addr2 uint64) ([]proc.Asm } regs, _ := curthread.Registers() - return proc.Disassemble(curthread, regs, d.target.Breakpoints(), d.target.BinInfo(), addr1, addr2) + return proc.Disassemble(d.target.Memory(), regs, d.target.Breakpoints(), d.target.BinInfo(), addr1, addr2) } func (d *Debugger) AsmInstructionText(inst *proc.AsmInstruction, flavour proc.AssemblyFlavour) string { @@ -1554,9 +1553,9 @@ func (d *Debugger) ExamineMemory(address uint64, length int) ([]byte, error) { d.targetMutex.Lock() defer d.targetMutex.Unlock() - thread := d.target.CurrentThread() + mem := d.target.Memory() data := make([]byte, length) - n, err := thread.ReadMemory(data, address) + n, err := mem.ReadMemory(data, address) if err != nil { return nil, err } diff --git a/service/test/integration1_test.go b/service/test/integration1_test.go index 89b5f413..08966c52 100644 --- a/service/test/integration1_test.go +++ b/service/test/integration1_test.go @@ -751,23 +751,9 @@ func Test1ClientServer_FullStacktrace(t *testing.T) { } } - firsterr := false - if goversion.VersionAfterOrEqual(runtime.Version(), 1, 14) { - // We try to make sure that all goroutines are stopped at a sensible place - // before reading their stacktrace, but due to the nature of the test - // program there is no guarantee that we always find them in a reasonable - // state. - // Asynchronous preemption in Go 1.14 exacerbates this problem, to avoid - // unnecessary flakiness allow a single goroutine to be in a bad state. - firsterr = true - } for i := range found { if !found[i] { - if firsterr { - firsterr = false - } else { - t.Fatalf("Goroutine %d not found", i) - } + t.Fatalf("Goroutine %d not found", i) } } diff --git a/service/test/integration2_test.go b/service/test/integration2_test.go index a3001f04..fa8b8ed0 100644 --- a/service/test/integration2_test.go +++ b/service/test/integration2_test.go @@ -928,23 +928,9 @@ func TestClientServer_FullStacktrace(t *testing.T) { } } - firsterr := false - if goversion.VersionAfterOrEqual(runtime.Version(), 1, 14) { - // We try to make sure that all goroutines are stopped at a sensible place - // before reading their stacktrace, but due to the nature of the test - // program there is no guarantee that we always find them in a reasonable - // state. - // Asynchronous preemption in Go 1.14 exacerbates this problem, to avoid - // unnecessary flakiness allow a single goroutine to be in a bad state. - firsterr = true - } for i := range found { if !found[i] { - if firsterr { - firsterr = false - } else { - t.Fatalf("Goroutine %d not found", i) - } + t.Fatalf("Goroutine %d not found", i) } } @@ -2092,14 +2078,19 @@ func TestIssue2162(t *testing.T) { t.Skip("skip it for stepping into one place where no source for pc when on pie mode or windows") } withTestClient2("issue2162", t, func(c service.Client) { - _, err := c.CreateBreakpoint(&api.Breakpoint{FunctionName: "main.main"}) + state, err := c.GetState() + assertNoError(err, t, "GetState()") + if state.CurrentThread.Function == nil { + // Can't call Step if we don't have the source code of the current function + return + } + + _, err = c.CreateBreakpoint(&api.Breakpoint{FunctionName: "main.main"}) if err != nil { t.Fatalf("Unexpected error: %v", err) } _, err = c.Step() - if err != nil { - assertNoError(err, t, "Step()") - } + assertNoError(err, t, "Step()") }) } diff --git a/service/test/variables_test.go b/service/test/variables_test.go index 77a269db..1c892754 100644 --- a/service/test/variables_test.go +++ b/service/test/variables_test.go @@ -91,7 +91,7 @@ func evalScope(p *proc.Target) (*proc.EvalScope, error) { if err != nil { return nil, err } - return proc.FrameToScope(p.BinInfo(), p.CurrentThread(), nil, frame), nil + return proc.FrameToScope(p.BinInfo(), p.Memory(), nil, frame), nil } func evalVariable(p *proc.Target, symbol string, cfg proc.LoadConfig) (*proc.Variable, error) { @@ -468,7 +468,7 @@ func TestLocalVariables(t *testing.T) { var frame proc.Stackframe frame, err = findFirstNonRuntimeFrame(p) if err == nil { - scope = proc.FrameToScope(p.BinInfo(), p.CurrentThread(), nil, frame) + scope = proc.FrameToScope(p.BinInfo(), p.Memory(), nil, frame) } } else { scope, err = proc.GoroutineScope(p.CurrentThread())