[PATCH] D39114: [XRay] Initial XRay in Darwin Support
Dean Michael Berris via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 9 02:17:35 PST 2017
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
+ 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?
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
> 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.
More information about the cfe-commits