Move RR_MAGIC_SAVE_DATA_FD support to MagicSaveDataMonitor

CMakeLists.txt

@@ -100,6 +100,7 @@ add_executable(rr
   src/kernel_abi.cc
   src/kernel_metadata.cc
   src/log.cc
+  src/MagicSaveDataMonitor.cc
   src/main.cc
   src/Monkeypatcher.cc
   src/PerfCounters.cc

include/rr/rr.h

@@ -12,7 +12,13 @@
  * Tracees using this interface should take care that the buffers
  * storing the data are either not racy, or are synchronized by the
  * tracee.
+ *
+ * To simplify things, we make this a valid fd opened to /dev/null during
+ * recording.
+ *
+ * Tracees may close this fd, or dup() something over it, etc. If that happens,
+ * it will lose its magical properties.
  */
-#define RR_MAGIC_SAVE_DATA_FD (-42)
+#define RR_MAGIC_SAVE_DATA_FD 999
 
 #endif /* RR_H_ */

src/MagicSaveDataMonitor.cc

@@ -0,0 +1,66 @@
+/* -*- Mode: C++; tab-width: 8; c-basic-offset: 2; indent-tabs-mode: nil; -*- */
+
+#include "MagicSaveDataMonitor.h"
+
+#include <rr/rr.h>
+
+#include "log.h"
+#include "Session.h"
+#include "task.h"
+#include "util.h"
+
+static void dump_path_data(Task* t, TraceFrame::Time global_time,
+                           const char* tag, char* filename,
+                           size_t filename_size, const void* buf,
+                           size_t buf_len, remote_ptr<void> addr) {
+  format_dump_filename(t, global_time, tag, filename, filename_size);
+  dump_binary_data(filename, tag, (const uint32_t*)buf, buf_len / 4, addr);
+}
+
+static void notify_save_data_error(Task* t, remote_ptr<void> addr,
+                                   const void* rec_buf, size_t rec_buf_len,
+                                   const void* rep_buf, size_t rep_buf_len) {
+  char rec_dump[PATH_MAX];
+  char rep_dump[PATH_MAX];
+  TraceFrame::Time global_time = t->current_trace_frame().time();
+
+  dump_path_data(t, global_time, "rec_save_data", rec_dump, sizeof(rec_dump),
+                 rec_buf, rec_buf_len, addr);
+  dump_path_data(t, global_time, "rep_save_data", rep_dump, sizeof(rep_dump),
+                 rep_buf, rep_buf_len, addr);
+
+  ASSERT(t,
+         (rec_buf_len == rep_buf_len && !memcmp(rec_buf, rep_buf, rec_buf_len)))
+      << "Divergence in contents of 'tracee-save buffer'.  Recording executed\n"
+         "\n"
+         "  write(" << RR_MAGIC_SAVE_DATA_FD << ", " << addr << ", "
+      << rec_buf_len << ")\n"
+                        "\n"
+                        "and replay executed\n"
+                        "\n"
+                        "  write(" << RR_MAGIC_SAVE_DATA_FD << ", " << addr
+      << ", " << rep_buf_len
+      << ")\n"
+         "\n"
+         "The contents of the tracee-save buffers have been dumped to disk.\n"
+         "Compare them by using the following command\n"
+         "\n"
+         "$ diff -u " << rec_dump << " " << rep_dump
+      << " >save-data-diverge.diff\n";
+}
+
+void MagicSaveDataMonitor::did_write(Task* t,
+                                     const std::vector<Range>& ranges) {
+  for (auto& r : ranges) {
+    if (t->session().is_recording()) {
+      t->record_remote(r.data.cast<uint8_t>(), r.length);
+    } else if (t->session().is_replaying()) {
+      auto bytes = t->read_mem(r.data.cast<uint8_t>(), r.length);
+      auto rec = t->trace_reader().read_raw_data();
+      if (rec.data != bytes) {
+        notify_save_data_error(t, rec.addr, rec.data.data(), rec.data.size(),
+                               bytes.data(), bytes.size());
+      }
+    }
+  }
+}

src/MagicSaveDataMonitor.h

@@ -0,0 +1,24 @@
+/* -*- Mode: C++; tab-width: 8; c-basic-offset: 2; indent-tabs-mode: nil; -*- */
+
+#ifndef RR_MAGIC_SAVE_DATA_MONITOR_H_
+#define RR_MAGIC_SAVE_DATA_MONITOR_H_
+
+#include "FileMonitor.h"
+
+/**
+ * A FileMonitor to track writes to RR_MAGIC_SAVE_DATA_FD.
+ * Currently does nothing other than prevent syscallbuf from buffering output
+ * to those fds.
+ */
+class MagicSaveDataMonitor : public FileMonitor {
+public:
+  MagicSaveDataMonitor() {}
+
+  /**
+   * During recording, record the written data.
+   * During replay, check that the written data matches what was recorded.
+   */
+  virtual void did_write(Task* t, const std::vector<Range>& ranges);
+};
+
+#endif /* RR_MAGIC_SAVE_DATA_MONITOR_H_ */

src/RecordSession.cc

@@ -808,7 +808,6 @@ void RecordSession::runnable_state_changed(Task* t, RecordResult* step_result) {
         }
 
         t->push_event(SyscallEvent(t->regs().original_syscallno(), t->arch()));
-        rec_before_record_syscall_entry(t, t->ev().Syscall().number);
       }
       ASSERT(t, EV_SYSCALL == t->ev().type());
       check_perf_counters_working(t, step_result);

