Add flow graph printing into a .cfg file with flag --print-flow-graph-file.

Add flow graph printing into a .cfg file with flag --print-flow-graph-file.

This CL adds a flag --print-flow-graph-file which produces one
.cfg-file per isolate when enabled. Currently it prints the final
flow graph before code generation.

I also added SuccessorAt and SuccessorCount to allow easy iteration
over the successors of a basic block.
Committed: https://code.google.com/p/dart/source/detail?r=8322

[srdjan, 2012-06-01 15:35:26]

LGTM with comments once FILE access OKd by asiva

https://chromiumcodereview.appspot.com/10446116/diff/1/vm/il_printer.h
File vm/il_printer.h (right):

https://chromiumcodereview.appspot.com/10446116/diff/1/vm/il_printer.h#newcode65
vm/il_printer.h:65: file_ = isolate->flow_graph_file();
Move the code into .cc

https://chromiumcodereview.appspot.com/10446116/diff/1/vm/intermediate_langua...
File vm/intermediate_language.cc (right):

https://chromiumcodereview.appspot.com/10446116/diff/1/vm/intermediate_langua...
vm/intermediate_language.cc:278: ASSERT(StraightLineSuccessor() == NULL ||
Add parenthesis.

https://chromiumcodereview.appspot.com/10446116/diff/1/vm/intermediate_langua...
vm/intermediate_language.cc:284: BlockEntryInstr*
Instruction::SuccessorAt(intptr_t index) const {
ASSERT(0 <= index && index < SuccessorCount())

https://chromiumcodereview.appspot.com/10446116/diff/1/vm/intermediate_langua...
vm/intermediate_language.cc:294: BlockEntryInstr*
GraphEntryInstr::SuccessorAt(intptr_t index) const {
ASSERT(0 <= index && index < SuccessorCount())

https://chromiumcodereview.appspot.com/10446116/diff/1/vm/intermediate_langua...
File vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10446116/diff/1/vm/intermediate_langua...
vm/intermediate_language.h:1387: // function.
Isn't it vice versa: 0 successors means last instruction in a block?

[siva, 2012-06-01 16:51:55]

We don't allow access to files from within the VM this is because we link the VM
in dartium too (chrome version) and
the code you have currently will not work there.

Could you look at the implementation of
--generate_pprof_symbols and do something similar.

Maybe once you are done with the implementing it that way, we can refactor a bit
and come up with a generic dart api function for pulling out debug/pprof or
trace information from the VM.

[Florian Schneider, 2012-06-04 12:29:06]

New patchset uploaded that uses the exisiting API for file I/O.

[srdjan, 2012-06-04 15:15:01]

LGTM  but wait also on Siva's LGTM please.

[siva, 2012-06-04 18:23:57]

LGTM with some comments.

https://chromiumcodereview.appspot.com/10446116/diff/11001/bin/main.cc
File bin/main.cc (right):

https://chromiumcodereview.appspot.com/10446116/diff/11001/bin/main.cc#newcod...
bin/main.cc:139: ASSERT(flow_graph_file != NULL);
Where is flow_graph_file closed?

https://chromiumcodereview.appspot.com/10446116/diff/11001/vm/dart.h
File vm/dart.h (right):

https://chromiumcodereview.appspot.com/10446116/diff/11001/vm/dart.h#newcode46
vm/dart.h:46: static FileWriterFunction flow_graph_writer_;
If you have this as a static function with a static file handle, how does it
work in multiple isolate situations? Is it ok if you get multiple flow graphs
from different isolates intertwined in the same output file?

https://chromiumcodereview.appspot.com/10446116/diff/11001/vm/flow_graph_buil...
File vm/flow_graph_builder.cc (right):

https://chromiumcodereview.appspot.com/10446116/diff/11001/vm/flow_graph_buil...
vm/flow_graph_builder.cc:2317: FlowGraphVisualizer printer(function,
reverse_postorder);
Over here this appears to be a per isolate object but when dumping out the
information, output from all isolates will get mixed up. Not sure how the
visualizer will handle that.

[Florian Schneider, 2012-06-06 11:06:10]

https://chromiumcodereview.appspot.com/10446116/diff/11001/bin/main.cc
File bin/main.cc (right):

https://chromiumcodereview.appspot.com/10446116/diff/11001/bin/main.cc#newcod...
bin/main.cc:139: ASSERT(flow_graph_file != NULL);
On 2012/06/04 18:23:57, asiva wrote:
> Where is flow_graph_file closed?

At process exit.  I could close it explicitly right before every possible exit
of main() which is currently in four different places.  Since it is only opened
when the --generate-flow-graph flag is on, I prefer not explicitly closing it
for conciseness.

https://chromiumcodereview.appspot.com/10446116/diff/11001/vm/dart.h
File vm/dart.h (right):

https://chromiumcodereview.appspot.com/10446116/diff/11001/vm/dart.h#newcode46
vm/dart.h:46: static FileWriterFunction flow_graph_writer_;
On 2012/06/04 18:23:57, asiva wrote:
> If you have this as a static function with a static file handle, how does it
> work in multiple isolate situations? Is it ok if you get multiple flow graphs
> from different isolates intertwined in the same output file?

It does not work.  I'm fine for now with it working only for the single-isolate
case: The tests and benchmarks we're looking at do not create multiple isolate.

Alternative would be to either have one file per isolate or control file access
using a lock so that only one isolate at a time can write a flow graph.

[siva, 2012-06-06 17:02:47]

https://chromiumcodereview.appspot.com/10446116/diff/11001/bin/main.cc
File bin/main.cc (right):

https://chromiumcodereview.appspot.com/10446116/diff/11001/bin/main.cc#newcod...
bin/main.cc:139: ASSERT(flow_graph_file != NULL);
I disagree, correctness not conciseness is the yardstick to use. If a file is
opened it needs to be closed properly. You cannot rely on process exit doing the
right thing here.

Especially if you create these files on a per isolate basis then you should
close them when shutting down the isolate.


On 2012/06/06 11:06:10, Florian Schneider wrote:
> On 2012/06/04 18:23:57, asiva wrote:
> > Where is flow_graph_file closed?
> 
> At process exit.  I could close it explicitly right before every possible exit
> of main() which is currently in four different places.  Since it is only
opened
> when the --generate-flow-graph flag is on, I prefer not explicitly closing it
> for conciseness.

https://chromiumcodereview.appspot.com/10446116/diff/11001/vm/dart.h
File vm/dart.h (right):

https://chromiumcodereview.appspot.com/10446116/diff/11001/vm/dart.h#newcode46
vm/dart.h:46: static FileWriterFunction flow_graph_writer_;
Having one file per isolate is probably the way to go.
I think in your current case you might think it is fine if it works only for a
single isolate but somebody else might come in try this flag on a test which has
multiple isolates and without realizing that the implementation of this flag is
broken might spend hours trying to debug why the visualizer is not displaying
stuff correctly.

If you intend to leave it as is, at least put a warning when this option is used
which states "this option is only partially implemented and does not work at all
when the application uses multiple isolates". 

On 2012/06/06 11:06:10, Florian Schneider wrote:
> On 2012/06/04 18:23:57, asiva wrote:
> > If you have this as a static function with a static file handle, how does it
> > work in multiple isolate situations? Is it ok if you get multiple flow
graphs
> > from different isolates intertwined in the same output file?
> 
> It does not work.  I'm fine for now with it working only for the
single-isolate
> case: The tests and benchmarks we're looking at do not create multiple
isolate.
> 
> Alternative would be to either have one file per isolate or control file
access
> using a lock so that only one isolate at a time can write a flow graph.

[Florian Schneider, 2012-06-07 09:32:26]

https://chromiumcodereview.appspot.com/10446116/diff/11001/bin/main.cc
File bin/main.cc (right):

https://chromiumcodereview.appspot.com/10446116/diff/11001/bin/main.cc#newcod...
bin/main.cc:139: ASSERT(flow_graph_file != NULL);
On 2012/06/06 17:02:47, asiva wrote:
> I disagree, correctness not conciseness is the yardstick to use. If a file is
> opened it needs to be closed properly. You cannot rely on process exit doing
the
> right thing here.
> 
> Especially if you create these files on a per isolate basis then you should
> close them when shutting down the isolate.
> 
> 
> On 2012/06/06 11:06:10, Florian Schneider wrote:
> > On 2012/06/04 18:23:57, asiva wrote:
> > > Where is flow_graph_file closed?
> > 
> > At process exit.  I could close it explicitly right before every possible
exit
> > of main() which is currently in four different places.  Since it is only
> opened
> > when the --generate-flow-graph flag is on, I prefer not explicitly closing
it
> > for conciseness.
> 

I view this as an experimental flag.

How about abnormal exit (SIGSEGV, SIGTERM, SIGKILL, etc.) then? We rely the
OS/library to clean up in this case.

https://chromiumcodereview.appspot.com/10446116/diff/11001/vm/dart.h
File vm/dart.h (right):

https://chromiumcodereview.appspot.com/10446116/diff/11001/vm/dart.h#newcode46
vm/dart.h:46: static FileWriterFunction flow_graph_writer_;
On 2012/06/06 17:02:47, asiva wrote:
> Having one file per isolate is probably the way to go.
> I think in your current case you might think it is fine if it works only for a
> single isolate but somebody else might come in try this flag on a test which
has
> multiple isolates and without realizing that the implementation of this flag
is
> broken might spend hours trying to debug why the visualizer is not displaying
> stuff correctly.
> 
> If you intend to leave it as is, at least put a warning when this option is
used
> which states "this option is only partially implemented and does not work at
all
> when the application uses multiple isolates". 
> 
> On 2012/06/06 11:06:10, Florian Schneider wrote:
> > On 2012/06/04 18:23:57, asiva wrote:
> > > If you have this as a static function with a static file handle, how does
it
> > > work in multiple isolate situations? Is it ok if you get multiple flow
> graphs
> > > from different isolates intertwined in the same output file?
> > 
> > It does not work.  I'm fine for now with it working only for the
> single-isolate
> > case: The tests and benchmarks we're looking at do not create multiple
> isolate.
> > 
> > Alternative would be to either have one file per isolate or control file
> access
> > using a lock so that only one isolate at a time can write a flow graph.
> 

I can add a TODO. I don't think this is a typical use case. If there is a large
number of isolates though it may be very confusing to have one file per isolate.
- i.e. finding the method of interest would involve grepping the files first.