Move generate_perf_events_symbols flag to VM

runtime/vm/code_observers.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/code_observers.h"
 
 #include "vm/dart.h"
 #include "vm/debuginfo.h"
 #include "vm/flags.h"
 #include "vm/isolate.h"
 #include "vm/os.h"
 #include "vm/vtune.h"
 #include "vm/zone.h"
+#include "bin/file.h"

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

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.

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

Done.

 
 namespace dart {
 
 DEFINE_FLAG(bool, generate_gdb_symbols, false,
     "Generate symbols of generated dart functions for debugging with GDB");
+DEFINE_FLAG(bool, generate_perf_events_symbols, false,
+    "Generate events symbols for profiling with perf");
 
 intptr_t CodeObservers::observers_length_ = 0;
 CodeObserver** CodeObservers::observers_ = NULL;
 
 
 void CodeObservers::Register(CodeObserver* observer) {
   observers_length_++;
   observers_ = reinterpret_cast<CodeObserver**>(
       realloc(observers_, sizeof(observer) * observers_length_));
   if (observers_ == NULL) {
@@ skipping 20 matching lines @@
 bool CodeObservers::AreActive() {
   for (intptr_t i = 0; i < observers_length_; i++) {
     if (observers_[i]->IsActive()) return true;
   }
   return false;
 }
 
 
 class PerfCodeObserver : public CodeObserver {
  public:
+  PerfCodeObserver() {
+    Dart_FileOpenCallback file_open = Isolate::file_open_callback();
+    if (file_open == NULL) {
+      return;
+    }
+    const char* format = "/tmp/perf-%ld.map";
+    intptr_t pid = getpid();

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

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.

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

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.

+    intptr_t len = OS::SNPrint(NULL, 0, format, pid);
+    char* filename = new char[len + 1];
+    OS::SNPrint(filename, len + 1, format, pid);
+    out_file_ = (*file_open)(filename);
+  }
+
+  // Not currently being called
+  ~PerfCodeObserver() {
+    Dart_FileCloseCallback file_close = Isolate::file_close_callback();
+    if (file_close != NULL) {

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

Also you probably need:
ASSERT(out_file_ != NULL);

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

Done.

+      (*file_close)(out_file_);
+    }
+  }
+
   virtual bool IsActive() const {
-    return Dart::perf_events_file() != NULL;
+    return FLAG_generate_perf_events_symbols;
   }
 
   virtual void Notify(const char* name,
                       uword base,
                       uword prologue_offset,
                       uword size,
                       bool optimized) {
+    Dart_FileWriteCallback file_write = Isolate::file_write_callback();
+    ASSERT(file_write != NULL);
     const char* format = "%"Px" %"Px" %s%s\n";
     const char* marker = optimized ? "*" : "";
     intptr_t len = OS::SNPrint(NULL, 0, format, base, size, marker, name);
     char* buffer = Isolate::Current()->current_zone()->Alloc<char>(len + 1);
     OS::SNPrint(buffer, len + 1, format, base, size, marker, name);
-    Dart_FileWriteCallback file_write = Isolate::file_write_callback();
-    ASSERT(file_write != NULL);
-    void* file = Dart::perf_events_file();
-    ASSERT(file != NULL);
-    (*file_write)(buffer, len, file);
+    ASSERT(out_file_ != NULL);
+    (*file_write)(buffer, len, out_file_);
   }
+
+ private:
+  void* out_file_;

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

Not related to your change but these classes in this file are missing the
standard DISALLOW stuff...

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

Any action to take here?

 };
 
 
 class PprofCodeObserver : public CodeObserver {
  public:
   virtual bool IsActive() const {
     return Dart::pprof_symbol_generator() != NULL;
   }
 
   virtual void Notify(const char* name,
@@ skipping 35 matching lines @@
                                  (base + prologue_offset),
                                  (size - prologue_offset));
     } else {
       DebugInfo::RegisterSection(name, base, size);
     }
   }
 };
 
 
 void CodeObservers::InitOnce() {
+// TODO(meh): move OS-specific logic to separate module
+#if defined(TARGET_OS_LINUX)

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

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]

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.

   Register(new PerfCodeObserver);
+#endif
   Register(new PprofCodeObserver);
   Register(new GdbCodeObserver);
 #if defined(DART_VTUNE_SUPPORT)
   Register(new VTuneCodeObserver);
 #endif
 }
 
 
 }  // namespace dart