i#1703 drcachesim: split reader from simulator

Commit log for first patchset:
---------------
i#1703 drcachesim: split reader from simulator

Split trace reader from simulator directory into separate reader
directory.
---------------

[zhaoqin, 2016-09-13 04:16:35]

Commit log for latest patchset:
---------------
i#1703 drcachesim: split reader from simulator

Split trace reader from simulator directory into separate reader
directory.

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

[bruening, 2016-09-13 19:13:17]

https://codereview.appspot.com/305470043/diff/20001/clients/drcachesim/reader...
File clients/drcachesim/reader/ipc_reader.cpp (right):

https://codereview.appspot.com/305470043/diff/20001/clients/drcachesim/reader...
clients/drcachesim/reader/ipc_reader.cpp:37: #include "../simulator/utils.h"
The reader should not depend on files in the simulator.  This should be in
common or sthg.

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

https://codereview.appspot.com/305470043/diff/20001/clients/drcachesim/reader...
clients/drcachesim/reader/launcher.cpp:59: #include
"../simulator/tlb_simulator.h"
The reader should not depend on the simulator.  Probably this launcher should be
in simulator/, or perhaps if the plan is to generalize it to be a front-end for
any *online* tool, it should be in its own launcher/ dir with comments about
generalizing.

[zhaoqin, 2016-09-14 01:48:20]

Commit log for latest patchset:
---------------
i#1703 drcachesim: split reader from simulator

Split trace reader from simulator directory into separate reader
directory.

Move launcher out of simulator direcory.

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

[zhaoqin, 2016-09-14 01:48:44]

https://codereview.appspot.com/305470043/diff/20001/clients/drcachesim/reader...
File clients/drcachesim/reader/ipc_reader.cpp (right):

https://codereview.appspot.com/305470043/diff/20001/clients/drcachesim/reader...
clients/drcachesim/reader/ipc_reader.cpp:37: #include "../simulator/utils.h"
On 2016/09/13 19:13:17, bruening wrote:
> The reader should not depend on files in the simulator.  This should be in
> common or sthg.

Done.

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

https://codereview.appspot.com/305470043/diff/20001/clients/drcachesim/reader...
clients/drcachesim/reader/launcher.cpp:59: #include
"../simulator/tlb_simulator.h"
On 2016/09/13 19:13:17, bruening wrote:
> The reader should not depend on the simulator.  Probably this launcher should
be
> in simulator/, or perhaps if the plan is to generalize it to be a front-end
for
> any *online* tool, it should be in its own launcher/ dir with comments about
> generalizing.

Done.
I moved launcher.cpp to drcachesim/ directory, it is kind of strange to have a
directory with one file.
Also, I am not thinking about making it for all "online" tools, but using it for
offline launcher by skipping the starting DR part instead.

[bruening, 2016-09-14 21:17:53]

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

https://codereview.appspot.com/305470043/diff/20001/clients/drcachesim/reader...
clients/drcachesim/reader/launcher.cpp:59: #include
"../simulator/tlb_simulator.h"
On 2016/09/14 01:48:44, zhaoqin wrote:
> On 2016/09/13 19:13:17, bruening wrote:
> > The reader should not depend on the simulator.  Probably this launcher
should
> be
> > in simulator/, or perhaps if the plan is to generalize it to be a front-end
> for
> > any *online* tool, it should be in its own launcher/ dir with comments about
> > generalizing.
> 
> Done.
> I moved launcher.cpp to drcachesim/ directory, it is kind of strange to have a
> directory with one file.
> Also, I am not thinking about making it for all "online" tools, but using it
for
> offline launcher by skipping the starting DR part instead.

Xref the decisions from the other CL where analyzer.* are being added: it seems
like launcher should go in the same place as analyzer.*, which is not in
simulator/, right?

[zhaoqin, 2016-09-14 21:36:02]

On 2016/09/14 21:17:53, bruening wrote:
>
https://codereview.appspot.com/305470043/diff/20001/clients/drcachesim/reader...
> File clients/drcachesim/reader/launcher.cpp (right):
> 
>
https://codereview.appspot.com/305470043/diff/20001/clients/drcachesim/reader...
> clients/drcachesim/reader/launcher.cpp:59: #include
> "../simulator/tlb_simulator.h"
> On 2016/09/14 01:48:44, zhaoqin wrote:
> > On 2016/09/13 19:13:17, bruening wrote:
> > > The reader should not depend on the simulator.  Probably this launcher
> should
> > be
> > > in simulator/, or perhaps if the plan is to generalize it to be a
front-end
> > for
> > > any *online* tool, it should be in its own launcher/ dir with comments
about
> > > generalizing.
> > 
> > Done.
> > I moved launcher.cpp to drcachesim/ directory, it is kind of strange to have
a
> > directory with one file.
> > Also, I am not thinking about making it for all "online" tools, but using it
> for
> > offline launcher by skipping the starting DR part instead.
> 
> Xref the decisions from the other CL where analyzer.* are being added: it
seems
> like launcher should go in the same place as analyzer.*, which is not in
> simulator/, right?

