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

[Reid Kleckner via llvm-commits]

rnk added a subscriber: rnk.

================
Comment at: include/CMakeLists.txt:1
@@ -1,2 +1,2 @@
 set(SANITIZER_HEADERS
   sanitizer/allocator_interface.h
----------------
Maybe rename this to COMPILER_RT_HEADERS, since it's not just sanitizers?

================
Comment at: include/xray/xray_interface.h:17-19
@@ +16,5 @@
+
+namespace __xray {
+enum class EntryType : unsigned short { ENTRY = 0, EXIT = 1 };
+}
+
----------------
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.

================
Comment at: include/xray/xray_interface.h:45-47
@@ +44,5 @@
+//
+//   - 0 : XRay is not initialized.
+//   - 1 : We've done the notification.
+//   - 2 : Patching / un-patching is on-going.
+extern int __xray_patch();
----------------
Maybe this should be another enum? XRayPatchingStatus?

================
Comment at: lib/xray/xray_interface.cc:33-52
@@ +32,22 @@
+
+void __xray_FunctionEntry() {
+  // First thing we do is save the caller provided registers before doing any
+  // actual work.
+  uint64_t Rrdi, Rrax, Rrdx, Rrsi, Rrcx, Rr8, Rr9, Rrbp;
+  __asm__ __volatile__("mov %%rbp, %0" : "=m"(Rrbp));
+  __asm__ __volatile__("mov %%rdi, %0" : "=m"(Rrdi));
+  __asm__ __volatile__("mov %%rax, %0" : "=m"(Rrax));
+  __asm__ __volatile__("mov %%rdx, %0" : "=m"(Rrdx));
+  __asm__ __volatile__("mov %%rsi, %0" : "=m"(Rrsi));
+  __asm__ __volatile__("mov %%rcx, %0" : "=m"(Rrcx));
+  __asm__ __volatile__("mov %%r8, %0" : "=m"(Rr8));
+  __asm__ __volatile__("mov %%r9, %0" : "=m"(Rr9));
+
+  // FIXME: Handle async signal safety, and prevent recursive calls.
+  auto Fn = __xray::XRayPatchedFunction.load(std::memory_order_acquire);
+  if (Fn != nullptr) {
+    int32_t FunctionID;
+    static constexpr unsigned short Type =
+        static_cast<unsigned short>(__xray::EntryType::ENTRY);
+    __asm__("mov %%r10d, %0" : "=g"(FunctionID) : : "%r10");
+    (*Fn)(FunctionID, Type);
----------------
This inline asm is too fragile. There's nothing to say the compiler won't use R10 to load Fn. I think this should write a standalone .s file like xray_hooks_x86_64.s, and then we can add _arm.s, _aarch64.s, etc as needed. There's existing cmake for adding .s files in lib/builtins.

================
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));
----------------
RBP is already a CSR, why do we need to save it?

================
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();
+
----------------
We aren't allowed to include xray/xray_interface.h for these?

================
Comment at: lib/xray/xray_interface_internal.h:45
@@ +44,3 @@
+
+enum class EntryType : unsigned short { ENTRY = 0, EXIT = 1 };
+
----------------
ditto, it'd be nice to get this from the public interface


http://reviews.llvm.org/D21612