Simplify task exit handling by allowing Task::registers to be the sou…

…rce of truth during an unexpected exit

src/AutoRemoteSyscalls.cc

@@ -67,14 +67,7 @@ AutoRestoreMem::~AutoRestoreMem() {
     remote.task()->write_bytes_helper(addr, len, data.data());
   }
   remote.regs().set_sp(remote.regs().sp() + len);
-  if (remote.task()->is_stopped()) {
-    remote.task()->set_regs(remote.regs());
-    // If the task is not stopped it must have been kicked out of a
-    // stop via SIGKILL or equivalent. In that case we should not be
-    // trying to do anything more with it until restore_state_to(),
-    // which will clean things up, so it doesn't matter if we
-    // set the task's registers here.
-  }
+  remote.task()->set_regs(remote.regs());
 }
 
 static bool is_SIGTRAP_default_and_unblocked(Task* t) {
@@ -270,7 +263,7 @@ void AutoRemoteSyscalls::restore_state_to(Task* t) {
     // Don't restore status; callers need to see the task is exiting.
     // And the other stuff we don't below won't work.
     // But do restore registers so it looks like the exit happened in a clean state.
-    t->override_regs_during_exit(regs);
+    t->set_regs(regs);
     return;
   }
 

src/Task.cc

@@ -90,6 +90,7 @@ Task::Task(Session& session, pid_t _tid, pid_t _rec_tid, uint32_t serial,
       last_resume_orig_cx(0),
       did_set_breakpoint_after_cpuid(false),
       is_stopped_(false),
+      in_unexpected_exit(false),
       seccomp_bpf_enabled(false),
       registers_dirty(false),
       orig_syscallno_dirty(false),
@@ -976,7 +977,7 @@ bool Task::exit_syscall_and_prepare_restart() {
     // make it look like we really entered the syscall. Then
     // handle_ptrace_exit_event will record something appropriate.
     r.emulate_syscall_entry();
-    override_regs_during_exit(r);
+    set_regs(r);
     return false;
   }
   LOG(debug) << "exit_syscall_and_prepare_restart done";
@@ -1100,7 +1101,10 @@ string Task::read_c_str(remote_ptr<char> child_addr, bool *ok) {
 }
 
 const Registers& Task::regs() const {
-  ASSERT(this, is_stopped_ || was_reaped_);
+  // If we're in an unexpected exit then the tracee may
+  // not be stopped but we know its registers won't change again,
+  // so it's safe to ask for them here.
+  ASSERT(this, is_stopped_ || was_reaped_ || in_unexpected_exit);
   return registers;
 }
 
@@ -1566,7 +1570,8 @@ bool Task::resume_execution(ResumeRequest how, WaitRequest wait_how,
 }
 
 void Task::set_regs(const Registers& regs) {
-  ASSERT(this, is_stopped_);
+  // Only allow registers to be set while our copy is the source of truth.
+  ASSERT(this, is_stopped_ || in_unexpected_exit);
   if (registers.original_syscallno() != regs.original_syscallno()) {
     orig_syscallno_dirty = true;
   }
@@ -1577,12 +1582,6 @@ void Task::set_regs(const Registers& regs) {
   }
 }
 
-void Task::override_regs_during_exit(const Registers& regs) {
-  orig_syscallno_dirty = true;
-  registers_dirty = true;
-  registers = regs;
-}
-
 void Task::flush_regs() {
   if (registers_dirty) {
     LOG(debug) << "Flushing registers for tid " << tid << " " << registers;
@@ -2171,6 +2170,7 @@ bool Task::did_waitpid(WaitStatus status) {
         LOG(debug) << "Unexpected process death getting siginfo for " << tid;
         // Let's pretend this stop never happened.
         set_stopped(false);
+        in_unexpected_exit = true;
         return false;
       }
     }
@@ -2231,6 +2231,7 @@ bool Task::did_waitpid(WaitStatus status) {
         // but in that case we're going to ignore this signal-stop
         // so it doesn't matter.
         set_stopped(false);
+        in_unexpected_exit = true;
         return false;
       }
     }

src/Task.h

@@ -598,13 +598,6 @@ class Task {
   /** Set the tracee's registers to |regs|. Lazy. */
   void set_regs(const Registers& regs);
 
-  /** Set the tracee's registers to |regs|. The task
-   * is known to be exiting (running towards, or actually in, a
-   * not-yet-reported PTRACE_EVENT_EXIT or reap) so its real registers
-   * won't change underneath us, but we need to override those
-   * registers with our values for recording purposes. */
-  void override_regs_during_exit(const Registers& regs);
-
   /** Ensure registers are flushed back to the underlying task. */
   void flush_regs();
 
@@ -1275,7 +1268,12 @@ class Task {
   // Count of all ticks seen by this task since tracees became
   // consistent and the task last wait()ed.
   Ticks ticks;
-  // When |is_stopped_|, these are our child registers.
+  // Copy of the child registers.
+  // When is_stopped_ or in_unexpected_exit, these are the source of
+  // truth. Otherwise the child is running and the registers could be
+  // changed by the kernel or user-space execution, and the values here
+  // are meaningless.
+  // See also registers_dirty.
   Registers registers;
   // Where we last resumed execution
   remote_code_ptr address_of_last_execution_resume;
@@ -1298,6 +1296,9 @@ class Task {
   // without our knowledge, due to a SIGKILL or equivalent such as
   // zap_pid_ns_processes.
   bool is_stopped_;
+  // True when we've been kicked out of a ptrace-stop via SIGKILL or
+  // equivalent.
+  bool in_unexpected_exit;
   /* True when the seccomp filter has been enabled via prctl(). This happens
    * in the first system call issued by the initial tracee (after it returns
    * from kill(SIGSTOP) to synchronize with the tracer). */

src/record_syscall.cc

@@ -6970,11 +6970,7 @@ void rec_process_syscall(RecordTask* t) {
 
   aarch64_kernel_bug_workaround(t, syscall_state);
 
-  if (t->is_stopped()) {
-    t->on_syscall_exit(sys_ev.number, sys_ev.arch(), t->regs());
-    // If t was killed via SIGKILL or equivalent, these post-syscall
-    // state updates don't matter.
-  }
+  t->on_syscall_exit(sys_ev.number, sys_ev.arch(), t->regs());
   t->syscall_state = nullptr;
 
   MonitoredSharedMemory::check_all(t);