From 7afda8dbe052bee80841c553581f1a85d71345ff Mon Sep 17 00:00:00 2001 From: Alessandro Arzilli Date: Tue, 25 Jun 2019 22:50:05 +0200 Subject: [PATCH] proc,service: remove support for locspec ':0' (#1588) The location specified ':0' could be used to set a breakpoint on the entry point of the function (as opposed to locspec '' which sets it after the prologue). Setting a breakpoint on an entry point is almost never useful, the way this feature was implemented could cause it to be used accidentally and there are other ways to accomplish the same task (by setting a breakpoint on the PC address directly). --- Documentation/api/ClientHowto.md | 3 +- pkg/proc/gdbserial/rr_test.go | 2 +- pkg/proc/proc.go | 23 +++++--------- pkg/proc/proc_test.go | 33 +++++++++----------- service/debugger/debugger.go | 6 +--- service/debugger/locations.go | 8 ++--- service/rpc2/server.go | 4 +-- service/test/integration1_test.go | 50 ------------------------------- service/test/integration2_test.go | 14 ++++++--- service/test/variables_test.go | 4 +-- 10 files changed, 40 insertions(+), 107 deletions(-) diff --git a/Documentation/api/ClientHowto.md b/Documentation/api/ClientHowto.md index d29ab44a..00556837 100644 --- a/Documentation/api/ClientHowto.md +++ b/Documentation/api/ClientHowto.md @@ -177,8 +177,7 @@ as valid. If you want to let your users specify a breakpoint on a function selected from a list of all functions you should specify the name of the function in -the FunctionName field of Breakpoint and set Line to -1. *Do not omit Line, -do not set Line to 0*. +the FunctionName field of Breakpoint. If you want to support the [same language as dlv's break and trace commands](//github.com/go-delve/Delve/tree/master/Documentation/cli/locspec.md) you should call RPCServer.FindLocation and diff --git a/pkg/proc/gdbserial/rr_test.go b/pkg/proc/gdbserial/rr_test.go index 1b22e143..1137882a 100644 --- a/pkg/proc/gdbserial/rr_test.go +++ b/pkg/proc/gdbserial/rr_test.go @@ -55,7 +55,7 @@ func assertNoError(err error, t testing.TB, s string) { } func setFunctionBreakpoint(p proc.Process, t *testing.T, fname string) *proc.Breakpoint { - addr, err := proc.FindFunctionLocation(p, fname, true, 0) + addr, err := proc.FindFunctionLocation(p, fname, 0) assertNoError(err, t, fmt.Sprintf("FindFunctionLocation(%s)", fname)) bp, err := p.SetBreakpoint(addr, proc.UserBreakpoint, nil) assertNoError(err, t, fmt.Sprintf("SetBreakpoint(%#x) function %s", addr, fname)) diff --git a/pkg/proc/proc.go b/pkg/proc/proc.go index 1685b7f6..bc2bcc8f 100644 --- a/pkg/proc/proc.go +++ b/pkg/proc/proc.go @@ -100,27 +100,20 @@ func (err *ErrFunctionNotFound) Error() string { } // FindFunctionLocation finds address of a function's line -// If firstLine == true is passed FindFunctionLocation will attempt to find the first line of the function // If lineOffset is passed FindFunctionLocation will return the address of that line -// Pass lineOffset == 0 and firstLine == false if you want the address for the function's entry point -// Note that setting breakpoints at that address will cause surprising behavior: -// https://github.com/go-delve/delve/issues/170 -func FindFunctionLocation(p Process, funcName string, firstLine bool, lineOffset int) (uint64, error) { +func FindFunctionLocation(p Process, funcName string, lineOffset int) (uint64, error) { bi := p.BinInfo() origfn := bi.LookupFunc[funcName] if origfn == nil { return 0, &ErrFunctionNotFound{funcName} } - if firstLine { + if lineOffset <= 0 { return FirstPCAfterPrologue(p, origfn, false) - } else if lineOffset > 0 { - filename, lineno := origfn.cu.lineInfo.PCToLine(origfn.Entry, origfn.Entry) - breakAddr, _, err := bi.LineToPC(filename, lineno+lineOffset) - return breakAddr, err } - - return origfn.Entry, nil + filename, lineno := origfn.cu.lineInfo.PCToLine(origfn.Entry, origfn.Entry) + breakAddr, _, err := bi.LineToPC(filename, lineno+lineOffset) + return breakAddr, err } // FunctionReturnLocations will return a list of addresses corresponding @@ -740,9 +733,9 @@ func FrameToScope(bi *BinaryInfo, thread MemoryReadWriter, g *G, frames ...Stack // createUnrecoveredPanicBreakpoint creates the unrecoverable-panic breakpoint. // This function is meant to be called by implementations of the Process interface. func createUnrecoveredPanicBreakpoint(p Process, writeBreakpoint WriteBreakpointFn) { - panicpc, err := FindFunctionLocation(p, "runtime.startpanic", true, 0) + panicpc, err := FindFunctionLocation(p, "runtime.startpanic", 0) if _, isFnNotFound := err.(*ErrFunctionNotFound); isFnNotFound { - panicpc, err = FindFunctionLocation(p, "runtime.fatalpanic", true, 0) + panicpc, err = FindFunctionLocation(p, "runtime.fatalpanic", 0) } if err == nil { bp, err := p.Breakpoints().SetWithID(unrecoveredPanicID, panicpc, writeBreakpoint) @@ -754,7 +747,7 @@ func createUnrecoveredPanicBreakpoint(p Process, writeBreakpoint WriteBreakpoint } func createFatalThrowBreakpoint(p Process, writeBreakpoint WriteBreakpointFn) { - fatalpc, err := FindFunctionLocation(p, "runtime.fatalthrow", true, 0) + fatalpc, err := FindFunctionLocation(p, "runtime.fatalthrow", 0) if err == nil { bp, err := p.Breakpoints().SetWithID(fatalThrowID, fatalpc, writeBreakpoint) if err == nil { diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go index 9f41d2bb..e2b2392b 100644 --- a/pkg/proc/proc_test.go +++ b/pkg/proc/proc_test.go @@ -177,7 +177,7 @@ func TestExitAfterContinue(t *testing.T) { } func setFunctionBreakpoint(p proc.Process, fname string) (*proc.Breakpoint, error) { - addr, err := proc.FindFunctionLocation(p, fname, true, 0) + addr, err := proc.FindFunctionLocation(p, fname, 0) if err != nil { return nil, err } @@ -246,7 +246,7 @@ func TestHalt(t *testing.T) { func TestStep(t *testing.T) { protest.AllowRecording(t) withTestProcess("testprog", t, func(p proc.Process, fixture protest.Fixture) { - helloworldaddr, err := proc.FindFunctionLocation(p, "main.helloworld", false, 0) + helloworldaddr, err := proc.FindFunctionLocation(p, "main.helloworld", 0) assertNoError(err, t, "FindFunctionLocation") _, err = p.SetBreakpoint(helloworldaddr, proc.UserBreakpoint, nil) @@ -269,7 +269,7 @@ func TestStep(t *testing.T) { func TestBreakpoint(t *testing.T) { protest.AllowRecording(t) withTestProcess("testprog", t, func(p proc.Process, fixture protest.Fixture) { - helloworldaddr, err := proc.FindFunctionLocation(p, "main.helloworld", false, 0) + helloworldaddr, err := proc.FindFunctionLocation(p, "main.helloworld", 0) assertNoError(err, t, "FindFunctionLocation") bp, err := p.SetBreakpoint(helloworldaddr, proc.UserBreakpoint, nil) @@ -294,7 +294,7 @@ func TestBreakpoint(t *testing.T) { func TestBreakpointInSeparateGoRoutine(t *testing.T) { protest.AllowRecording(t) withTestProcess("testthreads", t, func(p proc.Process, fixture protest.Fixture) { - fnentry, err := proc.FindFunctionLocation(p, "main.anotherthread", false, 0) + fnentry, err := proc.FindFunctionLocation(p, "main.anotherthread", 0) assertNoError(err, t, "FindFunctionLocation") _, err = p.SetBreakpoint(fnentry, proc.UserBreakpoint, nil) @@ -324,7 +324,7 @@ func TestBreakpointWithNonExistantFunction(t *testing.T) { func TestClearBreakpointBreakpoint(t *testing.T) { withTestProcess("testprog", t, func(p proc.Process, fixture protest.Fixture) { - fnentry, err := proc.FindFunctionLocation(p, "main.sleepytime", false, 0) + fnentry, err := proc.FindFunctionLocation(p, "main.sleepytime", 0) assertNoError(err, t, "FindFunctionLocation") bp, err := p.SetBreakpoint(fnentry, proc.UserBreakpoint, nil) assertNoError(err, t, "SetBreakpoint()") @@ -732,7 +732,7 @@ func TestFindReturnAddressTopOfStackFn(t *testing.T) { protest.AllowRecording(t) withTestProcess("testreturnaddress", t, func(p proc.Process, fixture protest.Fixture) { fnName := "runtime.rt0_go" - fnentry, err := proc.FindFunctionLocation(p, fnName, false, 0) + fnentry, err := proc.FindFunctionLocation(p, fnName, 0) assertNoError(err, t, "FindFunctionLocation") if _, err := p.SetBreakpoint(fnentry, proc.UserBreakpoint, nil); err != nil { t.Fatal(err) @@ -754,7 +754,7 @@ func TestSwitchThread(t *testing.T) { if err == nil { t.Fatal("Expected error for invalid thread id") } - pc, err := proc.FindFunctionLocation(p, "main.main", true, 0) + pc, err := proc.FindFunctionLocation(p, "main.main", 0) if err != nil { t.Fatal(err) } @@ -800,7 +800,7 @@ func TestCGONext(t *testing.T) { protest.AllowRecording(t) withTestProcess("cgotest", t, func(p proc.Process, fixture protest.Fixture) { - pc, err := proc.FindFunctionLocation(p, "main.main", true, 0) + pc, err := proc.FindFunctionLocation(p, "main.main", 0) if err != nil { t.Fatal(err) } @@ -1790,13 +1790,9 @@ func TestIssue332_Part2(t *testing.T) { regs, err := p.CurrentThread().Registers(false) assertNoError(err, t, "Registers()") pc := regs.PC() - pcAfterPrologue, err := proc.FindFunctionLocation(p, "main.changeMe", true, -1) + pcAfterPrologue, err := proc.FindFunctionLocation(p, "main.changeMe", 0) assertNoError(err, t, "FindFunctionLocation()") - pcEntry, err := proc.FindFunctionLocation(p, "main.changeMe", false, 0) - if err != nil { - t.Fatalf("got error while finding function location: %v", err) - } - if pcAfterPrologue == pcEntry { + if pcAfterPrologue == p.BinInfo().LookupFunc["main.changeMe"].Entry { t.Fatalf("main.changeMe and main.changeMe:0 are the same (%x)", pcAfterPrologue) } if pc != pcAfterPrologue { @@ -1815,7 +1811,7 @@ func TestIssue332_Part2(t *testing.T) { func TestIssue396(t *testing.T) { withTestProcess("callme", t, func(p proc.Process, fixture protest.Fixture) { - _, err := proc.FindFunctionLocation(p, "main.init", true, -1) + _, err := proc.FindFunctionLocation(p, "main.init", 0) assertNoError(err, t, "FindFunctionLocation()") }) } @@ -2128,7 +2124,7 @@ func TestIssue573(t *testing.T) { // of the function and the internal breakpoint set by StepInto may be missed. protest.AllowRecording(t) withTestProcess("issue573", t, func(p proc.Process, fixture protest.Fixture) { - fentry, _ := proc.FindFunctionLocation(p, "main.foo", false, 0) + fentry, _ := proc.FindFunctionLocation(p, "main.foo", 0) _, err := p.SetBreakpoint(fentry, proc.UserBreakpoint, nil) assertNoError(err, t, "SetBreakpoint()") assertNoError(proc.Continue(p), t, "Continue()") @@ -2140,9 +2136,8 @@ func TestIssue573(t *testing.T) { func TestTestvariables2Prologue(t *testing.T) { withTestProcess("testvariables2", t, func(p proc.Process, fixture protest.Fixture) { - addrEntry, err := proc.FindFunctionLocation(p, "main.main", false, 0) - assertNoError(err, t, "FindFunctionLocation - entrypoint") - addrPrologue, err := proc.FindFunctionLocation(p, "main.main", true, 0) + addrEntry := p.BinInfo().LookupFunc["main.main"].Entry + addrPrologue, err := proc.FindFunctionLocation(p, "main.main", 0) assertNoError(err, t, "FindFunctionLocation - postprologue") if addrEntry == addrPrologue { t.Fatalf("Prologue detection failed on testvariables2.go/main.main") diff --git a/service/debugger/debugger.go b/service/debugger/debugger.go index 0e44ae5f..611cd888 100644 --- a/service/debugger/debugger.go +++ b/service/debugger/debugger.go @@ -395,11 +395,7 @@ func (d *Debugger) CreateBreakpoint(requestedBp *api.Breakpoint) (*api.Breakpoin } addr, err = proc.FindFileLocation(d.target, fileName, requestedBp.Line) case len(requestedBp.FunctionName) > 0: - if requestedBp.Line >= 0 { - addr, err = proc.FindFunctionLocation(d.target, requestedBp.FunctionName, false, requestedBp.Line) - } else { - addr, err = proc.FindFunctionLocation(d.target, requestedBp.FunctionName, true, 0) - } + addr, err = proc.FindFunctionLocation(d.target, requestedBp.FunctionName, requestedBp.Line) default: addr = requestedBp.Addr } diff --git a/service/debugger/locations.go b/service/debugger/locations.go index 29419f3c..d1329139 100644 --- a/service/debugger/locations.go +++ b/service/debugger/locations.go @@ -250,7 +250,7 @@ func (loc *RegexLocationSpec) Find(d *Debugger, scope *proc.EvalScope, locStr st } r := make([]api.Location, 0, len(matches)) for i := range matches { - addr, err := proc.FindFunctionLocation(d.target, matches[i], true, 0) + addr, err := proc.FindFunctionLocation(d.target, matches[i], 0) if err == nil { r = append(r, api.Location{PC: addr}) } @@ -385,11 +385,7 @@ func (loc *NormalLocationSpec) Find(d *Debugger, scope *proc.EvalScope, locStr s } addr, err = proc.FindFileLocation(d.target, candidateFiles[0], loc.LineOffset) } else { // len(candidateFUncs) == 1 - if loc.LineOffset < 0 { - addr, err = proc.FindFunctionLocation(d.target, candidateFuncs[0], true, 0) - } else { - addr, err = proc.FindFunctionLocation(d.target, candidateFuncs[0], false, loc.LineOffset) - } + addr, err = proc.FindFunctionLocation(d.target, candidateFuncs[0], loc.LineOffset) } if err != nil { diff --git a/service/rpc2/server.go b/service/rpc2/server.go index 542cbeec..3e829a2a 100644 --- a/service/rpc2/server.go +++ b/service/rpc2/server.go @@ -222,9 +222,7 @@ type CreateBreakpointOut struct { // // - If arg.Breakpoint.FunctionName is not an empty string // the breakpoint will be created on the specified function:line -// location. Note that setting a breakpoint on a function's entry point -// (line == 0) can have surprising consequences, it is advisable to -// use line = -1 instead which will skip the prologue. +// location. // // - Otherwise the value specified by arg.Breakpoint.Addr will be used. func (s *RPCServer) CreateBreakpoint(arg CreateBreakpointIn, out *CreateBreakpointOut) error { diff --git a/service/test/integration1_test.go b/service/test/integration1_test.go index 35c65836..bc7f880e 100644 --- a/service/test/integration1_test.go +++ b/service/test/integration1_test.go @@ -1001,56 +1001,6 @@ func Test1ClientServer_CondBreakpoint(t *testing.T) { }) } -func Test1SkipPrologue(t *testing.T) { - withTestClient1("locationsprog2", t, func(c *rpc1.RPCClient) { - <-c.Continue() - - afunction := findLocationHelper(t, c, "main.afunction", false, 1, 0)[0] - findLocationHelper(t, c, "*fn1", false, 1, afunction) - findLocationHelper(t, c, "locationsprog2.go:8", false, 1, afunction) - - afunction0 := findLocationHelper(t, c, "main.afunction:0", false, 1, 0)[0] - - if afunction == afunction0 { - t.Fatal("Skip prologue failed") - } - }) -} - -func Test1SkipPrologue2(t *testing.T) { - withTestClient1("callme", t, func(c *rpc1.RPCClient) { - callme := findLocationHelper(t, c, "main.callme", false, 1, 0)[0] - callmeZ := findLocationHelper(t, c, "main.callme:0", false, 1, 0)[0] - findLocationHelper(t, c, "callme.go:5", false, 1, callme) - if callme == callmeZ { - t.Fatal("Skip prologue failed") - } - - callme2 := findLocationHelper(t, c, "main.callme2", false, 1, 0)[0] - callme2Z := findLocationHelper(t, c, "main.callme2:0", false, 1, 0)[0] - findLocationHelper(t, c, "callme.go:12", false, 1, callme2) - if callme2 == callme2Z { - t.Fatal("Skip prologue failed") - } - - callme3 := findLocationHelper(t, c, "main.callme3", false, 1, 0)[0] - callme3Z := findLocationHelper(t, c, "main.callme3:0", false, 1, 0)[0] - ver, _ := goversion.Parse(runtime.Version()) - if ver.Major < 0 || ver.AfterOrEqual(goversion.GoVer18Beta) { - findLocationHelper(t, c, "callme.go:19", false, 1, callme3) - } else { - // callme3 does not have local variables therefore the first line of the - // function is immediately after the prologue - // This is only true before 1.8 where frame pointer chaining introduced a - // bit of prologue even for functions without local variables - findLocationHelper(t, c, "callme.go:19", false, 1, callme3Z) - } - if callme3 == callme3Z { - t.Fatal("Skip prologue failed") - } - }) -} - func Test1Issue419(t *testing.T) { // Calling service/rpc.(*Client).Halt could cause a crash because both Halt and Continue simultaneously // try to read 'runtime.g' and debug/dwarf.Data.Type is not thread safe diff --git a/service/test/integration2_test.go b/service/test/integration2_test.go index c0f152ee..5300a47e 100644 --- a/service/test/integration2_test.go +++ b/service/test/integration2_test.go @@ -1085,6 +1085,12 @@ func TestClientServer_CondBreakpoint(t *testing.T) { }) } +func clientEvalVariable(t *testing.T, c service.Client, expr string) *api.Variable { + v, err := c.EvalVariable(api.EvalScope{GoroutineID: -1}, expr, normalLoadConfig) + assertNoError(err, t, fmt.Sprintf("EvalVariable(%s)", expr)) + return v +} + func TestSkipPrologue(t *testing.T) { withTestClient2("locationsprog2", t, func(c service.Client) { <-c.Continue() @@ -1093,7 +1099,7 @@ func TestSkipPrologue(t *testing.T) { findLocationHelper(t, c, "*fn1", false, 1, afunction) findLocationHelper(t, c, "locationsprog2.go:8", false, 1, afunction) - afunction0 := findLocationHelper(t, c, "main.afunction:0", false, 1, 0)[0] + afunction0 := uint64(clientEvalVariable(t, c, "main.afunction").Addr) if afunction == afunction0 { t.Fatal("Skip prologue failed") @@ -1104,21 +1110,21 @@ func TestSkipPrologue(t *testing.T) { func TestSkipPrologue2(t *testing.T) { withTestClient2("callme", t, func(c service.Client) { callme := findLocationHelper(t, c, "main.callme", false, 1, 0)[0] - callmeZ := findLocationHelper(t, c, "main.callme:0", false, 1, 0)[0] + callmeZ := uint64(clientEvalVariable(t, c, "main.callme").Addr) findLocationHelper(t, c, "callme.go:5", false, 1, callme) if callme == callmeZ { t.Fatal("Skip prologue failed") } callme2 := findLocationHelper(t, c, "main.callme2", false, 1, 0)[0] - callme2Z := findLocationHelper(t, c, "main.callme2:0", false, 1, 0)[0] + callme2Z := uint64(clientEvalVariable(t, c, "main.callme2").Addr) findLocationHelper(t, c, "callme.go:12", false, 1, callme2) if callme2 == callme2Z { t.Fatal("Skip prologue failed") } callme3 := findLocationHelper(t, c, "main.callme3", false, 1, 0)[0] - callme3Z := findLocationHelper(t, c, "main.callme3:0", false, 1, 0)[0] + callme3Z := uint64(clientEvalVariable(t, c, "main.callme3").Addr) ver, _ := goversion.Parse(runtime.Version()) if ver.Major < 0 || ver.AfterOrEqual(goversion.GoVer18Beta) { findLocationHelper(t, c, "callme.go:19", false, 1, callme3) diff --git a/service/test/variables_test.go b/service/test/variables_test.go index 7c38c6e8..b2baad17 100644 --- a/service/test/variables_test.go +++ b/service/test/variables_test.go @@ -1063,7 +1063,7 @@ func TestConstants(t *testing.T) { } func setFunctionBreakpoint(p proc.Process, fname string) (*proc.Breakpoint, error) { - addr, err := proc.FindFunctionLocation(p, fname, true, 0) + addr, err := proc.FindFunctionLocation(p, fname, 0) if err != nil { return nil, err } @@ -1181,7 +1181,7 @@ func TestCallFunction(t *testing.T) { } withTestProcess("fncall", t, func(p proc.Process, fixture protest.Fixture) { - _, err := proc.FindFunctionLocation(p, "runtime.debugCallV1", true, 0) + _, err := proc.FindFunctionLocation(p, "runtime.debugCallV1", 0) if err != nil { t.Skip("function calls not supported on this version of go") }