i#2006 generalize drcachesim: add analysis_tool_t

Commit log for first patchset:
---------------
i#2006 generalize drcachesim: add analysis_tool_t

Adds analysis_tool_t as the base analysis tool class.

Updates simulator_t to inherit from analysis_tool_t.

Updates analyzer_t as the driver to create analysis tools and
feed trace entries to those analysis tools.
---------------

[bruening, 2016-09-26 17:39:25]

Contains a couple of bugs from moving the iterator.  The general design is good
and matches the resolution of earlier discussion.  Maybe worth another quick
look for the bug fixes.

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/analysis_t...
File clients/drcachesim/analysis_tool.h (right):

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/analysis_t...
clients/drcachesim/analysis_tool.h:40: #include "reader/reader.h"
This can be removed

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/analysis_t...
clients/drcachesim/analysis_tool.h:48: virtual bool process(const memref_t
&memref) = 0;
I would prefer a more specific name like process_memref()

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/analysis_t...
clients/drcachesim/analysis_tool.h:49: virtual bool print_stats() = 0;
Probably this should be more general: an exit event?  Or is the destructor
supposed to be that?  Should the destructor do results printing?

If this stays as a print_* I think print_results is better than print_stats.

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

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/analyzer.c...
clients/drcachesim/analyzer.cpp:70: delete trace_end;
corner case bug: if !success some of these might not be created -- best to
initialize to null in that case to avoid corruption here.

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/analyzer.c...
clients/drcachesim/analyzer.cpp:119: /* FIXME i#2006: add multiple tools support
*/
Hmm, as discussed, it seems a better design to have a single top-level tool who
then goes and loads whatever tool libraries it needs (simulator, callstacks,
etc.), rather than to have multiple top-level tools.  At least add a comment
about this.

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

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/analyzer.h...
clients/drcachesim/analyzer.h:46: // be queried via operator!.
This comment should probably be in analysis_tool_t as well

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

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/launcher.c...
clients/drcachesim/launcher.cpp:41: # include <windows.h>
Best to be LEAN_AND_MEAN.  Presumably this was coming from dr_api.h before --
are there really things in the code here that need windows.h?  If it's
analyzer.h that needs it (you inserted it above dr_api.h) then analyzer.h should
include it -- but that would be surprising too.  Who needs windows.h here?!?

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/launcher.c...
clients/drcachesim/launcher.cpp:45: # include <unistd.h>
Similar questions here

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
File clients/drcachesim/simulator/cache_simulator.cpp (right):

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
clients/drcachesim/simulator/cache_simulator.cpp:131: int last_core = 0;
bug: these no longer work properly as cached values -- they have to be
persistent across iterations and thus should be member variables

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
File clients/drcachesim/simulator/tlb_simulator.cpp (right):

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
clients/drcachesim/simulator/tlb_simulator.cpp:118: int last_core = 0;
Same bug.

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
clients/drcachesim/simulator/tlb_simulator.cpp:129: // both warmup and simulated
references are simulated
May as well fix the style violation: cap + punct

[zhaoqin, 2016-09-26 20:06:50]

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/analysis_t...
File clients/drcachesim/analysis_tool.h (right):

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/analysis_t...
clients/drcachesim/analysis_tool.h:40: #include "reader/reader.h"
On 2016/09/26 17:39:25, bruening wrote:
> This can be removed

Done.

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/analysis_t...
clients/drcachesim/analysis_tool.h:48: virtual bool process(const memref_t
&memref) = 0;
On 2016/09/26 17:39:25, bruening wrote:
> I would prefer a more specific name like process_memref()

Done.

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/analysis_t...
clients/drcachesim/analysis_tool.h:49: virtual bool print_stats() = 0;
On 2016/09/26 17:39:25, bruening wrote:
> Probably this should be more general: an exit event?  Or is the destructor
> supposed to be that?  Should the destructor do results printing?
> 
> If this stays as a print_* I think print_results is better than print_stats.

I am not sure if we want to move it to the destructor because some tool may want
to print the stats in the middle of execution.
change the name.

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

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/analyzer.c...
clients/drcachesim/analyzer.cpp:70: delete trace_end;
On 2016/09/26 17:39:25, bruening wrote:
> corner case bug: if !success some of these might not be created -- best to
> initialize to null in that case to avoid corruption here.

