[PATCH] D44892: Add PerfJITEventListener for perf profiling support.

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/comments addressed before submission.

I didn't review all of the perf API usage closely, but I'm assuming this at least mostly works.  If it does, it's better to have something checked in we can improve than nothing.



================
Comment at: CMakeLists.txt:448
+  endif( NOT CMAKE_SYSTEM_NAME MATCHES "Linux" )
+endif( LLVM_USE_PERF )
+
----------------
anarazel wrote:
> Should this be enable this by default on linux?
No.  Or at least, not in the first patch.


================
Comment at: CMakeLists.txt:659
+if (LLVM_USE_PERF)
+  set(LLVMOPTIONALCOMPONENTS PerfJITEvents)
+endif (LLVM_USE_PERF)
----------------
You're dropping the other optional components.  Please do what OPROFILE does just above.


================
Comment at: lib/ExecutionEngine/MCJIT/MCJIT.cpp:224
+  // Can't call notifiers yet as relocations have not yet been performed, and
+  // memory hasn't been marked executable.
+  PendingLoadedObjects.push_back(LoadedObject->get());
----------------
anarazel wrote:
> Arguably this should be merged separately?
Indeed, this needs to be separated and reviewed separately.  I fine with that being done in either order w.r.t. this patch landing.


================
Comment at: lib/ExecutionEngine/PerfJITEvents/PerfJITEventListener.cpp:225
+  if (!Dumpstream->has_error())
+    SuccessfullyInitialized = true;
+}
----------------
Probably better to just report a fatal error.  Error handling for event handlers like this is not well specified at the moment.  


================
Comment at: lib/ExecutionEngine/PerfJITEvents/PerfJITEventListener.cpp:377
+  // check ELF signature
+  if (id[0] != 0x7f || id[1] != 'E' || id[2] != 'L' || id[3] != 'F') {
+    errs() << "invalid elf signature\n";
----------------
is there a cleaner way to check for this?  


================
Comment at: lib/ExecutionEngine/PerfJITEvents/PerfJITEventListener.cpp:389
+  hdr.ElfMach = info.e_machine;
+  ::close(SelfFD);
+  return true;
----------------
Instead of the goto error idiom, please use a RAII pattern with a destructor which closes it.


================
Comment at: lib/ExecutionEngine/PerfJITEvents/PerfJITEventListener.cpp:479
+                      sizeof(LineInfo));
+    Dumpstream->write(Line.FileName.c_str(), Line.FileName.size() + 1);
+  }
----------------
anarazel wrote:
> This should be optimized to avoid re-emitting the filename if previous filename is the same.
I think this is fine for the first version, but yes, a good followup.


Repository:
  rL LLVM

https://reviews.llvm.org/D44892