Yes, I would prefer the launcher.cpp and analyzer.* both in the top level.

[zhaoqin, 2016-09-15 15:12:20]

Commit log for latest patchset:
---------------
i#2006 generalize drcachesim: split reader from simulator

Split trace reader from simulator directory into separate reader
directory.

Move launcher out of simulator direcory.

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

[bruening, 2016-09-15 19:33:53]

Add the move of memref to the commit msg.

Please put back the deleted #includes (2 instances): relying on transitively
included content is not a good idea.

If that is fixed then LGTM

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/analyz...
File clients/drcachesim/analyzer.h (right):

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/analyz...
clients/drcachesim/analyzer.h:39: #include "./reader/ipc_reader.h"
why "./"?

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/common...
File clients/drcachesim/common/memref.h (right):

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/common...
clients/drcachesim/common/memref.h:40: #include "../common/trace_entry.h"
Local now

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/reader...
File clients/drcachesim/reader/ipc_reader.h (right):

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/reader...
clients/drcachesim/reader/ipc_reader.h:45: #include "../common/memref.h"
As we discussed offline: in some sense memref seems to belong in reader/ but
OTOH it is used everywhere after having been read, so perhaps in commmon/ is
fine.

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/simula...
File clients/drcachesim/simulator/caching_device.h (right):

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/simula...
clients/drcachesim/simulator/caching_device.h:57: virtual void request(const
memref_t &memref);
This uses memref right here, so it should include memref.h

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/simula...
File clients/drcachesim/simulator/tlb_simulator.cpp (right):

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/simula...
clients/drcachesim/simulator/tlb_simulator.cpp:56: ipc_iter =
ipc_reader_t(op_ipc_name.get_value().c_str());
This uses ipc_reader so it should include the header

[zhaoqin, 2016-09-15 20:10:46]

Commit log for latest patchset:
---------------
i#2006 generalize drcachesim: split reader from simulator

Split trace reader from simulator directory into separate reader
directory.

Move launcher out of simulator direcory.

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

[zhaoqin, 2016-09-15 20:11:50]

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/analyz...
File clients/drcachesim/analyzer.h (right):

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/analyz...
clients/drcachesim/analyzer.h:39: #include "./reader/ipc_reader.h"
On 2016/09/15 19:33:53, bruening wrote:
> why "./"?

Done.

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/common...
File clients/drcachesim/common/memref.h (right):

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/common...
clients/drcachesim/common/memref.h:40: #include "../common/trace_entry.h"
On 2016/09/15 19:33:53, bruening wrote:
> Local now

Done.

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/reader...
File clients/drcachesim/reader/ipc_reader.h (right):

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/reader...
clients/drcachesim/reader/ipc_reader.h:45: #include "../common/memref.h"
On 2016/09/15 19:33:53, bruening wrote:
> As we discussed offline: in some sense memref seems to belong in reader/ but
> OTOH it is used everywhere after having been read, so perhaps in commmon/ is
> fine.

For the future external tool usage, we may want to put memref.h in somewhere so
that the external module/tool could include it.

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/simula...
File clients/drcachesim/simulator/caching_device.h (right):

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/simula...
clients/drcachesim/simulator/caching_device.h:57: virtual void request(const
memref_t &memref);
On 2016/09/15 19:33:53, bruening wrote:
> This uses memref right here, so it should include memref.h

Done.

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/simula...
File clients/drcachesim/simulator/tlb_simulator.cpp (right):

https://codereview.appspot.com/305470043/diff/80001/clients/drcachesim/simula...
clients/drcachesim/simulator/tlb_simulator.cpp:56: ipc_iter =
ipc_reader_t(op_ipc_name.get_value().c_str());
On 2016/09/15 19:33:53, bruening wrote:
> This uses ipc_reader so it should include the header

Done.

[zhaoqin, 2016-09-15 20:32:17]

Committed as
https://github.com/DynamoRIO/dynamorio/commit/fcd2c4a95032237a4e366ea477a14cf...

Final commit log: 
---------------
i#2006 generalize drcachesim: split reader from simulator

Split trace reader from simulator directory into separate reader
directory.

Move launcher out of simulator direcory.

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