commit c8be5d9c1c2b9250289f9ed4163e73dae202fc3a
author Nigel Tao <nigeltao@golang.org> Fri Mar 28 16:34:13 2014 +1100
committer Nigel Tao <nigeltao@golang.org> Fri Mar 28 16:34:13 2014 +1100
tree 852d835e9d4236c37af7668753d72097d6d6d4ec
parent 12dc2addc3deb2a008f9027e1488df6b6472a55f

ogle/program/server: lift all breakpoints before Resume returns.

LGTM=r
R=r
https://golang.org/cl/81360047

program/server/server.go

diff --git a/program/server/server.go b/program/server/server.go
index 745e5ac..8497efa 100644
--- a/program/server/server.go
+++ b/program/server/server.go
@@ -181,7 +181,24 @@
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	err := s.ptraceCont(s.proc.Pid, 0)
+	regs := syscall.PtraceRegs{}
+	err := s.ptraceGetRegs(s.proc.Pid, &regs)
+	if err != nil {
+		return err
+	}
+	if _, ok := s.breakpoints[regs.Rip]; ok {
+		err = s.ptraceSingleStep(s.proc.Pid)
+		if err != nil {
+			return fmt.Errorf("ptraceSingleStep: %v", err)
+		}
+	}
+
+	err = s.setBreakpoints()
+	if err != nil {
+		return err
+	}
+
+	err = s.ptraceCont(s.proc.Pid, 0)
 	if err != nil {
 		return err
 	}
@@ -191,45 +208,24 @@
 		return err
 	}
 
-	regs := syscall.PtraceRegs{}
+	err = s.liftBreakpoints()
+	if err != nil {
+		return err
+	}
+
 	err = s.ptraceGetRegs(s.proc.Pid, &regs)
 	if err != nil {
 		return err
 	}
 
-	resp.Status.PC = regs.Rip
-	resp.Status.SP = regs.Rsp
-
-	// If we're stopped on a breakpoint, restore the original code,
-	// back up the PC, step through a single instruction, and reset the breakpoint.
-	// TODO: should this happen here or just before the ptraceCont call?
-	bp, ok := s.breakpoints[regs.Rip-1] // TODO: -1 because on amd64, INT 3 is 1 byte (0xcc).
-	if ok {
-		pc := uintptr(regs.Rip - 1)
-		err := s.ptracePoke(s.proc.Pid, pc, []byte{bp.origInstr})
-		if err != nil {
-			return fmt.Errorf("ptracePoke: %v", err)
-		}
-
-		regs.Rip-- // TODO: depends on length of trap.
-		err = s.ptraceSetRegs(s.proc.Pid, &regs)
-		if err != nil {
-			return fmt.Errorf("ptraceSetRegs: %v", err)
-		}
-
-		err = s.ptraceSingleStep(s.proc.Pid)
-		if err != nil {
-			return fmt.Errorf("ptraceSingleStep: %v", err)
-		}
-
-		buf := make([]byte, 1)
-		buf[0] = 0xcc // INT 3 instruction on x86.
-		err = s.ptracePoke(s.proc.Pid, pc, buf)
-		if err != nil {
-			return fmt.Errorf("ptracePoke: %v", err)
-		}
+	regs.Rip-- // TODO: depends on length of trap.
+	err = s.ptraceSetRegs(s.proc.Pid, &regs)
+	if err != nil {
+		return fmt.Errorf("ptraceSetRegs: %v", err)
 	}
 
+	resp.Status.PC = regs.Rip
+	resp.Status.SP = regs.Rsp
 	return nil
 }
 
@@ -255,19 +251,36 @@
 		if err != nil {
 			return fmt.Errorf("ptracePoke: %v", err)
 		}
-
 		s.breakpoints[pc] = breakpoint{pc: pc, origInstr: buf[0]}
-
-		buf[0] = 0xcc // INT 3 instruction on x86.
-		err = s.ptracePoke(s.proc.Pid, uintptr(pc), buf[:])
-		if err != nil {
-			return fmt.Errorf("ptracePoke: %v", err)
-		}
 	}
 
 	return nil
 }
 
+func (s *Server) setBreakpoints() error {
+	var buf [1]byte
+	buf[0] = 0xcc // INT 3 instruction on x86.
+	for pc := range s.breakpoints {
+		err := s.ptracePoke(s.proc.Pid, uintptr(pc), buf[:])
+		if err != nil {
+			return fmt.Errorf("setBreakpoints: %v", err)
+		}
+	}
+	return nil
+}
+
+func (s *Server) liftBreakpoints() error {
+	var buf [1]byte
+	for pc, breakpoint := range s.breakpoints {
+		buf[0] = breakpoint.origInstr
+		err := s.ptracePoke(s.proc.Pid, uintptr(pc), buf[:])
+		if err != nil {
+			return fmt.Errorf("liftBreakpoints: %v", err)
+		}
+	}
+	return nil
+}
+
 func (s *Server) Eval(req *proxyrpc.EvalRequest, resp *proxyrpc.EvalResponse) (err error) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
@@ -349,8 +362,7 @@
 	}
 	fp := regs.Rsp + 8 // TODO: 8 for the return address is amd64 specific.
 
-	// TODO: the -1 on the next line backs up the INT3 breakpoint. Should it be there?
-	entry, err := s.entryForPC(regs.Rip - 1)
+	entry, err := s.entryForPC(regs.Rip)
 	if err != nil {
 		return err
 	}