Move extra_regs to ReplayTask, use extra_regs_fallible in more places.

Fixes #3725
rr-debugger · Apr 18, 2024 · b9117dd · rocallahan · Apr 18, 2024 · khuey
1 parent b8ed44a
commit b9117dd
Show file tree

Hide file tree

Showing 14 changed files with 133 additions and 93 deletions.
diff --git a/src/BreakpointCondition.h b/src/BreakpointCondition.h
@@ -5,12 +5,12 @@
 
 namespace rr {
 
-class Task;
+class ReplayTask;
 
 class BreakpointCondition {
 public:
   virtual ~BreakpointCondition() {}
-  virtual bool evaluate(Task* t) const = 0;
+  virtual bool evaluate(ReplayTask* t) const = 0;
 };
 
 } // namespace rr

diff --git a/src/GdbCommand.cc b/src/GdbCommand.cc
@@ -26,7 +26,7 @@ static SimpleGdbCommand elapsed_time(
     });
 
 static SimpleGdbCommand when(
-    "when", "Print the numer of the last completely replayed rr event.",
+    "when", "Print the number of the last completely replayed rr event.",
     [](GdbServer&, Task* t, const vector<string>&) {
       if (!t->session().is_replaying()) {
         return GdbCommandHandler::cmd_end_diversion();

diff --git a/src/GdbServer.cc b/src/GdbServer.cc
@@ -108,7 +108,7 @@ static bool set_reg(Task* target, const GdbRegisterValue& reg) {
     return true;
   }
 
-  ExtraRegisters extra_regs = target->extra_regs();
+  ExtraRegisters extra_regs = *target->extra_regs_fallible();
   if (extra_regs.write_register(reg.name, reg.value, reg.size)) {
     target->set_extra_regs(extra_regs);
     return true;
@@ -166,10 +166,10 @@ static WatchType watchpoint_type(GdbRequestType req) {
 }
 
 static void maybe_singlestep_for_event(Task* t, GdbRequest* req) {
-  if (!t->session().is_replaying()) {
+  ReplayTask* rt = ReplayTask::cast_or_null(t);
+  if (!rt) {
     return;
   }
-  auto rt = static_cast<ReplayTask*>(t);
   if (trace_instructions_up_to_event(
           rt->session().current_trace_frame().time())) {
     fputs("Stepping: ", stderr);
@@ -218,7 +218,7 @@ class GdbBreakpointCondition : public BreakpointCondition {
       expressions.push_back(GdbServerExpression(b.data(), b.size()));
     }
   }
-  virtual bool evaluate(Task* t) const override {
+  virtual bool evaluate(ReplayTask* t) const override {
     for (auto& e : expressions) {
       GdbServerExpression::Value v;
       // Break if evaluation fails or the result is nonzero
@@ -638,12 +638,12 @@ void GdbServer::dispatch_debugger_request(Session& session,
     }
     case DREQ_GET_REG: {
       GdbRegisterValue reg =
-          get_reg(target->regs(), target->extra_regs(), req.reg().name);
+          get_reg(target->regs(), *target->extra_regs_fallible(), req.reg().name);
       dbg->reply_get_reg(reg);
       return;
     }
     case DREQ_GET_REGS: {
-      dispatch_regs_request(target->regs(), target->extra_regs());
+      dispatch_regs_request(target->regs(), *target->extra_regs_fallible());
       return;
     }
     case DREQ_SET_REG: {
@@ -768,7 +768,7 @@ void GdbServer::dispatch_debugger_request(Session& session,
       }
       SavedRegisters& regs = saved_register_states[state_index];
       regs.regs = target->regs();
-      regs.extra_regs = target->extra_regs();
+      regs.extra_regs = *target->extra_regs_fallible();
       dbg->reply_save_register_state(true, state_index);
       return;
     }

diff --git a/src/GdbServerExpression.cc b/src/GdbServerExpression.cc
@@ -139,7 +139,7 @@ struct ExpressionState {
     push(stack[stack.size() - 1 - offset].i);
   }
 
-  void step(Task* t) {
+  void step(ReplayTask* t) {
     DEBUG_ASSERT(!error);
     BinaryOperands operands;
     switch (fetch<uint8_t>()) {
@@ -424,7 +424,7 @@ GdbServerExpression::GdbServerExpression(const uint8_t* data, size_t size) {
 }
 #endif
 
-bool GdbServerExpression::evaluate(Task* t, Value* result) const {
+bool GdbServerExpression::evaluate(ReplayTask* t, Value* result) const {
   if (bytecode_variants.empty()) {
     return false;
   }

diff --git a/src/GdbServerExpression.h b/src/GdbServerExpression.h
@@ -10,7 +10,7 @@
 
 namespace rr {
 
-class Task;
+class ReplayTask;
 
 /**
  * gdb has a simple bytecode language for writing expressions to be evaluated
@@ -31,7 +31,7 @@ class GdbServerExpression {
    * If evaluation succeeds, store the final result in *result and return true.
    * Otherwise return false.
    */
-  bool evaluate(Task* t, Value* result) const;
+  bool evaluate(ReplayTask* t, Value* result) const;
 
 private:
   /**

diff --git a/src/RecordSession.cc b/src/RecordSession.cc
@@ -1649,9 +1649,11 @@ static bool inject_handled_signal(RecordTask* t) {
   // to 4.7 or thereabouts ptrace can still return stale values. Fix that here.
   // This also sets bit 0 of the XINUSE register to 1 to avoid issues where it
   // get set to 1 nondeterministically.
-  ExtraRegisters e = t->extra_regs();
-  e.reset();
-  t->set_extra_regs(e);
+  if (auto e_ptr = t->extra_regs_fallible()) {
+    ExtraRegisters e = *e_ptr;
+    e.reset();
+    t->set_extra_regs(e);
+  }
 
   return true;
 }

diff --git a/src/RecordTask.cc b/src/RecordTask.cc
@@ -2018,7 +2018,7 @@ void RecordTask::record_event(Event ev, FlushSyscallbuf flush,
       registers = &regs();
     }
     if (ev.record_extra_regs()) {
-      extra_registers = &extra_regs();
+      extra_registers = extra_regs_fallible();
     }
   }
 

diff --git a/src/ReplayCommand.cc b/src/ReplayCommand.cc
@@ -392,7 +392,7 @@ static void serve_replay_no_debugger(const string& trace_dir,
         replay_session->trace_reader().time() >= flags.singlestep_to_event) {
       cmd = RUN_SINGLESTEP;
       fputs("Stepping from: ", stderr);
-      Task* t = replay_session->current_task();
+      ReplayTask* t = ReplayTask::cast_or_null(replay_session->current_task());
       t->regs().print_register_file_compact(stderr);
       fputc(' ', stderr);
       t->extra_regs().print_register_file_compact(stderr);

diff --git a/src/ReplayTask.cc b/src/ReplayTask.cc
@@ -210,6 +210,20 @@ void ReplayTask::set_real_tid_and_update_serial(pid_t tid) {
   serial = session().next_task_serial();
 }
 
+/* static */ ReplayTask* ReplayTask::cast_or_null(Task* t) {
+  if (t->session().is_replaying() || t->session().is_diversion()) {
+    return static_cast<ReplayTask*>(t);
+  }
+  return nullptr;
+}
+
+const ExtraRegisters& ReplayTask::extra_regs() {
+  if (!extra_regs_fallible()) {
+    ASSERT(this, false) << "Can't find task for infallible extra_regs";
+  }
+  return extra_registers;
+}
+
 bool ReplayTask::post_vm_clone(CloneReason reason, int flags, Task* origin) {
   if (Task::post_vm_clone(reason, flags, origin) &&
       reason == TRACEE_CLONE &&

diff --git a/src/ReplayTask.h b/src/ReplayTask.h
@@ -78,6 +78,9 @@ class ReplayTask final : public Task {
    */
   void set_real_tid_and_update_serial(pid_t tid);
 
+  /** Return the extra registers of this. Asserts if the task died. */
+  const ExtraRegisters& extra_regs();
+
   void note_sched_in_syscallbuf_syscall_hook() {
     seen_sched_in_syscallbuf_syscall_hook = true;
   }
@@ -86,6 +89,8 @@ class ReplayTask final : public Task {
     return name_;
   }
 
+  static ReplayTask* cast_or_null(Task* t);
+
 private:
   template <typename Arch> void init_buffers_arch();
 

diff --git a/src/RerunCommand.cc b/src/RerunCommand.cc
@@ -217,7 +217,7 @@ static uint64_t seg_reg(const Registers& regs, uint8_t index) {
   }
 }
 
-static void print_regs(Task* t, FrameTime event, uint64_t instruction_count,
+static void print_regs(ReplayTask* t, FrameTime event, uint64_t instruction_count,
                        const RerunFlags& flags, const vector<TraceField>& fields, FILE* out) {
   if (fields.empty()) {
     return;
@@ -587,7 +587,7 @@ static const uint64_t sentinel_ret_address = 9;
 static void run_diversion_function(ReplaySession& replay, Task* task,
                                    const RerunFlags& flags) {
   DiversionSession::shr_ptr diversion_session = replay.clone_diversion();
-  Task* t = diversion_session->find_task(task->tuid());
+  ReplayTask* t = ReplayTask::cast_or_null(diversion_session->find_task(task->tuid()));
   Registers regs = t->regs();
   // align stack
   auto sp = remote_ptr<uint64_t>(regs.sp().as_int() & ~uintptr_t(0xf)) - 1;
@@ -669,7 +669,8 @@ static int rerun(const string& trace_dir, const RerunFlags& flags, CommandForChe
   while (replay_session->trace_reader().time() < flags.trace_end) {
     RunCommand cmd = RUN_CONTINUE;
 
-    Task* old_task = replay_session->current_task();
+    auto old_task_p = replay_session->current_task();
+    ReplayTask* old_task = old_task_p ? ReplayTask::cast_or_null(old_task_p) : nullptr;
     auto old_task_tuid = old_task ? old_task->tuid() : TaskUid();
     remote_code_ptr old_ip = old_task ? old_task->ip() : remote_code_ptr();
     FrameTime before_time = replay_session->trace_reader().time();
@@ -702,7 +703,8 @@ static int rerun(const string& trace_dir, const RerunFlags& flags, CommandForChe
     if (cmd != RUN_CONTINUE) {
       // The old_task may have exited (and been deallocated) in the `replay_session->replay_step(cmd)` above.
       // So we need to try and obtain it from the session again to make sure it still exists.
-      Task* old_task = old_task_tuid.tid() ? replay_session->find_task(old_task_tuid) : nullptr;
+      Task* old_task_p = old_task_tuid.tid() ? replay_session->find_task(old_task_tuid) : nullptr;
+      ReplayTask* old_task = old_task_p ? ReplayTask::cast_or_null(old_task_p) : nullptr;
       remote_code_ptr after_ip = old_task ? old_task->ip() : remote_code_ptr();
       DEBUG_ASSERT(after_time >= before_time && after_time <= before_time + 1);
 

diff --git a/src/Task.cc b/src/Task.cc
@@ -544,17 +544,21 @@ static void ptrace_syscall_exit_legacy_arch(Task* t, Task* tracee, const Registe
     case Arch::PTRACE_SETFPREGS: {
       auto data = t->read_mem(
           remote_ptr<typename Arch::user_fpregs_struct>(regs.arg4()));
-      auto r = tracee->extra_regs();
-      r.set_user_fpregs_struct(t, Arch::arch(), &data, sizeof(data));
-      tracee->set_extra_regs(r);
+      if (auto r_ptr = tracee->extra_regs_fallible()) {
+        ExtraRegisters r = *r_ptr;
+        r.set_user_fpregs_struct(t, Arch::arch(), &data, sizeof(data));
+        tracee->set_extra_regs(r);
+      }
       break;
     }
     case Arch::PTRACE_SETFPXREGS: {
       auto data =
           t->read_mem(remote_ptr<X86Arch::user_fpxregs_struct>(regs.arg4()));
-      auto r = tracee->extra_regs();
-      r.set_user_fpxregs_struct(t, data);
-      tracee->set_extra_regs(r);
+      if (auto r_ptr = tracee->extra_regs_fallible()) {
+        ExtraRegisters r = *r_ptr;
+        r.set_user_fpxregs_struct(t, data);
+        tracee->set_extra_regs(r);
+      }
       break;
     }
     case Arch::PTRACE_POKEUSR: {
@@ -749,10 +753,12 @@ void Task::on_syscall_exit_arch(int syscallno, const Registers& regs) {
             case NT_PRFPREG: {
               auto set = ptrace_get_regs_set<Arch>(
                   this, regs, user_fpregs_struct_size(tracee->arch()));
-              ExtraRegisters r = tracee->extra_regs();
-              r.set_user_fpregs_struct(this, tracee->arch(), set.data(),
-                                       set.size());
-              tracee->set_extra_regs(r);
+              if (auto r_ptr = tracee->extra_regs_fallible()) {
+                ExtraRegisters r = *r_ptr;
+                r.set_user_fpregs_struct(this, tracee->arch(), set.data(),
+                                         set.size());
+                tracee->set_extra_regs(r);
+              }
               break;
             }
             case NT_ARM_SYSTEM_CALL: {
@@ -777,29 +783,31 @@ void Task::on_syscall_exit_arch(int syscallno, const Registers& regs) {
               break;
             }
             case NT_X86_XSTATE: {
-              switch (tracee->extra_regs().format()) {
-                case ExtraRegisters::XSAVE: {
-                  XSaveLayout layout;
-                  ReplaySession* replay = session().as_replay();
-                  if (replay) {
-                    layout = xsave_layout_from_trace(
-                        replay->trace_reader().cpuid_records());
-                  } else {
-                    layout = xsave_native_layout();
+              if (auto extra_regs = tracee->extra_regs_fallible()) {
+                switch (extra_regs->format()) {
+                  case ExtraRegisters::XSAVE: {
+                    XSaveLayout layout;
+                    ReplaySession* replay = session().as_replay();
+                    if (replay) {
+                      layout = xsave_layout_from_trace(
+                          replay->trace_reader().cpuid_records());
+                    } else {
+                      layout = xsave_native_layout();
+                    }
+                    auto set = ptrace_get_regs_set<Arch>(this, regs, layout.full_size);
+                    ExtraRegisters r;
+                    bool ok =
+                        r.set_to_raw_data(tracee->arch(), ExtraRegisters::XSAVE,
+                                          set.data(), set.size(), layout);
+                    ASSERT(this, ok) << "Invalid XSAVE data";
+                    tracee->set_extra_regs(r);
+                    break;
                   }
-                  auto set = ptrace_get_regs_set<Arch>(this, regs, layout.full_size);
-                  ExtraRegisters r;
-                  bool ok =
-                      r.set_to_raw_data(tracee->arch(), ExtraRegisters::XSAVE,
-                                        set.data(), set.size(), layout);
-                  ASSERT(this, ok) << "Invalid XSAVE data";
-                  tracee->set_extra_regs(r);
-                  break;
+                  default:
+                    ASSERT(this, false) << "Unknown ExtraRegisters format; "
+                                           "Should have been caught during "
+                                           "prepare_ptrace";
                 }
-                default:
-                  ASSERT(this, false) << "Unknown ExtraRegisters format; "
-                                         "Should have been caught during "
-                                         "prepare_ptrace";
               }
               break;
             }
@@ -1043,7 +1051,7 @@ void Task::post_exec(const string& exe_file) {
 
   extra_registers = ExtraRegisters(registers.arch());
   extra_registers_known = false;
-  ExtraRegisters e = extra_regs();
+  ExtraRegisters e = *extra_regs_fallible();
   e.reset();
   set_extra_regs(e);
 
@@ -1174,13 +1182,6 @@ const ExtraRegisters* Task::extra_regs_fallible() {
   return &extra_registers;
 }
 
-const ExtraRegisters& Task::extra_regs() {
-  if (!extra_regs_fallible()) {
-    ASSERT(this, false) << "Can't find task for infallible extra_regs";
-  }
-  return extra_registers;
-}
-
 #if defined(__i386__) || defined(__x86_64__)
 static ssize_t dr_user_word_offset(size_t i) {
   DEBUG_ASSERT(i < NUM_X86_DEBUG_REGS);
@@ -2152,7 +2153,7 @@ static bool simulate_transient_error(Task* t) {
   static FrameTime simulate_error_at_event_ = simulate_error_at_event();
 
   if (simulated_error || !t->session().is_replaying() ||
-      static_cast<ReplayTask*>(t)->session().trace_stream()->time() < simulate_error_at_event_) {
+      ReplayTask::cast_or_null(t)->session().trace_stream()->time() < simulate_error_at_event_) {
     return false;
   }
   simulated_error = true;
@@ -2716,7 +2717,7 @@ Task::CapturedState Task::capture_state() {
   state.serial = serial;
   state.tguid = thread_group()->tguid();
   state.regs = regs();
-  state.extra_regs = extra_regs();
+  state.extra_regs = *extra_regs_fallible();
   state.prname = name();
   if (arch() == aarch64) {
     bool ok = read_aarch64_tls_register(&state.tls_register);

diff --git a/src/Task.h b/src/Task.h
@@ -504,9 +504,6 @@ class Task {
   /** Return the current regs of this. */
   const Registers& regs() const;
 
-  /** Return the extra registers of this. Asserts if the task died. */
-  const ExtraRegisters& extra_regs();
-
   /** Return the extra registers of this, or null if the task died. */
   const ExtraRegisters* extra_regs_fallible();