i#1729 offline traces: fix initial tid entry errors (#2461)

Fixes two issues with offline traces (#1729): first, changes the reader to
not assume a separate tid header right after the initial tid,pid header, as
offline traces (as opposed to online) do not contain it.  Second, changes
raw2trace to not emit a superfluous tid entry of 0 just prior to the real
initial tid entry for each thread.

Adds asserts to catch related errors in the future in the test suite.
The extra checks failed on the multiproc test so we fix that here for online
by adding a fork event handler that sends a new header to the simulator.
We partially solve #2384 by creating a new offline dir and files but more
work is needed to merge the final traces in the added test here: that's
left for #2384.

clients/drcachesim/reader/reader.cpp

@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2016 Google, Inc.  All rights reserved.
+ * Copyright (c) 2016-2017 Google, Inc.  All rights reserved.
  * **********************************************************/
 
 /*
@@ -91,6 +91,7 @@ reader_t::operator++()
         case TRACE_TYPE_PREFETCH_WRITE:
         case TRACE_TYPE_PREFETCH_INSTR:
             have_memref = true;
+            assert(cur_tid != 0 && cur_pid != 0);
             cur_ref.data.pid = cur_pid;
             cur_ref.data.tid = cur_tid;
             cur_ref.data.type = (trace_type_t) input_entry->type;
@@ -108,6 +109,7 @@ reader_t::operator++()
         case TRACE_TYPE_INSTR_INDIRECT_CALL:
         case TRACE_TYPE_INSTR_RETURN:
             have_memref = true;
+            assert(cur_tid != 0 && cur_pid != 0);
             cur_ref.instr.pid = cur_pid;
             cur_ref.instr.tid = cur_tid;
             cur_ref.instr.type = (trace_type_t) input_entry->type;
@@ -131,6 +133,7 @@ reader_t::operator++()
             break;
         case TRACE_TYPE_INSTR_FLUSH:
         case TRACE_TYPE_DATA_FLUSH:
+            assert(cur_tid != 0 && cur_pid != 0);
             cur_ref.flush.pid = cur_pid;
             cur_ref.flush.tid = cur_tid;
             cur_ref.flush.type = (trace_type_t) input_entry->type;
@@ -146,20 +149,25 @@ reader_t::operator++()
             break;
         case TRACE_TYPE_THREAD:
             cur_tid = (memref_tid_t) input_entry->addr;
+            // tid2pid might not be filled in yet: if so, we expect a
+            // TRACE_TYPE_PID entry right after this one, and later asserts
+            // will complain if it wasn't there.
             cur_pid = tid2pid[cur_tid];
             break;
         case TRACE_TYPE_THREAD_EXIT:
             cur_tid = (memref_tid_t) input_entry->addr;
             cur_pid = tid2pid[cur_tid];
+            assert(cur_tid != 0 && cur_pid != 0);
             // We do pass this to the caller but only some fields are valid:
             cur_ref.exit.pid = cur_pid;
             cur_ref.exit.tid = cur_tid;
             cur_ref.exit.type = (trace_type_t) input_entry->type;
             have_memref = true;
             break;
         case TRACE_TYPE_PID:
+            cur_pid = (memref_pid_t) input_entry->addr;
             // We do want to replace, in case of tid reuse.
-            tid2pid[cur_tid] = (memref_pid_t) input_entry->addr;
+            tid2pid[cur_tid] = cur_pid;
             break;
         default:
             ERRMSG("Unknown trace entry type %d\n", input_entry->type);

clients/drcachesim/tests/offline-multiproc.templatex

@@ -0,0 +1,20 @@
+all done
+Cache simulation results:
+Core #0 \(1 thread\(s\)\)
+  L1I stats:
+    Hits:                    *[0-9\.,]*
+    Misses:                  *[0-9,\.]*
+.*    Miss rate:             *[0-9]*[,\.]..%
+  L1D stats:
+    Hits:                    *[0-9\.,]*
+    Misses:                  *[0-9\.,]*
+.*   Miss rate:              *[0-9]*[,\.]..%
+Core #1 \(0 thread\(s\)\)
+Core #2 \(0 thread\(s\)\)
+Core #3 \(0 thread\(s\)\)
+LL stats:
+    Hits:                    *[0-9\.,]*
+    Misses:                  *[0-9\.,]*
+.*    Local miss rate:       *[0-9]*[,\.]..%
+    Child hits:              *[0-9\.,]*
+    Total miss rate:         *[0-9]*[,\.]..%

clients/drcachesim/tracer/raw2trace.cpp

@@ -443,12 +443,18 @@ raw2trace_t::merge_and_process_thread_files()
                    "\n", (uint)tids[next_tidx], times[next_tidx]);
             tidx = next_tidx;
             times[tidx] = 0; // Read from file for this thread's next timestamp.
-            size += instru.append_tid(buf, tids[tidx]);
-            // We have to write this now before we append any bb entries.
-            CHECK((uint)size < MAX_COMBINED_ENTRIES, "Too many entries");
-            if (!out_file.write((char*)buf_base, size))
-                FATAL_ERROR("Failed to write to output file");
-            buf = buf_base;
+            if (tids[tidx] != INVALID_THREAD_ID) {
+                // The initial read from a file may not have seen its tid entry
+                // yet.  We expect to hit that entry next.
+                size += instru.append_tid(buf, tids[tidx]);
+            }
+            if (size > 0) {
+                // We have to write this now before we append any bb entries.
+                CHECK((uint)size < MAX_COMBINED_ENTRIES, "Too many entries");
+                if (!out_file.write((char*)buf_base, size))
+                    FATAL_ERROR("Failed to write to output file");
+                buf = buf_base;
+            }
             size = 0;
         }
         VPRINT(4, "About to read thread %d at pos %d\n",

clients/drcachesim/tracer/tracer.cpp

@@ -768,24 +768,15 @@ event_pre_syscall(void *drcontext, int sysnum)
     return true;
 }
 
+/* Initializes a thread either at process init or fork init, where we want
+ * a new offline file or a new thread,process registration pair for online.
+ */
 static void
-event_thread_init(void *drcontext)
+init_thread_in_process(void *drcontext)
 {
+    per_thread_t *data = (per_thread_t *) drmgr_get_tls_field(drcontext, tls_idx);
     char buf[MAXIMUM_PATH];
     byte *proc_info;
-    per_thread_t *data = (per_thread_t *)
-        dr_thread_alloc(drcontext, sizeof(per_thread_t));
-    DR_ASSERT(data != NULL);
-    memset(data, 0, sizeof(*data));
-    drmgr_set_tls_field(drcontext, tls_idx, data);
-
-    /* Keep seg_base in a per-thread data structure so we can get the TLS
-     * slot and find where the pointer points to in the buffer.
-     */
-    data->seg_base = (byte *) dr_get_dr_segment_base(tls_seg);
-    DR_ASSERT(data->seg_base != NULL);
-    create_buffer(data);
-
     if (op_offline.get_value()) {
         /* We do not need to call drx_init before using drx_open_unique_appid_file.
          * Since we're now in a subdir we could make the name simpler but this
@@ -839,6 +830,27 @@ event_thread_init(void *drcontext)
     // XXX i#1729: gather and store an initial callstack for the thread.
 }
 
+static void
+event_thread_init(void *drcontext)
+{
+    per_thread_t *data = (per_thread_t *)
+        dr_thread_alloc(drcontext, sizeof(per_thread_t));
+    DR_ASSERT(data != NULL);
+    memset(data, 0, sizeof(*data));
+    drmgr_set_tls_field(drcontext, tls_idx, data);
+
+    /* Keep seg_base in a per-thread data structure so we can get the TLS
+     * slot and find where the pointer points to in the buffer.
+     */
+    data->seg_base = (byte *) dr_get_dr_segment_base(tls_seg);
+    DR_ASSERT(data->seg_base != NULL);
+    create_buffer(data);
+
+    init_thread_in_process(drcontext);
+
+    // XXX i#1729: gather and store an initial callstack for the thread.
+}
+
 static void
 event_thread_exit(void *drcontext)
 {
@@ -951,6 +963,29 @@ init_offline_dir(void)
     return (module_file != INVALID_FILE);
 }
 
+#ifdef UNIX
+static void
+fork_init(void *drcontext)
+{
+    /* We use DR_FILE_CLOSE_ON_FORK, and we dumped outstanding data prior to the
+     * fork syscall, so we just need to create a new subdir, new module log, and
+     * a new initial thread file for offline, or register the new process for
+     * online.
+     */
+    per_thread_t *data = (per_thread_t *) drmgr_get_tls_field(drcontext, tls_idx);
+    /* Only count refs in the new process (plus, we use this to set up the
+     * initial header in memtrace() for offline).
+     */
+    data->num_refs = 0;
+    if (op_offline.get_value()) {
+        if (!init_offline_dir()) {
+            FATAL("Failed to create a subdir in %s\n", op_outdir.get_value().c_str());
+        }
+    }
+    init_thread_in_process(drcontext);
+}
+#endif
+
 /* We export drmemtrace_client_main so that a global dr_client_main can initialize
  * drmemtrace client by calling drmemtrace_client_main in a statically linked
  * multi-client executable.
@@ -1024,6 +1059,9 @@ drmemtrace_client_main(client_id_t id, int argc, const char *argv[])
 
     /* register events */
     dr_register_exit_event(event_exit);
+#ifdef UNIX
+    dr_register_fork_init_event(fork_init);
+#endif
     if (!drmgr_register_thread_init_event(event_thread_init) ||
         !drmgr_register_thread_exit_event(event_thread_exit) ||
         !drmgr_register_pre_syscall_event(event_pre_syscall) ||

suite/tests/CMakeLists.txt

@@ -2495,10 +2495,10 @@ if (CLIENT_INTERFACE)
       get_target_path_for_execution(drcachesim_path drcachesim)
       prefix_cmd_if_necessary(drcachesim_path ON ${drcachesim_path})
 
-      macro (torunonly_drcacheoff testname exetgt)
+      macro (torunonly_drcacheoff testname exetgt app_args)
         torunonly_ci(tool.drcacheoff.${testname} ${exetgt} drcachesim
           "offline-${testname}.c" # for templatex basename
-          "-offline" "" "")
+          "-offline" "" "${app_args}")
         set(tool.drcacheoff.${testname}_toolname "drcachesim")
         set(tool.drcacheoff.${testname}_basedir
           "${PROJECT_SOURCE_DIR}/clients/drcachesim/tests")
@@ -2513,28 +2513,36 @@ if (CLIENT_INTERFACE)
 
       # We could share drcachesim-simple.templatex if we had the launcher fork
       # and print out the "---- <application exited with code 0> ----".
-      torunonly_drcacheoff(simple ${ci_shared_app})
+      torunonly_drcacheoff(simple ${ci_shared_app} "")
+
+      if (UNIX) # Enable on Windows once i#1727 is fixed.
+        # FIXME i#2384: we need to combine the subdir from the child with the
+        # parent in some way and update the templatex file to include both.
+        # Right now we're only ensuring this doesn't crash and that one of
+        # the processes produced traces.
+        torunonly_drcacheoff(multiproc tool.multiproc "${tool.multiproc_path}")
+      endif ()
 
       # FIXME i#2007: fails to link on A64
       # XXX i#1551: startstop API is NYI on ARM
       # XXX i#1997: dynamorio_static is not supported on Mac yet
       if (NOT AARCH64 AND NOT ARM AND NOT APPLE)
-        torunonly_drcacheoff(burst_static tool.drcacheoff.burst_static)
+        torunonly_drcacheoff(burst_static tool.drcacheoff.burst_static "")
         set(tool.drcacheoff.burst_static_nodr ON)
 
-        torunonly_drcacheoff(burst_replace tool.drcacheoff.burst_replace)
+        torunonly_drcacheoff(burst_replace tool.drcacheoff.burst_replace "")
         set(tool.drcacheoff.burst_replace_nodr ON)
 
-        torunonly_drcacheoff(burst_replaceall tool.drcacheoff.burst_replaceall)
+        torunonly_drcacheoff(burst_replaceall tool.drcacheoff.burst_replaceall "")
         set(tool.drcacheoff.burst_replaceall_nodr ON)
 
         if (UNIX)
           # FIXME i#2040: this hits static client issues on Windows
-          torunonly_drcacheoff(burst_threads tool.drcacheoff.burst_threads)
+          torunonly_drcacheoff(burst_threads tool.drcacheoff.burst_threads "")
           set(tool.drcacheoff.burst_threads_nodr ON)
 
           # FIXME i#2099: the weak symbol is not supported not work on Windows
-          torunonly_drcacheoff(burst_client tool.drcacheoff.burst_client)
+          torunonly_drcacheoff(burst_client tool.drcacheoff.burst_client "")
           set(tool.drcacheoff.burst_client_nodr ON)
         endif ()
       endif ()