proc,terminal: Ensure correct exit status (#2543)

Ensure that any command executed after the process we are trying to
debug prints a correct and consistent exit status.

Previously the exit code was being lost after the first time we printed
that a process has exited. Additionally, certain commands would print
the PID of the process and other would not. This change makes everything
more correct and consistent.

pkg/proc/interface.go

@@ -26,6 +26,10 @@ type Process interface {
 // the `proc` package.
 // This is temporary and in support of an ongoing refactor.
 type ProcessInternal interface {
+	// Valid returns true if this Process can be used. When it returns false it
+	// also returns an error describing why the Process is invalid (either
+	// ErrProcessExited or ErrProcessDetached).
+	Valid() (bool, error)
 	// 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
@@ -87,10 +91,6 @@ type Info interface {
 	// ResumeNotify specifies a channel that will be closed the next time
 	// ContinueOnce finishes resuming the target.
 	ResumeNotify(chan<- struct{})
-	// Valid returns true if this Process can be used. When it returns false it
-	// also returns an error describing why the Process is invalid (either
-	// ErrProcessExited or ErrProcessDetached).
-	Valid() (bool, error)
 	BinInfo() *BinaryInfo
 	EntryPoint() (uint64, error)
 

pkg/proc/target.go

@@ -62,6 +62,10 @@ type Target struct {
 	// This must be cleared whenever the target is resumed.
 	gcache goroutineCache
 	iscgo  *bool
+
+	// exitStatus is the exit status of the process we are debugging.
+	// Saved here to relay to any future commands.
+	exitStatus int
 }
 
 // ErrProcessExited indicates that the process has exited and contains both
@@ -203,6 +207,20 @@ func (t *Target) IsCgo() bool {
 	return false
 }
 
+// Valid returns true if this Process can be used. When it returns false it
+// also returns an error describing why the Process is invalid (either
+// ErrProcessExited or ErrProcessDetached).
+func (t *Target) Valid() (bool, error) {
+	ok, err := t.proc.Valid()
+	if !ok && err != nil {
+		if pe, ok := err.(ErrProcessExited); ok {
+			pe.Status = t.exitStatus
+			err = pe
+		}
+	}
+	return ok, err
+}
+
 // SupportsFunctionCalls returns whether or not the backend supports
 // calling functions during a debug session.
 // Currently only non-recorded processes running on AMD64 support

pkg/proc/target_exec.go

@@ -83,6 +83,9 @@ func (dbp *Target) Continue() error {
 					dbp.selectedGoroutine, _ = GetG(curth)
 				}
 			}
+			if pe, ok := err.(ErrProcessExited); ok {
+				dbp.exitStatus = pe.Status
+			}
 			return err
 		}
 		if dbp.StopReason == StopLaunched {

pkg/terminal/command.go

@@ -1308,7 +1308,7 @@ func scopePrefixSwitch(t *Term, ctx callContext) error {
 
 func exitedToError(state *api.DebuggerState, err error) (*api.DebuggerState, error) {
 	if err == nil && state.Exited {
-		return nil, fmt.Errorf("Process has exited with status %d", state.ExitStatus)
+		return nil, fmt.Errorf("Process %d has exited with status %d", state.Pid, state.ExitStatus)
 	}
 	return state, err
 }

service/api/types.go

@@ -17,6 +17,8 @@ var ErrNotExecutable = errors.New("not an executable file")
 
 // DebuggerState represents the current context of the debugger.
 type DebuggerState struct {
+	// PID of the process we are debugging.
+	Pid int
 	// Running is true if the process is running and no other information can be collected.
 	Running bool
 	// Recording is true if the process is currently being recorded and no other

service/debugger/debugger.go

@@ -1189,11 +1189,12 @@ func (d *Debugger) Command(command *api.DebuggerCommand, resumeNotify chan struc
 	}
 
 	if err != nil {
-		if exitedErr, exited := err.(proc.ErrProcessExited); command.Name != api.SwitchGoroutine && command.Name != api.SwitchThread && exited {
+		if pe, ok := err.(proc.ErrProcessExited); ok && command.Name != api.SwitchGoroutine && command.Name != api.SwitchThread {
 			state := &api.DebuggerState{}
+			state.Pid = d.target.Pid()
 			state.Exited = true
-			state.ExitStatus = exitedErr.Status
+			state.ExitStatus = pe.Status
-			state.Err = errors.New(exitedErr.Error())
+			state.Err = pe
 			return state, nil
 		}
 		return nil, err

service/test/integration2_test.go

@@ -1773,6 +1773,23 @@ func TestClientServerConsistentExit(t *testing.T) {
 		if state.ExitStatus != 2 {
 			t.Fatalf("Process exit status is not 2, got: %v", state.ExitStatus)
 		}
+
+		// Ensure future commands also return the correct exit status.
+		// Previously there was a bug where the command which prompted the
+		// process to exit (continue, next, etc...) would return the corrent
+		// exit status but subsequent commands would return an incorrect exit
+		// status of 0. To test this we simply repeat the 'next' command and
+		// ensure we get the correct response again.
+		state, err = c.Next()
+		if err != nil {
+			t.Fatalf("Unexpected error: %v", err)
+		}
+		if !state.Exited {
+			t.Fatal("Second process state is not exited")
+		}
+		if state.ExitStatus != 2 {
+			t.Fatalf("Second process exit status is not 2, got: %v", state.ExitStatus)
+		}
 	})
 }