i#2511: isolate raw2trace #2512

s-kanev · 2017-07-07T21:50:53Z

Splits out a self-contained library, which:

only takes already opened std::istream and std::ostream-compatible files
and doesn't own them, so it can be used with more exotic filesystems;
takes in the contents of the module file to avoid doing IO from
drmodtrack_offline_read();
returns errors on failure;
doesn't depend on dr_frontend.

Preserves the current behavior with raw2trace_helper_t, which walks
directories and opens files.

TESTED: ctest (failures failed before this commit as well)

Splits out a self-contained library, which: - only takes already opened std::istream and std::ostream-compatible files and doesn't own them, so it can be used with more exotic filesystems; - takes in the contents of the module file to avoid doing IO from drmodtrack_offline_read(); - returns errors on failure; - doesn't depend on dr_frontend. Preserves the current behavior with raw2trace_helper_t, which walks directories and opens files. TESTED: ctest (failures failed before this commit as well) Fixes DynamoRIO#2511

s-kanev · 2017-07-07T22:33:19Z

AppVeyor seems to fail on installing doxygen. I don't have a Windows box handy, if you give me credentials on one, I can take a look.

derekbruening · 2017-07-10T03:42:07Z

This doxygen install issue appeared a few days ago out of the blue, I was hoping it was some transient failure that would just go away (initially it surfaced as more of a network not-found error) but it doesn't seem to be the case.

fhahn · 2017-07-10T10:32:24Z

I think choco might got upgraded and the new default now is to not allow empty checksums by default. Ideally, the doxygen package would come with a proper checksum, but for now, installing it with --allow-empty-checksums should fix the builds: #2513

fhahn · 2017-07-10T15:38:42Z

The appveyor builds should be fixed now, I've used the Update branch button, to trigger a new build.

derekbruening

Mostly minor style issues

derekbruening · 2017-07-10T03:43:07Z

clients/drcachesim/CMakeLists.txt

