i#2006 generalize drcachesim: add default analysis tool

Commit log for first patchset:
---------------
i#2006 generalize drcachesim: add default analysis tool

Add default analyzsis tool which simply counts trace entries.
---------------

[bruening, 2016-09-18 05:09:26]

> Add default analyzsis tool which simply counts trace entries.

Spelling error

[bruening, 2016-09-18 15:35:00]

Hmm, I think a design doc will be a better vehicle for discussion.  My comments
are not about the code but about the design, b/c I don't understand how the
proposal here fits with the tools that want to run the cache simulator.  Is the
simulator also going to create a tool instance and call process() on it?  If so,
shouldn't the code that does that be shared with simulator_t?

Maybe you could point at a description of how the different types of tools are
going to operate and which parts of them use subclassing vs an event model or
whatnot.

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/CMakeLists...
File clients/drcachesim/CMakeLists.txt (right):

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/CMakeLists...
clients/drcachesim/CMakeLists.txt:58: simulator/simulator.cpp
logically this seems better earlier, why move it down here underneath its child
class?

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/analyzer.cpp
File clients/drcachesim/analyzer.cpp (right):

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/analyzer.c...
clients/drcachesim/analyzer.cpp:45: if (op_ipc_name.get_value().empty()) {
This is in the constructor now and generalized

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/analyzer.c...
clients/drcachesim/analyzer.cpp:50: ipc_iter =
ipc_reader_t(op_ipc_name.get_value().c_str());
This is in the constructor now and generalized

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/analyzer.c...
clients/drcachesim/analyzer.cpp:67: if (!ipc_iter.init()) {
This is now start_reading() to share code

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/analyzer.c...
clients/drcachesim/analyzer.cpp:73: if (!tool->process(memref)) {
So for a non-simulator tool, the idea is that this base class run() supplies the
loop and a tool is not a subclass of analyzer_t but instead a separate object
that supplies the loop body.

But what will it look like for a simulator tool, i.e., a tool that wants to run
the simulator and take actions on cache misses?  It seems best to have a similar
model for both, if possible.  Maybe a design doc to sketch it out is easiest.

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/analyzer.h
File clients/drcachesim/analyzer.h (right):

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/analyzer.h...
clients/drcachesim/analyzer.h:47: virtual bool init();
Unfortunately we had simultaneous work and have conflicts here: to get the ipc
vs file reader setup code automatically shared I moved it to the constructor and
got rid of init()

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/analyzer.h...
clients/drcachesim/analyzer.h:50: virtual bool print_stats();
The rest of the diff shows this to be the end result of the tool, so this name
doesn't seem suitable to me: "stats" are debug counters.  This routine here is
meant to be the end result of the analysis, something much more general and
broad than debug counters.  Plus on Windows we might not "print" it.  Something
like "display_results"?

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/analyzer.h...
clients/drcachesim/analyzer.h:53: analysis_tool_t
*create_analysis_tool(std::string tool_name);
Maybe better to have a design doc?  So the model here is like a DR client: you
pass in a pointer to the library to load?  No, this is just a name, and
analysis_tool_t is compiled in -- so how are multiple tools supported?  Is the
name just decoration?

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/common/opt...
File clients/drcachesim/common/options.cpp (right):

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/common/opt...
clients/drcachesim/common/options.cpp:139: "Supported analysis tools: "".");
There should be a FIXME or sthg b/c an empty string is not a presentable result

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/launcher.cpp
File clients/drcachesim/launcher.cpp (right):

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/launcher.c...
clients/drcachesim/launcher.cpp:231: else if (op_simulator_type.get_value() ==
ANALYZER)
To me this name is confusing: analyzer_t is the base class of simulator_t, so
why would asking for ANALYZER now suddenly mean a peer of simulator?

Also, what about tools that want a cache or tlb simulation in addition to their
own analysis?

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/tools/anal...
File clients/drcachesim/tools/analysis_stats.h (right):

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/tools/anal...
clients/drcachesim/tools/analysis_stats.h:43: class analysis_stats_t
See earlier comment about "stats" not seeming like the right name for the
results of analysis.

I guess I'm not convinced a separate class for holding some counts is a
fundamental part of every tool?

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/tools/anal...
File clients/drcachesim/tools/analysis_tool.cpp (right):

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/tools/anal...
clients/drcachesim/tools/analysis_tool.cpp:56: // The basic trace stats
counting.
Why would a specific tool doing some counting be the base class?  The base class
should be prob be general and abstract, and a specific tool would be a subclass,
right?  Or are you saying that every single tool also wants to count types?

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/tools/anal...
clients/drcachesim/tools/analysis_tool.cpp:57: stats->process(memref);
I would say, eliminate the stats class and have the work of the tool (for this
tool, counting types) be here.

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/tools/anal...
File clients/drcachesim/tools/analysis_tool.h (right):

https://codereview.appspot.com/309410043/diff/1/clients/drcachesim/tools/anal...
clients/drcachesim/tools/analysis_tool.h:33: /* analysis_tool: represent a
memory trace analysis tool.
See earlier comment about analyzer_t being a base class of simulator_t causing
confusion about this vs the simulators vs tools on top of the simulators