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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
