i#2006 generalize drcachesim: support customized file operation

Commit log for first patchset:
---------------
i#2006 generalize drcachesim: support customized file operation

Adds drmemtrace_replace_file_ops for customized file operations.

Updates burst_static test.

Updates drcachesim docs.

Updates DynamoRIOConfig.cmake.in for using drmemtrace.h

Updates drcachesim.dox.in about offline analysis.
---------------

[zhaoqin, 2016-11-01 00:24:28]

Commit log for latest patchset:
---------------
i#2006 generalize drcachesim: support customized file operation

Adds drmemtrace_replace_file_ops for customized file operations.

Updates burst_static test.

Updates drcachesim docs.

Updates DynamoRIOConfig.cmake.in for using drmemtrace.h

Updates drcachesim.dox.in about offline analysis.

Review-URL: https://codereview.appspot.com/310660043
---------------

[bruening, 2016-11-01 20:31:38]

Not done yet (it's long...) but sending comments so far

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/CMakeL...
File clients/drcachesim/CMakeLists.txt (right):

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/CMakeL...
clients/drcachesim/CMakeLists.txt:247:
use_DynamoRIO_extension(tool.drcacheoff.burst_static drmemtrace_static)
This is very confusing vs line 245: a comment explaining this is for the header
only would help.

Why does this test need this header if it's not using custom i/o?

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
File clients/drcachesim/drcachesim.dox.in (right):

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:38: \p drcachesim is a DynamoRIO client
tool that collects memory access traces
There are 3 "tool" references here and it's a little confusing.

I would do s/tool// here

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:41: specific caching devices, e.g., CPU
caches and TLBs.  The tool
s/tool/tracer and simulator/

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:42: supports multiple processes each with
multiple threads.
s/supports/support/

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:57: online or offline analysis over the
collected traces.
s/analysis/analyses/

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:62: The collected trace could be either fed
to an online simulator or dumped
traces

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:63: for offline analysis.  The analyzer is
designed to be extensible,
This seems jumbled up with a weird order: "we collect traces; there is a default
simulator, which collects traces; collected traces could go to an online
simulator [as though you didn't just mention a simulator]; the analyzer [what is
that???] lets the user implement simulators".  I think this all needs to be
rewritten in a more logical sequence.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:64: allowing users to easily implement
simulator for different devices,
missing article

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:74: bin64/drrun -t drcachesim -ipc_name
pipe_name -- /path/to/target/app <args> <for> <app>
This is deliberately not listed to keep things simple: it's not needed, so why
complicated things?

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:79: via pipe \p pipe_name.  Any
Ditto

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tests/...
File clients/drcachesim/tests/burst_static.cpp (right):

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tests/...
clients/drcachesim/tests/burst_static.cpp:70: local_write_file(file_t file,
const void* data, size_t size)
Hmm, should a separate test be used instead of putting this into this basic
burst test?

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tests/...
clients/drcachesim/tests/burst_static.cpp:70: local_write_file(file_t file,
const void* data, size_t size)
Style violation: void *

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tests/...
clients/drcachesim/tests/burst_static.cpp:73: dr_fprintf(STDERR, "%d: write %d
bytes @ " PFX " to file %d\n",
Best to follow the existing code and use cerr

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tests/...
clients/drcachesim/tests/burst_static.cpp:75: if (count++ == 1) {
Worth a comment that you assume a single-threaded app so no races

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tests/...
clients/drcachesim/tests/burst_static.cpp:76: dr_fprintf(STDERR, "restore the
write file function\n");
Ditto

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
File clients/drcachesim/tracer/drmemtrace.h (right):

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:35: /* drmemtrace: inserts
instrumentation to write to a trace stream.
This should describe this header, not general drmemtrace.  This looks copied
from instru.h.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:54: } drmemtrace_status_t;
Hmm, so this is used just for the one function: do we anticipate adding more to
this file?

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:59: typedef file_t
(*drmemtrace_open_file_func_t)(const char* fname, uint mode_flag);
style violation: *

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:59: typedef file_t
(*drmemtrace_open_file_func_t)(const char* fname, uint mode_flag);
This flag needs to be described.  What kind of values does it take?  Imagine a
user on Windows, or different flavors of UNIX: what is this flag?

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:63: typedef ssize_t
(*drmemtrace_read_file_func_t)(file_t file, void* data, size_t count);
ditto

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:67: typedef ssize_t
(*drmemtrace_write_file_func_t)(file_t file, const void* data, size_t count);
style violation: line too long

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:67: typedef ssize_t
(*drmemtrace_write_file_func_t)(file_t file, const void* data, size_t count);
ditto

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:75: typedef bool
(*drmemtrace_create_dir_func_t)(const char* dir);
ditto

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:82: * \note The caller is responsible for
the transparence and isolation of using
transparency

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:83: * those functions.
Best to explain further: ", which will be called in the middle of arbitrary
application code."

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
File clients/drcachesim/tracer/instru_offline.cpp (right):

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/instru_offline.cpp:72: write_file_func(modfile, buf,
strlen(buf));
Best to check return value even if only an assert

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
File clients/drcachesim/tracer/tracer.cpp (right):

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:125: } file_ops_func_t;
Hmm, a typedef for a struct with only one instance?  Can just declare it w/o
typedef.

[bruening, 2016-11-02 16:39:18]

Rest of comments

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
File clients/drcachesim/tracer/tracer.cpp (right):

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:667: for (i = 0; i < 10000; i++) {
This needs a comment explaining why we have to loop, b/c normally this drx
routine ensures a unique file on its own

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:678: if (i == 10000) {
Used more than once so best to use a named constant

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:756: for (i = 0; i < 10000; i++) {
Ditto on comment

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:768: if (i == 10000)
Ditto on named constant

https://codereview.appspot.com/310660043/diff/20001/make/DynamoRIOConfig.cmak...
File make/DynamoRIOConfig.cmake.in (right):

https://codereview.appspot.com/310660043/diff/20001/make/DynamoRIOConfig.cmak...
make/DynamoRIOConfig.cmake.in:1038: set(DynamoRIO_EXT_drmemtrace_static_NOLIB
ON)
Hmm, I thought these were localized, not centralized here?

[zhaoqin, 2016-11-09 21:38:42]

Commit log for latest patchset:
---------------
i#2006 generalize drcachesim: support customized file operation

Adds drmemtrace_replace_file_ops for customized file operations.

Adds burst_replace test.

Updates drcachesim docs.

Updates DynamoRIOConfig.cmake.in for using drmemtrace.h

Updates drcachesim.dox.in about offline analysis.

Review-URL: https://codereview.appspot.com/310660043
---------------

[zhaoqin, 2016-11-09 21:39:20]

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/CMakeL...
File clients/drcachesim/CMakeLists.txt (right):

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/CMakeL...
clients/drcachesim/CMakeLists.txt:247:
use_DynamoRIO_extension(tool.drcacheoff.burst_static drmemtrace_static)
On 2016/11/01 20:31:36, bruening wrote:
> This is very confusing vs line 245: a comment explaining this is for the
header
> only would help.
Done
> 
> Why does this test need this header if it's not using custom i/o?

I used it for custom i/o, now move it to a separate test.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
File clients/drcachesim/drcachesim.dox.in (right):

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:38: \p drcachesim is a DynamoRIO client
tool that collects memory access traces
On 2016/11/01 20:31:36, bruening wrote:
> There are 3 "tool" references here and it's a little confusing.
> 
> I would do s/tool// here

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:41: specific caching devices, e.g., CPU
caches and TLBs.  The tool
On 2016/11/01 20:31:36, bruening wrote:
> s/tool/tracer and simulator/

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:42: supports multiple processes each with
multiple threads.
On 2016/11/01 20:31:36, bruening wrote:
> s/supports/support/

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:57: online or offline analysis over the
collected traces.
On 2016/11/01 20:31:37, bruening wrote:
> s/analysis/analyses/

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:62: The collected trace could be either fed
to an online simulator or dumped
On 2016/11/01 20:31:36, bruening wrote:
> traces

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:63: for offline analysis.  The analyzer is
designed to be extensible,
On 2016/11/01 20:31:37, bruening wrote:
> This seems jumbled up with a weird order: "we collect traces; there is a
default
> simulator, which collects traces; collected traces could go to an online
> simulator [as though you didn't just mention a simulator]; the analyzer [what
is
> that???] lets the user implement simulators".  I think this all needs to be
> rewritten in a more logical sequence.

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:64: allowing users to easily implement
simulator for different devices,
On 2016/11/01 20:31:36, bruening wrote:
> missing article

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:74: bin64/drrun -t drcachesim -ipc_name
pipe_name -- /path/to/target/app <args> <for> <app>
On 2016/11/01 20:31:36, bruening wrote:
> This is deliberately not listed to keep things simple: it's not needed, so why
> complicated things?

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:79: via pipe \p pipe_name.  Any
On 2016/11/01 20:31:36, bruening wrote:
> Ditto

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tests/...
File clients/drcachesim/tests/burst_static.cpp (right):

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tests/...
clients/drcachesim/tests/burst_static.cpp:70: local_write_file(file_t file,
const void* data, size_t size)
On 2016/11/01 20:31:37, bruening wrote:
> Style violation: void *

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tests/...
clients/drcachesim/tests/burst_static.cpp:70: local_write_file(file_t file,
const void* data, size_t size)
On 2016/11/01 20:31:37, bruening wrote:
> Hmm, should a separate test be used instead of putting this into this basic
> burst test?

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tests/...
clients/drcachesim/tests/burst_static.cpp:73: dr_fprintf(STDERR, "%d: write %d
bytes @ " PFX " to file %d\n",
On 2016/11/01 20:31:37, bruening wrote:
> Best to follow the existing code and use cerr

This is to be run in DR's context, so we should use DR functions for
transparency.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tests/...
clients/drcachesim/tests/burst_static.cpp:75: if (count++ == 1) {
On 2016/11/01 20:31:37, bruening wrote:
> Worth a comment that you assume a single-threaded app so no races

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tests/...
clients/drcachesim/tests/burst_static.cpp:76: dr_fprintf(STDERR, "restore the
write file function\n");
On 2016/11/01 20:31:37, bruening wrote:
> Ditto

xref comment above about within DR context.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
File clients/drcachesim/tracer/drmemtrace.h (right):

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:35: /* drmemtrace: inserts
instrumentation to write to a trace stream.
On 2016/11/01 20:31:37, bruening wrote:
> This should describe this header, not general drmemtrace.  This looks copied
> from instru.h.

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:54: } drmemtrace_status_t;
On 2016/11/01 20:31:37, bruening wrote:
> Hmm, so this is used just for the one function: do we anticipate adding more
to
> this file?

I am not sure, but I would like to keep it just in case we need more interaction
between app and the drmemtrace.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:59: typedef file_t
(*drmemtrace_open_file_func_t)(const char* fname, uint mode_flag);
On 2016/11/01 20:31:37, bruening wrote:
> style violation: *

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:59: typedef file_t
(*drmemtrace_open_file_func_t)(const char* fname, uint mode_flag);
On 2016/11/01 20:31:37, bruening wrote:
> This flag needs to be described.  What kind of values does it take?  Imagine a
> user on Windows, or different flavors of UNIX: what is this flag?

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:63: typedef ssize_t
(*drmemtrace_read_file_func_t)(file_t file, void* data, size_t count);
On 2016/11/01 20:31:37, bruening wrote:
> ditto

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:67: typedef ssize_t
(*drmemtrace_write_file_func_t)(file_t file, const void* data, size_t count);
On 2016/11/01 20:31:37, bruening wrote:
> ditto

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:67: typedef ssize_t
(*drmemtrace_write_file_func_t)(file_t file, const void* data, size_t count);
On 2016/11/01 20:31:37, bruening wrote:
> style violation: line too long

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:75: typedef bool
(*drmemtrace_create_dir_func_t)(const char* dir);
On 2016/11/01 20:31:37, bruening wrote:
> ditto

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:82: * \note The caller is responsible for
the transparence and isolation of using
On 2016/11/01 20:31:37, bruening wrote:
> transparency

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:83: * those functions.
On 2016/11/01 20:31:37, bruening wrote:
> Best to explain further: ", which will be called in the middle of arbitrary
> application code."

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
File clients/drcachesim/tracer/instru_offline.cpp (right):

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/instru_offline.cpp:72: write_file_func(modfile, buf,
strlen(buf));
On 2016/11/01 20:31:37, bruening wrote:
> Best to check return value even if only an assert

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
File clients/drcachesim/tracer/tracer.cpp (right):

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:125: } file_ops_func_t;
On 2016/11/01 20:31:37, bruening wrote:
> Hmm, a typedef for a struct with only one instance?  Can just declare it w/o
> typedef.

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:667: for (i = 0; i < 10000; i++) {
On 2016/11/02 16:39:18, bruening wrote:
> This needs a comment explaining why we have to loop, b/c normally this drx
> routine ensures a unique file on its own

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:678: if (i == 10000) {
On 2016/11/02 16:39:18, bruening wrote:
> Used more than once so best to use a named constant

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:756: for (i = 0; i < 10000; i++) {
On 2016/11/02 16:39:18, bruening wrote:
> Ditto on comment

Done.

https://codereview.appspot.com/310660043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:768: if (i == 10000)
On 2016/11/02 16:39:18, bruening wrote:
> Ditto on named constant

Done.

https://codereview.appspot.com/310660043/diff/20001/make/DynamoRIOConfig.cmak...
File make/DynamoRIOConfig.cmake.in (right):

https://codereview.appspot.com/310660043/diff/20001/make/DynamoRIOConfig.cmak...
make/DynamoRIOConfig.cmake.in:1038: set(DynamoRIO_EXT_drmemtrace_static_NOLIB
ON)
On 2016/11/02 16:39:18, bruening wrote:
> Hmm, I thought these were localized, not centralized here?

for other app to use, shouldn't it be centralized?

[zhaoqin, 2016-11-09 21:50:25]

Commit log for latest patchset:
---------------
i#2006 generalize drcachesim: support customized file operation

Adds drmemtrace_replace_file_ops for customized file operations.

Adds burst_replace test.

Updates drcachesim docs.

Updates DynamoRIOConfig.cmake.in for using drmemtrace.h

Updates drcachesim.dox.in about offline analysis.

Review-URL: https://codereview.appspot.com/310660043
---------------

[zhaoqin, 2016-11-11 21:53:53]

On 2016/11/09 21:50:25, zhaoqin wrote:
> Commit log for latest patchset:
> ---------------
> i#2006 generalize drcachesim: support customized file operation
> 
> Adds drmemtrace_replace_file_ops for customized file operations.
> 
> Adds burst_replace test.
> 
> Updates drcachesim docs.
> 
> Updates DynamoRIOConfig.cmake.in for using drmemtrace.h
> 
> Updates drcachesim.dox.in about offline analysis.
> 
> Review-URL: https://codereview.appspot.com/310660043
> ---------------

Ping, just in case you missed this one.

[bruening, 2016-11-14 19:04:46]

Has a bug: will likely fail to build on Windows.  Rest of comments should be
fixable without another look so LGTM w/ comments if bug is fixed.

https://codereview.appspot.com/310660043/diff/20001/make/DynamoRIOConfig.cmak...
File make/DynamoRIOConfig.cmake.in (right):

https://codereview.appspot.com/310660043/diff/20001/make/DynamoRIOConfig.cmak...
make/DynamoRIOConfig.cmake.in:1038: set(DynamoRIO_EXT_drmemtrace_static_NOLIB
ON)
On 2016/11/09 21:39:20, zhaoqin wrote:
> On 2016/11/02 16:39:18, bruening wrote:
> > Hmm, I thought these were localized, not centralized here?
> 
> for other app to use, shouldn't it be centralized?

The point is that it is less maintainable and extensible to have to edit here:
much easier for a 3rd party contribute an extension when they don't have to edit
the DR repo code.  We set up all this infrastructure for a 3rd party extension
package to work without requiring centralized changes inside DR, yet that breaks
down here.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/CMakeL...
File clients/drcachesim/CMakeLists.txt (right):

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/CMakeL...
clients/drcachesim/CMakeLists.txt:251:
add_win32_flags(tool.drcacheoff.burst_static)
bug: was this tested on Windows?  I would expect it to fail w/ this bug.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/drcach...
File clients/drcachesim/drcachesim.dox.in (right):

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:40: The default analyzer is a cache
simulator which simulates a set of
This is still disjointed: a different name is used in each sentence from the
prior one.  I would do:

s/analyzer/analysis tool/

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:41: specific caching devices, e.g., CPU
caches and TLBs.  The tracer and simulator
s/tracer/trace collector/

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:55: \p drcachesim consists of two
components: a tracer and a analyzer.
grammar: an

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:58: The analyzer consumes the traces
(online or offline), and performs
suggest: s/,//

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tests/...
File clients/drcachesim/tests/burst_replace.cpp (right):

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tests/...
clients/drcachesim/tests/burst_replace.cpp:78: /* Assuming it is a
single-threaded app, so we do not worry about
s/so//

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tests/...
clients/drcachesim/tests/burst_replace.cpp:100:
drmemtrace_replace_file_ops(NULL, NULL, local_write_file, NULL, NULL);
So there are no tests of the other parameters here not being NULL?

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
File clients/drcachesim/tracer/drmemtrace.h (right):

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:35: /* drmemtrace: APIs for applications
that is statically linked with drmemtrace
grammar: s/is/are/

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:63: /**
nit: blank line in between each exported function decl is the style used in all
our headers

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:64: * Function for file read.
What is the return value?

optional: it might be nice to have @param and @return comments in this header
for more formal and complete docs.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:67: /**
nit: blank line

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:68: * Function for file write.
What is the return value?

Any special behavior on signal interruption?

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:73: /**
nit: blank line

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:77: /**
nit: blank line

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
File clients/drcachesim/tracer/instru_offline.cpp (right):

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
clients/drcachesim/tracer/instru_offline.cpp:72: ssize_t written =
write_file_func(modfile, buf, strlen(buf));
Are you sure this compiles everywhere?  Some compiler in release build is going
to complain (Windows maybe) about an unused var.  Hmm, wait: DR_ASSERT is
defined for both release and debug builds identically?  Is that what we
intended?

[zhaoqin, 2016-11-15 16:29:07]

https://codereview.appspot.com/310660043/diff/20001/make/DynamoRIOConfig.cmak...
File make/DynamoRIOConfig.cmake.in (right):

https://codereview.appspot.com/310660043/diff/20001/make/DynamoRIOConfig.cmak...
make/DynamoRIOConfig.cmake.in:1038: set(DynamoRIO_EXT_drmemtrace_static_NOLIB
ON)
On 2016/11/14 19:04:45, bruening wrote:
> On 2016/11/09 21:39:20, zhaoqin wrote:
> > On 2016/11/02 16:39:18, bruening wrote:
> > > Hmm, I thought these were localized, not centralized here?
> > 
> > for other app to use, shouldn't it be centralized?
> 
> The point is that it is less maintainable and extensible to have to edit here:
> much easier for a 3rd party contribute an extension when they don't have to
edit
> the DR repo code.  We set up all this infrastructure for a 3rd party extension
> package to work without requiring centralized changes inside DR, yet that
breaks
> down here.

Acknowledged.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/CMakeL...
File clients/drcachesim/CMakeLists.txt (right):

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/CMakeL...
clients/drcachesim/CMakeLists.txt:251:
add_win32_flags(tool.drcacheoff.burst_static)
On 2016/11/14 19:04:45, bruening wrote:
> bug: was this tested on Windows?  I would expect it to fail w/ this bug.

Done.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/drcach...
File clients/drcachesim/drcachesim.dox.in (right):

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:40: The default analyzer is a cache
simulator which simulates a set of
On 2016/11/14 19:04:45, bruening wrote:
> This is still disjointed: a different name is used in each sentence from the
> prior one.  I would do:
> 
> s/analyzer/analysis tool/

Done.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:41: specific caching devices, e.g., CPU
caches and TLBs.  The tracer and simulator
On 2016/11/14 19:04:45, bruening wrote:
> s/tracer/trace collector/

Done.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:55: \p drcachesim consists of two
components: a tracer and a analyzer.
On 2016/11/14 19:04:45, bruening wrote:
> grammar: an

Done.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/drcach...
clients/drcachesim/drcachesim.dox.in:58: The analyzer consumes the traces
(online or offline), and performs
On 2016/11/14 19:04:45, bruening wrote:
> suggest: s/,//

Done.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tests/...
File clients/drcachesim/tests/burst_replace.cpp (right):

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tests/...
clients/drcachesim/tests/burst_replace.cpp:78: /* Assuming it is a
single-threaded app, so we do not worry about
On 2016/11/14 19:04:45, bruening wrote:
> s/so//

Done.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tests/...
clients/drcachesim/tests/burst_replace.cpp:100:
drmemtrace_replace_file_ops(NULL, NULL, local_write_file, NULL, NULL);
On 2016/11/14 19:04:45, bruening wrote:
> So there are no tests of the other parameters here not being NULL?

Done.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
File clients/drcachesim/tracer/drmemtrace.h (right):

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:35: /* drmemtrace: APIs for applications
that is statically linked with drmemtrace
On 2016/11/14 19:04:45, bruening wrote:
> grammar: s/is/are/

Done.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:63: /**
On 2016/11/14 19:04:45, bruening wrote:
> nit: blank line in between each exported function decl is the style used in
all
> our headers

Done.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:64: * Function for file read.
On 2016/11/14 19:04:46, bruening wrote:
> What is the return value?
> 
> optional: it might be nice to have @param and @return comments in this header
> for more formal and complete docs.

Done.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:67: /**
On 2016/11/14 19:04:46, bruening wrote:
> nit: blank line

Done.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:68: * Function for file write.
On 2016/11/14 19:04:45, bruening wrote:
> What is the return value?
> 
> Any special behavior on signal interruption?

Done.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:73: /**
On 2016/11/14 19:04:45, bruening wrote:
> nit: blank line

Done.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
clients/drcachesim/tracer/drmemtrace.h:77: /**
On 2016/11/14 19:04:46, bruening wrote:
> nit: blank line

Done.

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
File clients/drcachesim/tracer/instru_offline.cpp (right):

https://codereview.appspot.com/310660043/diff/60001/clients/drcachesim/tracer...
clients/drcachesim/tracer/instru_offline.cpp:72: ssize_t written =
write_file_func(modfile, buf, strlen(buf));
On 2016/11/14 19:04:46, bruening wrote:
> Are you sure this compiles everywhere?  Some compiler in release build is
going
> to complain (Windows maybe) about an unused var.  Hmm, wait: DR_ASSERT is
> defined for both release and debug builds identically?  Is that what we
> intended?

I think it has been there from the very beginning, I thought it was intended.

[zhaoqin, 2016-11-15 16:30:04]

Commit log for latest patchset:
---------------
i#2006 generalize drcachesim: support customized file operation

Adds drmemtrace_replace_file_ops for customized file operations.

Adds burst_replace test.

Updates drcachesim docs.

Updates DynamoRIOConfig.cmake.in for using drmemtrace.h

Updates drcachesim.dox.in about offline analysis.

Review-URL: https://codereview.appspot.com/310660043
---------------

[zhaoqin, 2016-11-15 17:41:28]

Commit log for latest patchset:
---------------
i#2006 generalize drcachesim: support customized file operation

Adds drmemtrace_replace_file_ops for customized file operations.

Adds burst_replace test.

Updates drcachesim docs.

Updates DynamoRIOConfig.cmake.in for using drmemtrace.h

Updates drcachesim.dox.in about offline analysis.

Review-URL: https://codereview.appspot.com/310660043
---------------

[zhaoqin, 2016-11-15 19:20:55]

Committed as
https://github.com/DynamoRIO/dynamorio/commit/4d7d5422aa8fb1bb2c6c971cd2d05ab...

Final commit log: 
---------------
i#2006 generalize drcachesim: support customized file operation

Adds drmemtrace_replace_file_ops for customized file operations.

Adds burst_replace test.

Updates drcachesim docs.

Updates DynamoRIOConfig.cmake.in for using drmemtrace.h

Updates drcachesim.dox.in about offline analysis.

Review-URL: https://codereview.appspot.com/310660043
---------------