src/preload/preload.c

@@ -74,7 +74,6 @@
 #include <unistd.h>
 
 /* NB: don't include any other local headers here. */
-#include "rr/rr.h"
 
 #ifdef memcpy
 #undef memcpy
@@ -1624,12 +1623,7 @@ static long sys_write(const struct syscall_info* call) {
 
   assert(syscallno == call->no);
 
-  /* We always have to trap for writes to the magic save-data
-   * fd, because the rr tracer processes them specially.
-   *
-   * TODO: buffer them normally here. */
-  if (RR_MAGIC_SAVE_DATA_FD == fd ||
-      !start_commit_buffered_syscall(syscallno, ptr, MAY_BLOCK)) {
+  if (!start_commit_buffered_syscall(syscallno, ptr, MAY_BLOCK)) {
     return traced_raw_syscall(call);
   }
 

src/record_syscall.cc

@@ -674,28 +674,6 @@ void TaskSyscallState::process_syscall_results() {
   }
 }
 
-template <typename Arch>
-static void rec_before_record_syscall_entry_arch(Task* t, int syscallno) {
-  if (Arch::write != syscallno) {
-    return;
-  }
-  int fd = t->regs().arg1_signed();
-  if (RR_MAGIC_SAVE_DATA_FD != fd) {
-    return;
-  }
-  remote_ptr<void> buf = t->regs().arg2();
-  size_t len = t->regs().arg3();
-
-  ASSERT(t, buf) << "Can't save a null buffer";
-
-  t->record_remote(buf, len);
-}
-
-void rec_before_record_syscall_entry(Task* t, int syscallno) {
-  RR_ARCH_FUNCTION(rec_before_record_syscall_entry_arch, t->arch(), t,
-                   syscallno)
-}
-
 template <typename Arch>
 static void prepare_recvmsg(Task* t, TaskSyscallState& syscall_state,
                             remote_ptr<typename Arch::msghdr> msgp,

src/record_syscall.h

@@ -5,13 +5,6 @@
 
 #include "task.h"
 
-/**
- * Call this just before the recorder is going to store a
- * syscall-entry event.  If any data needs to be saved at syscall
- * entry, do it now.
- */
-void rec_before_record_syscall_entry(Task* t, int syscallno);
-
 /**
  * Prepare |t| to enter its current syscall event.  Return ALLOW_SWITCH if
  * a context-switch is allowed for |t|, PREVENT_SWITCH if not.

src/replay_syscall.cc

@@ -24,8 +24,6 @@
 #include <sstream>
 #include <string>
 
-#include <rr/rr.h>
-
 #include "preload/preload_interface.h"
 
 #include "AutoRemoteSyscalls.h"
@@ -786,76 +784,6 @@ static void process_init_buffers(Task* t, SyscallEntryOrExit state,
   t->validate_regs();
 }
 
-static void dump_path_data(Task* t, int global_time, const char* tag,
-                           char* filename, size_t filename_size,
-                           const void* buf, size_t buf_len,
-                           remote_ptr<void> addr) {
-  format_dump_filename(t, global_time, tag, filename, filename_size);
-  dump_binary_data(filename, tag, (const uint32_t*)buf, buf_len / 4, addr);
-}
-
-static void notify_save_data_error(Task* t, remote_ptr<void> addr,
-                                   const void* rec_buf, size_t rec_buf_len,
-                                   const void* rep_buf, size_t rep_buf_len) {
-  char rec_dump[PATH_MAX];
-  char rep_dump[PATH_MAX];
-  int global_time = t->current_trace_frame().time();
-
-  dump_path_data(t, global_time, "rec_save_data", rec_dump, sizeof(rec_dump),
-                 rec_buf, rec_buf_len, addr);
-  dump_path_data(t, global_time, "rep_save_data", rep_dump, sizeof(rep_dump),
-                 rep_buf, rep_buf_len, addr);
-
-  ASSERT(t,
-         (rec_buf_len == rep_buf_len && !memcmp(rec_buf, rep_buf, rec_buf_len)))
-      << "Divergence in contents of 'tracee-save buffer'.  Recording executed\n"
-         "\n"
-         "  write(" << RR_MAGIC_SAVE_DATA_FD << ", " << addr << ", "
-      << rec_buf_len << ")\n"
-                        "\n"
-                        "and replay executed\n"
-                        "\n"
-                        "  write(" << RR_MAGIC_SAVE_DATA_FD << ", " << addr
-      << ", " << rep_buf_len
-      << ")\n"
-         "\n"
-         "The contents of the tracee-save buffers have been dumped to disk.\n"
-         "Compare them by using the following command\n"
-         "\n"
-         "$ diff -u " << rec_dump << " " << rep_dump
-      << " >save-data-diverge.diff\n";
-}
-
-/**
- * If the tracee saved data in this syscall to the magic save-data fd,
- * read and check the replay buffer against the one saved during
- * recording.
- */
-static void maybe_verify_tracee_saved_data(Task* t, const Registers& rec_regs) {
-  int fd = rec_regs.arg1_signed();
-  remote_ptr<void> rep_addr = rec_regs.arg2();
-  size_t rep_len = rec_regs.arg3();
-
-  if (RR_MAGIC_SAVE_DATA_FD != fd) {
-    return;
-  }
-
-  auto rec = t->trace_reader().read_raw_data();
-
-  // If the data address changed, something disastrous happened
-  // and the buffers aren't comparable.  Just bail.
-  ASSERT(t, rec.addr == rep_addr) << "Recorded write(" << rec.addr
-                                  << ") being replayed as write(" << rep_addr
-                                  << ")";
-
-  uint8_t rep_buf[rep_len];
-  t->read_bytes_helper(rep_addr, sizeof(rep_buf), rep_buf);
-  if (rec.data.size() != rep_len || memcmp(rec.data.data(), rep_buf, rep_len)) {
-    notify_save_data_error(t, rec.addr, rec.data.data(), rec.data.size(),
-                           rep_buf, rep_len);
-  }
-}
-
 template <typename Arch>
 static void rep_after_enter_syscall_arch(Task* t, int syscallno) {
   switch (syscallno) {
@@ -864,8 +792,6 @@ static void rep_after_enter_syscall_arch(Task* t, int syscallno) {
       return;
 
     case Arch::write:
-      maybe_verify_tracee_saved_data(t, t->current_trace_frame().regs());
-    // fall through
     case Arch::writev: {
       int fd = (int)t->regs().arg1_signed();
       t->fd_table()->will_write(t, fd);

src/task.cc

@@ -20,14 +20,17 @@
 #include <limits>
 #include <set>
 
+#include <rr/rr.h>
+
 #include "preload/preload_interface.h"
 
+#include "AutoRemoteSyscalls.h"
 #include "CPUIDBugDetector.h"
 #include "kernel_abi.h"
 #include "kernel_metadata.h"
 #include "kernel_supplement.h"
 #include "log.h"
-#include "AutoRemoteSyscalls.h"
+#include "MagicSaveDataMonitor.h"
 #include "RecordSession.h"
 #include "record_signal.h"
 #include "ReplaySession.h"
@@ -1769,16 +1772,23 @@ bool Task::try_wait() {
  * that rr bugs don't adversely affect the underlying system.
  */
 static void set_up_process() {
-  int orig_pers;
-
   /* TODO tracees can probably undo some of the setup below
    * ... */
 
+  int fd = open("/dev/null", O_WRONLY | O_CLOEXEC);
+  if (0 > fd) {
+    FATAL() << "error opening /dev/null";
+  }
+  if (RR_MAGIC_SAVE_DATA_FD != dup2(fd, RR_MAGIC_SAVE_DATA_FD)) {
+    FATAL() << "error duping to RR_MAGIC_SAVE_DATA_FD";
+  }
+
   /* Disable address space layout randomization, for obvious
    * reasons, and ensure that the layout is otherwise well-known
    * ("COMPAT").  For not-understood reasons, "COMPAT" layouts
    * have been observed in certain recording situations but not
    * in replay, which causes divergence. */
+  int orig_pers;
   if (0 > (orig_pers = personality(0xffffffff))) {
     FATAL() << "error getting personaity";
   }
@@ -2601,6 +2611,7 @@ static void perform_remote_clone(Task* parent, AutoRemoteSyscalls& remote,
 static void setup_fd_table(FdTable& fds) {
   fds.add_monitor(STDOUT_FILENO, new StdioMonitor(STDOUT_FILENO));
   fds.add_monitor(STDERR_FILENO, new StdioMonitor(STDERR_FILENO));
+  fds.add_monitor(RR_MAGIC_SAVE_DATA_FD, new MagicSaveDataMonitor());
 }
 
 /*static*/ Task* Task::spawn(Session& session, const TraceStream& trace,