Done.

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/analyzer.c...
clients/drcachesim/analyzer.cpp:119: /* FIXME i#2006: add multiple tools support
*/
On 2016/09/26 17:39:25, bruening wrote:
> Hmm, as discussed, it seems a better design to have a single top-level tool
who
> then goes and loads whatever tool libraries it needs (simulator, callstacks,
> etc.), rather than to have multiple top-level tools.  At least add a comment
> about this.

Done.
Unlike DR, I feel it is much easier to have multiple top tools to run at the
same time without interfering with each other, since they do not manipulate the
same data struct like DR does.
So I think it is easier to have multiple tools, just like multiple analysis
clients of DR.

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

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/analyzer.h...
clients/drcachesim/analyzer.h:46: // be queried via operator!.
On 2016/09/26 17:39:25, bruening wrote:
> This comment should probably be in analysis_tool_t as well

I think it is both, since we still check the analyzer.

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

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/launcher.c...
clients/drcachesim/launcher.cpp:41: # include <windows.h>
On 2016/09/26 17:39:25, bruening wrote:
> Best to be LEAN_AND_MEAN.  Presumably this was coming from dr_api.h before --
> are there really things in the code here that need windows.h?  If it's
> analyzer.h that needs it (you inserted it above dr_api.h) then analyzer.h
should
> include it -- but that would be surprising too.  Who needs windows.h here?!?

for FormatMessage in configure_applications.
Before we get it for free because the named_pipe.h from ipc_tracer.h included
it.

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/launcher.c...
clients/drcachesim/launcher.cpp:45: # include <unistd.h>
On 2016/09/26 17:39:25, bruening wrote:
> Similar questions here

The fork.

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
File clients/drcachesim/simulator/cache_simulator.cpp (right):

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
clients/drcachesim/simulator/cache_simulator.cpp:131: int last_core = 0;
On 2016/09/26 17:39:25, bruening wrote:
> bug: these no longer work properly as cached values -- they have to be
> persistent across iterations and thus should be member variables

Done.

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
File clients/drcachesim/simulator/tlb_simulator.cpp (right):

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
clients/drcachesim/simulator/tlb_simulator.cpp:118: int last_core = 0;
On 2016/09/26 17:39:25, bruening wrote:
> Same bug.

Done.

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
clients/drcachesim/simulator/tlb_simulator.cpp:129: // both warmup and simulated
references are simulated
On 2016/09/26 17:39:25, bruening wrote:
> May as well fix the style violation: cap + punct

Done.

[zhaoqin, 2016-09-26 20:08:08]

Commit log for latest patchset:
---------------
i#2006 generalize drcachesim: add analysis_tool_t

Adds analysis_tool_t as the base analysis tool class.

Updates simulator_t to inherit from analysis_tool_t.

Updates analyzer_t as the driver to create analysis tools and
feed trace entries to those analysis tools.

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

[bruening, 2016-09-26 21:06:43]

Looks like the caching bug is still there.

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
File clients/drcachesim/simulator/cache_simulator.cpp (right):

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
clients/drcachesim/simulator/cache_simulator.cpp:131: int last_core = 0;
On 2016/09/26 20:06:50, zhaoqin wrote:
> On 2016/09/26 17:39:25, bruening wrote:
> > bug: these no longer work properly as cached values -- they have to be
> > persistent across iterations and thus should be member variables
> 
> Done.

But last_thread is still a local var so the caching still doesn't work.

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
File clients/drcachesim/simulator/tlb_simulator.cpp (right):

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
clients/drcachesim/simulator/tlb_simulator.cpp:118: int last_core = 0;
On 2016/09/26 20:06:50, zhaoqin wrote:
> On 2016/09/26 17:39:25, bruening wrote:
> > Same bug.
> 
> Done.

Again, last_thread can't be a local.

https://codereview.appspot.com/309530043/diff/20001/clients/drcachesim/analyz...
File clients/drcachesim/analyzer.cpp (right):

https://codereview.appspot.com/309530043/diff/20001/clients/drcachesim/analyz...
clients/drcachesim/analyzer.cpp:69: if (trace_iter != NULL)
BTW, I don't think this is necessary: I think it's guaranteed to be a no-op,
just like free(NULL)

https://codereview.appspot.com/309530043/diff/20001/clients/drcachesim/analyz...
clients/drcachesim/analyzer.cpp:71: if (trace_end != NULL)
Ditto

https://codereview.appspot.com/309530043/diff/20001/clients/drcachesim/analyz...
clients/drcachesim/analyzer.cpp:121: /* FIXME i#2006: add multiple tools
support. */
tool

