Move generate_perf_events_symbols flag to VM

Move generate_perf_events_symbols flag to VM

The generate_perf_events_symbols flag is currently processed in the standalone dart program's main.cc. This means that other VM clients (e.g. Dartium) cannot generate symbols for perf profiling.

This CL moves the perf flag handling into the core VM.

[Max Heinritz, 2012-12-11 05:31:58]

New, cleaner issue for the same change. Please ignore the previous, messier (now
deleted) issue. Thanks!

[siva, 2012-12-11 18:02:51]

https://chromiumcodereview.appspot.com/11490026/diff/2001/runtime/vm/code_obs...
File runtime/vm/code_observers.cc (right):

https://chromiumcodereview.appspot.com/11490026/diff/2001/runtime/vm/code_obs...
runtime/vm/code_observers.cc:14: #include "bin/file.h"
We can't include bin header files directly like this in the VM.

I think you don't need this include as you are only dealing with the callbacks.

https://chromiumcodereview.appspot.com/11490026/diff/2001/runtime/vm/code_obs...
runtime/vm/code_observers.cc:68: intptr_t pid = getpid();
This may not port to Windows, maybe you should add an OS abstraction for this in
os.h

We probably should add the filename to the OS
abstraction:

e.g:

OS::PerfSymbolsFileName() or something like that.

and the getpid() /tmp/perf... stuff moved into that abstraction.

https://chromiumcodereview.appspot.com/11490026/diff/2001/runtime/vm/code_obs...
runtime/vm/code_observers.cc:78: if (file_close != NULL) {
Also you probably need:
ASSERT(out_file_ != NULL);

https://chromiumcodereview.appspot.com/11490026/diff/2001/runtime/vm/code_obs...
runtime/vm/code_observers.cc:104: void* out_file_;
Not related to your change but these classes in this file are missing the
standard DISALLOW stuff...

https://chromiumcodereview.appspot.com/11490026/diff/2001/runtime/vm/code_obs...
runtime/vm/code_observers.cc:161: #if defined(TARGET_OS_LINUX)
Why does this have to conditional on a OS define,

why not
  if (generate_perf_events_symbols) {
    Register(new PerfCodeObserver));
  }

[Max Heinritz, 2012-12-11 19:09:25]

Addresses Siva's comments.

https://chromiumcodereview.appspot.com/11490026/diff/2001/runtime/vm/code_obs...
File runtime/vm/code_observers.cc (right):

https://chromiumcodereview.appspot.com/11490026/diff/2001/runtime/vm/code_obs...
runtime/vm/code_observers.cc:14: #include "bin/file.h"
On 2012/12/11 18:02:51, siva wrote:
> We can't include bin header files directly like this in the VM.
> 
> I think you don't need this include as you are only dealing with the
callbacks.

Done.

https://chromiumcodereview.appspot.com/11490026/diff/2001/runtime/vm/code_obs...
runtime/vm/code_observers.cc:68: intptr_t pid = getpid();
On 2012/12/11 18:02:51, siva wrote:
> This may not port to Windows, maybe you should add an OS abstraction for this
in
> os.h
> 
> We probably should add the filename to the OS
> abstraction:
> 
> e.g:
> 
> OS::PerfSymbolsFileName() or something like that.
> 
> and the getpid() /tmp/perf... stuff moved into that abstraction.

Could I do this in another CL? I added a conditional on an OS define for now,
which is consistent with the existing implementation in main.cc.

https://chromiumcodereview.appspot.com/11490026/diff/2001/runtime/vm/code_obs...
runtime/vm/code_observers.cc:78: if (file_close != NULL) {
On 2012/12/11 18:02:51, siva wrote:
> Also you probably need:
> ASSERT(out_file_ != NULL);

Done.

https://chromiumcodereview.appspot.com/11490026/diff/2001/runtime/vm/code_obs...
runtime/vm/code_observers.cc:104: void* out_file_;
On 2012/12/11 18:02:51, siva wrote:
> Not related to your change but these classes in this file are missing the
> standard DISALLOW stuff...

Any action to take here?

https://chromiumcodereview.appspot.com/11490026/diff/2001/runtime/vm/code_obs...
runtime/vm/code_observers.cc:161: #if defined(TARGET_OS_LINUX)
On 2012/12/11 18:02:51, siva wrote:
> Why does this have to conditional on a OS define,
> 
> why not
>   if (generate_perf_events_symbols) {
>     Register(new PerfCodeObserver));
>   }

Done.

I had originally added this to prevent creation of a PerfCodeObeserver on
non-Linux systems. But now I realize that check belongs in the PerfCodeObeserver
class itself, because an instance be created elsewhere.

[siva, 2012-12-11 19:57:12]

Ok we can clean up the OS specific stuff in a new CL
(please create a bug and add a TODO(bug number) in the
code.

LGTM.

https://chromiumcodereview.appspot.com/11490026/diff/5001/runtime/vm/code_obs...
File runtime/vm/code_observers.cc (right):

https://chromiumcodereview.appspot.com/11490026/diff/5001/runtime/vm/code_obs...
runtime/vm/code_observers.cc:66: #if defined(TARGET_OS_LINUX)
Please add a TODO that this host specific code will be cleaned up in a new CL
(open a bug for it).

https://chromiumcodereview.appspot.com/11490026/diff/5001/runtime/vm/code_obs...
runtime/vm/code_observers.cc:163: // TODO(meh): move OS-specific logic to
separate module
Is this comment still valid?

[Max Heinritz, 2012-12-11 21:26:51]

Added TODO comments about moving registration and OS-specific logic to the OS
abstraction.

https://chromiumcodereview.appspot.com/11490026/diff/5001/runtime/vm/code_obs...
File runtime/vm/code_observers.cc (right):

https://chromiumcodereview.appspot.com/11490026/diff/5001/runtime/vm/code_obs...
runtime/vm/code_observers.cc:66: #if defined(TARGET_OS_LINUX)
On 2012/12/11 19:57:12, siva wrote:
> Please add a TODO that this host specific code will be cleaned up in a new CL
> (open a bug for it).

Done.

https://chromiumcodereview.appspot.com/11490026/diff/5001/runtime/vm/code_obs...
runtime/vm/code_observers.cc:163: // TODO(meh): move OS-specific logic to
separate module
On 2012/12/11 19:57:12, siva wrote:
> Is this comment still valid?

Clarified comment.