[PATCH] D39114: [XRay] Initial XRay in Darwin Support

[Kuba (Brecka) Mracek via Phabricator via llvm-commits]

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

Hi and sorry for replying so late! All of the changes LGTM with a few nits.

> Assembly for Darwin x86_64 and how we avoid the ELF'isms (do we need to implement darwin-specific assembly for MachO?)

This should be actually easily solvable by `#include`'ing sanitizer_common/sanitizer_asm.h and then using the macros from there (perhaps introducing some new macros or generalizing the existing ones). Take a look at what was done in lib/tsan/rtl/tsan_rtl_amd64.S. Names of symbols should use a macro like `ASM_SYMBOL` which will add an underscore on macOS. `.type` directives should use `ASM_TYPE_FUNCTION`. And so on. It should be possible to reuse almost all of the assembly instructions unchanged. I think I have an ugly old patch for this somewhere, I can send it to you if you want.

> Syscalls, testing, etc. -- whether we need to do anything special here for making it work on macOS.

We should try to move most of the tests in the Linux directory to a common tests directory. I don't see anything particularly Linux-y there.

Regarding syscalls, I'm not sure I understand the problem. Is XRay doing any syscall interception or something like that?



================
Comment at: compiler-rt/cmake/config-ix.cmake:419-425
+  # For XRay, only support x86_64 in APPLE.
+  # list_intersect(XRAY_SUPPORTED_ARCH
+  #   ALL_XRAY_SUPPORTED_ARCH
+  #   SANITIZER_COMMON_SUPPORTED_ARCH)
+  set(XRAY_SUPPORTED_ARCH ${X86_64})
+
+
----------------
Instead of this change, please do something similar like we do for `ALL_LSAN_SUPPORTED_ARCH` above (line 200).


================
Comment at: compiler-rt/lib/xray/CMakeLists.txt:82
+  add_compiler_rt_runtime(clang_rt.xray
+    STATIC
+    OS osx
----------------
`STATIC` is okay for now, but we have much better experience with dynamic libraries for runtimes. Is there some reason to use a static library?


================
Comment at: compiler-rt/test/xray/TestCases/Darwin/always-never-instrument.cc:12
+// RUN:    FileCheck %s --check-prefix ALWAYSINSTR
+// REQUIRES: x86_64-linux
+// REQUIRES: built-in-llvm-tree
----------------
Should this `REQUIRES` be here?

Similarly, in all the tests in the `Linux/` directory, we should change `REQUIRES: x86_64-linux` to just `REQUIRES: x86_64`.


https://reviews.llvm.org/D39114