[PATCH] D21612: Work-in-Progress compiler-rt prototype for XRay runtime.

[Dean Michael Berris via llvm-commits]

dberris marked 4 inline comments as done.
dberris added a comment.

Thanks for the review Reid, I'm going to clean this up a bit more later with your suggestions from the mailing list on configuration and other things. In the meantime, please have another look at what's now here.

Cheers


================
Comment at: include/CMakeLists.txt:1
@@ -1,2 +1,2 @@
 set(SANITIZER_HEADERS
   sanitizer/allocator_interface.h
----------------
rnk wrote:
> Maybe rename this to COMPILER_RT_HEADERS, since it's not just sanitizers?
Good point. Unfortunately the install path is still .../include/sanitizer -- I've added a new variable instead, COMPILER_RT_HEADERS which aggregates SANITIZER_HEADERS and XRAY_HEADERS, and another install rule for xray headers to go into .../include/xray.

Does this make sense?

================
Comment at: include/xray/xray_interface.h:17-19
@@ +16,5 @@
+
+namespace __xray {
+enum class EntryType : unsigned short { ENTRY = 0, EXIT = 1 };
+}
+
----------------
rnk wrote:
> Are you sure you want to use a C++ scoped enum in this interface? The whole thing is extern "C", it might be nicer to make it a C style enum, so you can use it in the handler prototype instead of unsigned short.
Good point, switched to using a C-style enum instead.

================
Comment at: lib/xray/xray_interface.cc:37
@@ +36,3 @@
+  uint64_t Rrdi, Rrax, Rrdx, Rrsi, Rrcx, Rr8, Rr9, Rrbp;
+  __asm__ __volatile__("mov %%rbp, %0" : "=m"(Rrbp));
+  __asm__ __volatile__("mov %%rdi, %0" : "=m"(Rrdi));
----------------
rnk wrote:
> RBP is already a CSR, why do we need to save it?
That's because this function isn't typically directly called -- this is jumped into unconditionally. Without stashing the RBP, it gets lost when the function call later on returns. Experimentation and hours of debugging led to this state of having to stash the RBP properly.

================
Comment at: lib/xray/xray_interface_internal.h:29-32
@@ +28,6 @@
+
+extern int __xray_set_handler(void (*entry)(int32_t, unsigned short));
+extern int __xray_remove_handler();
+extern int __xray_patch();
+extern int __xray_unpatch();
+
----------------
rnk wrote:
> We aren't allowed to include xray/xray_interface.h for these?
I've had to add an include directory directive to include the public header, but it works now. It's a little different from how we're doing it in the other sanitisers.


http://reviews.llvm.org/D21612