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

[Dean Michael Berris via Phabricator via cfe-commits]

dberris added a comment.

In https://reviews.llvm.org/D39114#914098, @kubamracek wrote:

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


All good, thanks for having a look!

>> 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.

Thanks for the hints, that was easy enough!

>> 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.

Agreed. Let me make a few of those changes.

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

Nope, just calling mmap and mprotect -- which should just be available on macOS. :)



================
Comment at: compiler-rt/lib/xray/CMakeLists.txt:82
+  add_compiler_rt_runtime(clang_rt.xray
+    STATIC
+    OS osx
----------------
kubamracek wrote:
> `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?
Let me try with a `SHARED` build, and report back.


================
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
----------------
kubamracek wrote:
> 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`.
Not anymore, let me change that next.


https://reviews.llvm.org/D39114