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

[Andres Freund via Phabricator via llvm-commits]

anarazel marked 2 inline comments as done.
anarazel added inline comments.


================
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());
----------------
reames wrote:
> 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.
Submitted as https://reviews.llvm.org/D47343


================
Comment at: lib/ExecutionEngine/PerfJITEvents/PerfJITEventListener.cpp:225
+  if (!Dumpstream->has_error())
+    SuccessfullyInitialized = true;
+}
----------------
reames wrote:
> Probably better to just report a fatal error.  Error handling for event handlers like this is not well specified at the moment.  
Hm. It seems not unrealistic for this to fail in some environments, e.g. due to running as a user with few permissions. It seems nicer to just not emit profiling data in that case, without fataling out.


================
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";
----------------
reames wrote:
> is there a cleaner way to check for this?  
I don't know of any way that's meaningfully cleaner. We could just skip the check and hope for the best I guess?


Repository:
  rL LLVM

https://reviews.llvm.org/D44892