@@ -89,6 +89,7 @@ add_executable(drcachesim
  simulator/analyzer_interface.cpp
  # We embed the raw2trace conversion for convenience:
  tracer/raw2trace.cpp
+  tracer/raw2trace_helper.cpp


How about "directory" or "dir_iter" instead of "helper"?

Done. Yup, it's a much better name.

derekbruening · 2017-07-10T03:45:47Z

clients/drcachesim/tracer/raw2trace.cpp

-void
-raw2trace_t::read_and_map_modules(void)
+std::string
+raw2trace_t::read_and_map_modules(const char* module_map)


style: C-style "char *m..."

derekbruening · 2017-07-10T15:57:33Z

clients/drcachesim/tracer/raw2trace.cpp

-trace_entry_t *
-raw2trace_t::append_memref(trace_entry_t *buf_in, uint tidx, instr_t *instr,
+std::string
+raw2trace_t::append_memref(trace_entry_t **buf_in, uint tidx, instr_t *instr,


derekbruening · 2017-07-10T15:57:44Z

clients/drcachesim/tracer/raw2trace.cpp

-bool
-raw2trace_t::append_bb_entries(uint tidx, offline_entry_t *in_entry)
+std::string
+raw2trace_t::append_bb_entries(uint tidx, offline_entry_t *in_entry, bool *handled)


derekbruening · 2017-07-10T16:00:45Z

clients/drcachesim/tracer/raw2trace.cpp

        }
    } while (thread_count > 0);
+    return std::string();


Sometimes you're using 'return ""' and others 'return std::string()' -- best to pick one. IMHO "" is clearer.

derekbruening · 2017-07-10T16:11:08Z

clients/drcachesim/tracer/raw2trace_helper.cpp

+ */
+
+/* raw2trace helper to iterate directories and open files.
+ * Separate from raw2trace_t, so that raw2trace doesn't depend on dr_frontend. */


style: prefer */ on own line

derekbruening · 2017-07-10T16:11:35Z

clients/drcachesim/tracer/raw2trace_helper.cpp

+} while (0)
+
+#define CHECK(val, msg, ...) do { \
+      if (!(val)) FATAL_ERROR(msg, ##__VA_ARGS__); \


indent looks off

derekbruening · 2017-07-10T16:12:38Z

clients/drcachesim/tracer/raw2trace_helper.cpp

+      if (this->verbosity >= (level)) { \
+                fprintf(stderr, "[drmemtrace]: "); \
+                fprintf(stderr, __VA_ARGS__); \
+            } \


indent is off in above lines

derekbruening · 2017-07-10T16:16:27Z

clients/drcachesim/tracer/raw2trace_helper.cpp

+}
+
+raw2trace_helper_t::raw2trace_helper_t(const std::string& indir_in,
+                                       const std::string& outname_in,


derekbruening · 2017-07-10T16:16:49Z

clients/drcachesim/tracer/raw2trace_helper.h

+
+class raw2trace_helper_t {
+public:
+    raw2trace_helper_t(const std::string& indir, const std::string& outname,


s-kanev

Sorry about the many style violations. I'll see if I can contribute a minimal .clang_format to catch similar ones.

s-kanev · 2017-07-10T19:58:49Z

clients/drcachesim/CMakeLists.txt

@@ -89,6 +89,7 @@ add_executable(drcachesim
  simulator/analyzer_interface.cpp
  # We embed the raw2trace conversion for convenience:
  tracer/raw2trace.cpp
+  tracer/raw2trace_helper.cpp


Done. Yup, it's a much better name.

s-kanev · 2017-07-10T19:59:36Z

clients/drcachesim/tracer/raw2trace.cpp

-void
-raw2trace_t::read_and_map_modules(void)
+std::string
+raw2trace_t::read_and_map_modules(const char* module_map)


s-kanev · 2017-07-10T20:07:55Z

clients/drcachesim/tracer/raw2trace.cpp

-trace_entry_t *
-raw2trace_t::append_memref(trace_entry_t *buf_in, uint tidx, instr_t *instr,
+std::string
+raw2trace_t::append_memref(trace_entry_t **buf_in, uint tidx, instr_t *instr,


s-kanev · 2017-07-10T20:08:23Z

clients/drcachesim/tracer/raw2trace.cpp

-bool
-raw2trace_t::append_bb_entries(uint tidx, offline_entry_t *in_entry)
+std::string
+raw2trace_t::append_bb_entries(uint tidx, offline_entry_t *in_entry, bool *handled)


s-kanev · 2017-07-10T20:10:30Z

clients/drcachesim/tracer/raw2trace.cpp

        }
    } while (thread_count > 0);
+    return std::string();


s-kanev · 2017-07-10T20:13:57Z

clients/drcachesim/tracer/raw2trace_helper.cpp

+} while (0)
+
+#define CHECK(val, msg, ...) do { \
+      if (!(val)) FATAL_ERROR(msg, ##__VA_ARGS__); \


s-kanev · 2017-07-10T20:14:04Z

clients/drcachesim/tracer/raw2trace_helper.cpp

+      if (this->verbosity >= (level)) { \
+                fprintf(stderr, "[drmemtrace]: "); \
+                fprintf(stderr, __VA_ARGS__); \
+            } \


s-kanev · 2017-07-10T20:14:34Z

clients/drcachesim/tracer/raw2trace_helper.cpp

+    thread_files.push_back(new std::ifstream(path, std::ifstream::binary));
+    if (!(*thread_files.back()))
+        FATAL_ERROR("Failed to open thread log file %s", path);
+    raw2trace_t::check_thread_file(thread_files.back());


Done. Thanks for catching.

s-kanev · 2017-07-10T20:15:16Z

clients/drcachesim/tracer/raw2trace_helper.cpp

+}
+
+raw2trace_helper_t::raw2trace_helper_t(const std::string& indir_in,
+                                       const std::string& outname_in,


s-kanev · 2017-07-10T20:15:25Z

clients/drcachesim/tracer/raw2trace_helper.h

+
+class raw2trace_helper_t {
+public:
+    raw2trace_helper_t(const std::string& indir, const std::string& outname,


derekbruening

Could we add a test for the i/o interface? The existing code is tested in the various torunonly_drcacheoff() tests through running -indir.

derekbruening · 2017-07-11T00:51:46Z

clients/drcachesim/tracer/raw2trace_directory.h

+                          unsigned int verbosity = 0);
+    ~raw2trace_directory_t();
+
+    char* modfile_bytes;


One more style issue.

derekbruening · 2017-07-11T00:54:55Z

clients/drcachesim/tracer/raw2trace_launcher.cpp

 (DROPTION_SCOPE_FRONTEND, "verbose", 0, "Verbosity level for diagnostic output",
 "Verbosity level for diagnostic output.");

+#define FATAL_ERROR(msg, ...) do { \
+      fprintf(stderr, "ERROR: " msg "\n", ##__VA_ARGS__);    \


indentation: Looks like this is 6 spaces?

s-kanev · 2017-07-11T22:29:16Z

Could we add a test for the i/o interface?

Sure, added one through the drraw2trace tool. PTAL because I'm very bad at cmake.
There's also a slightly finer-grained test in this change's internal counterpart.

derekbruening · 2017-07-12T04:18:28Z

Could we add a test for the i/o interface?

Sure, added one through the drraw2trace tool. PTAL because I'm very bad at cmake.

Hmm, I guess I was expecting a custom test of the interface for other than the built-in dir iterator, to ensure the interface fully isolates filesystem access: akin to the burst_replaceall test.

derekbruening · 2017-07-12T13:58:00Z

Could we add a test for the i/o interface?

Sure, added one through the drraw2trace tool. PTAL because I'm very bad at cmake.

Hmm, I guess I was expecting a custom test of the interface for other than the built-in dir iterator, to ensure the interface fully isolates filesystem access: akin to the burst_replaceall test.

I think my pointing at the existing end-to-end -indir tests was misleading: I meant that to say we already have a test of the dir iterator, not to suggest that a new test of using the interface for different code from the -indir should follow that template.

s-kanev · 2017-07-12T14:44:12Z

I meant that to say we already have a test of the dir iterator

Gotcha, I didn't realize drcachesim's -indir exercises the iterator as well (through analyzer_multi_t), though in hindsight that's obvious. In this case, what I added is definitely redundant.
I'll add a standalone test that just invokes raw2trace_t on a set of raw files, similar to what we do downstream.

s-kanev · 2017-07-14T17:58:48Z

This took embarrassingly long. I couldn't use the file op replacement API like in burst_replaceall, because the code we're trying to trace itself calls drmodtrack_* and that didn't seem to isolate well (always ended up segfaulting). So, I ended up just doing a simple ptrace syscall recorder and checking that all new files that we open are only because of loading new modules.
We could get more sophisticated -- e.g. checking that all trace reads/writes are to pre-opened file descriptors -- but I don't think it's worth the extra effort.

derekbruening · 2017-07-16T00:45:28Z

clients/drcachesim/CMakeLists.txt

    if (UNIX)
+      # uses ptrace, which is unix-only
+      add_executable(tool.drcacheoff.raw2trace_io tests/raw2trace_io.cpp
+                  tracer/instru.cpp


Indent should be just 2 here and line below

derekbruening · 2017-07-16T00:53:16Z

clients/drcachesim/tests/raw2trace_io.cpp

+                return 1;
+            }
+            /* We don't distinguish syscall entry/exit because orig_rax is set
+             * on both. */


style: prefer */ on own line

derekbruening · 2017-07-16T00:55:10Z

clients/drcachesim/tests/raw2trace_io.cpp

+            /* We don't distinguish syscall entry/exit because orig_rax is set
+             * on both. */
+            if (SYS_NUM(regs) == __NR_open || SYS_NUM(regs) == __NR_openat ||
+                SYS_NUM(regs) == __NR_creat) {


In cmake UNIX includes MacOS where there is no __NR_creat. If it's simpler you could just exclude in cmake (and ifdef sanity check at top of this file) via NOT APPLE.

derekbruening · 2017-07-16T00:58:05Z

clients/drcachesim/tests/raw2trace_io.cpp

+     * so they never get closed.
+     */
+    raw2trace_directory_t *dir =
+        new raw2trace_directory_t(op_indir.get_value(), op_out.get_value());


Hmm, ok, it's not quite what I was thinking of (which was more of a test that actually uses the new i/o interface), but it does test the i/o isolation.

s-kanev · 2017-07-18T14:59:11Z

@derekbruening, can you merge this? The CI failure looks completely unrelated -- I'm assuming from a flaky test.

derekbruening · 2017-07-18T15:35:30Z

OK, the general policy is to cite the issue for any flaky test or file one if it doesn't exist. That looks like #1308. This needs updating to master anyway so it will re-run.

Fixes a header guard error from 66fa2c7.

s-kanev added 2 commits July 7, 2017 14:46

Formatting

6f26231

Merge branch 'master' into i2511-raw2tracelib

5a505e5

derekbruening reviewed Jul 10, 2017

View reviewed changes

Style

3d242c5

s-kanev commented Jul 10, 2017

View reviewed changes

s-kanev added 3 commits July 10, 2017 13:35

More style

e508678

Include TESTANY in drcachesim utils.h

a0f7928

Fix VC++ false positive warning

ae91563

derekbruening reviewed Jul 11, 2017

View reviewed changes

s-kanev added 4 commits July 10, 2017 18:00

Really fix VC++ warning

a9855f4

Style

dbd4a32

Add raw2trace test with offline.burst_threads

8023236

Fix another VC++ 32b warning

2956a15

Merge branch 'master' into i2511-raw2tracelib

6afe3ab

s-kanev added 2 commits July 14, 2017 10:44

Add ptrace-based test checking that raw2trace doesn't open files.

5c5b71a

Formatting (tweaking clang-format to match vera++ checks)

d922e3f

Don't build test on Windows (ptrace)

fa1d5c4

derekbruening approved these changes Jul 16, 2017

View reviewed changes

s-kanev and others added 2 commits July 17, 2017 09:40

Formatting, disable test on MacOS, remove unused template.

db01df2

Merge branch 'master' into i2511-raw2tracelib

8bff965

Merge branch 'master' into i2511-raw2tracelib

ea948ab

derekbruening merged commit 66fa2c7 into DynamoRIO:master Jul 18, 2017

s-kanev added a commit to s-kanev/dynamorio that referenced this pull request Jul 18, 2017

Fix bad header guard from DynamoRIO#2512

a244359

derekbruening pushed a commit that referenced this pull request Jul 18, 2017

i#2511 isolate raw2trace: Fix bad header guard from #2512 (#2545)

841d459

Fixes a header guard error from 66fa2c7.

i#2511: isolate raw2trace #2512

i#2511: isolate raw2trace #2512

Conversation

s-kanev commented Jul 7, 2017

s-kanev commented Jul 7, 2017

derekbruening commented Jul 10, 2017

fhahn commented Jul 10, 2017

fhahn commented Jul 10, 2017

derekbruening left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-kanev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbruening left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-kanev commented Jul 11, 2017 • edited Loading

derekbruening commented Jul 12, 2017 • edited Loading

derekbruening commented Jul 12, 2017

s-kanev commented Jul 12, 2017

s-kanev commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-kanev commented Jul 18, 2017

derekbruening commented Jul 18, 2017

s-kanev commented Jul 11, 2017 •

edited

Loading

derekbruening commented Jul 12, 2017 •

edited

Loading