https://codereview.appspot.com/309530043/diff/20001/clients/drcachesim/analyz...
clients/drcachesim/analyzer.cpp:123: * multi-components tools.
component

https://codereview.appspot.com/309530043/diff/20001/clients/drcachesim/launch...
File clients/drcachesim/launcher.cpp (right):

https://codereview.appspot.com/309530043/diff/20001/clients/drcachesim/launch...
clients/drcachesim/launcher.cpp:41: # include <windows.h>
OK, but as requested please set WIN32_LEAN_AND_MEAN

[zhaoqin, 2016-09-26 21:16:38]

Commit log for latest patchset:
---------------
i#2006 generalize drcachesim: add analysis_tool_t

Adds analysis_tool_t as the base analysis tool class.

Updates simulator_t to inherit from analysis_tool_t.

Updates analyzer_t as the driver to create analysis tools and
feed trace entries to those analysis tools.

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

[zhaoqin, 2016-09-26 21:17:29]

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
File clients/drcachesim/simulator/cache_simulator.cpp (right):

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
clients/drcachesim/simulator/cache_simulator.cpp:131: int last_core = 0;
On 2016/09/26 21:06:43, bruening wrote:
> On 2016/09/26 20:06:50, zhaoqin wrote:
> > On 2016/09/26 17:39:25, bruening wrote:
> > > bug: these no longer work properly as cached values -- they have to be
> > > persistent across iterations and thus should be member variables
> > 
> > Done.
> 
> But last_thread is still a local var so the caching still doesn't work.

Done.

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
File clients/drcachesim/simulator/tlb_simulator.cpp (right):

https://codereview.appspot.com/309530043/diff/1/clients/drcachesim/simulator/...
clients/drcachesim/simulator/tlb_simulator.cpp:118: int last_core = 0;
On 2016/09/26 21:06:43, bruening wrote:
> On 2016/09/26 20:06:50, zhaoqin wrote:
> > On 2016/09/26 17:39:25, bruening wrote:
> > > Same bug.
> > 
> > Done.
> 
> Again, last_thread can't be a local.

Done.

https://codereview.appspot.com/309530043/diff/20001/clients/drcachesim/analyz...
File clients/drcachesim/analyzer.cpp (right):

https://codereview.appspot.com/309530043/diff/20001/clients/drcachesim/analyz...
clients/drcachesim/analyzer.cpp:69: if (trace_iter != NULL)
On 2016/09/26 21:06:43, bruening wrote:
> BTW, I don't think this is necessary: I think it's guaranteed to be a no-op,
> just like free(NULL)

Done.

https://codereview.appspot.com/309530043/diff/20001/clients/drcachesim/analyz...
clients/drcachesim/analyzer.cpp:71: if (trace_end != NULL)
On 2016/09/26 21:06:43, bruening wrote:
> Ditto

Done.

https://codereview.appspot.com/309530043/diff/20001/clients/drcachesim/analyz...
clients/drcachesim/analyzer.cpp:121: /* FIXME i#2006: add multiple tools
support. */
On 2016/09/26 21:06:43, bruening wrote:
> tool

Done.

https://codereview.appspot.com/309530043/diff/20001/clients/drcachesim/analyz...
clients/drcachesim/analyzer.cpp:123: * multi-components tools.
On 2016/09/26 21:06:43, bruening wrote:
> component

Done.

https://codereview.appspot.com/309530043/diff/20001/clients/drcachesim/launch...
File clients/drcachesim/launcher.cpp (right):

https://codereview.appspot.com/309530043/diff/20001/clients/drcachesim/launch...
clients/drcachesim/launcher.cpp:41: # include <windows.h>
On 2016/09/26 21:06:43, bruening wrote:
> OK, but as requested please set WIN32_LEAN_AND_MEAN

Done.

[bruening, 2016-09-26 21:49:27]

LGTM

[zhaoqin, 2016-09-27 03:39:18]

Committed as
https://github.com/DynamoRIO/dynamorio/commit/2038b647c84815b9839ee0c1d578870...

Final commit log: 
---------------
i#2006 generalize drcachesim: add analysis_tool_t

Adds analysis_tool_t as the base analysis tool class.

Updates simulator_t to inherit from analysis_tool_t.

Updates analyzer_t as the driver to create analysis tools and
feed trace entries to those analysis tools.

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