proc,service: remove support for locspec '<fnname>:0' (#1588)

The location specified '<fnname>:0' could be used to set a breakpoint
on the entry point of the function (as opposed to locspec '<fnname>'
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

@@ -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,
+the FunctionName field of Breakpoint.
-do not set Line to 0*.
 
 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

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))

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
+func FindFunctionLocation(p Process, funcName string, lineOffset int) (uint64, error) {
-// 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) {
 	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
 	}
-
+	filename, lineno := origfn.cu.lineInfo.PCToLine(origfn.Entry, origfn.Entry)
-	return origfn.Entry, nil
+	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 {

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 pcAfterPrologue == p.BinInfo().LookupFunc["main.changeMe"].Entry {
-		if err != nil {
-			t.Fatalf("got error while finding function location: %v", err)
-		}
-		if pcAfterPrologue == pcEntry {
 			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)
+		addrEntry := p.BinInfo().LookupFunc["main.main"].Entry
-		assertNoError(err, t, "FindFunctionLocation - entrypoint")
+		addrPrologue, err := proc.FindFunctionLocation(p, "main.main", 0)
-		addrPrologue, err := proc.FindFunctionLocation(p, "main.main", true, 0)
 		assertNoError(err, t, "FindFunctionLocation - postprologue")
 		if addrEntry == addrPrologue {
 			t.Fatalf("Prologue detection failed on testvariables2.go/main.main")

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, requestedBp.Line)
-			addr, err = proc.FindFunctionLocation(d.target, requestedBp.FunctionName, false, requestedBp.Line)
-		} else {
-			addr, err = proc.FindFunctionLocation(d.target, requestedBp.FunctionName, true, 0)
-		}
 	default:
 		addr = requestedBp.Addr
 	}

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], loc.LineOffset)
-			addr, err = proc.FindFunctionLocation(d.target, candidateFuncs[0], true, 0)
-		} else {
-			addr, err = proc.FindFunctionLocation(d.target, candidateFuncs[0], false, loc.LineOffset)
-		}
 	}
 
 	if err != nil {

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
+// location.
-// (line == 0) can have surprising consequences, it is advisable to
-// use line = -1 instead which will skip the prologue.
 //
 // - Otherwise the value specified by arg.Breakpoint.Addr will be used.
 func (s *RPCServer) CreateBreakpoint(arg CreateBreakpointIn, out *CreateBreakpointOut) error {

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

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)

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")
 		}