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