[PATCH] D29783: Add cmake build support for lsan on OS X

[Francis Ricci via Phabricator via llvm-commits]

fjricci added inline comments.


================
Comment at: cmake/config-ix.cmake:167-171
+if(APPLE)
+  set(ALL_LSAN_SUPPORTED_ARCH ${X86_64} ${ARM64})
+else()
+  set(ALL_LSAN_SUPPORTED_ARCH ${X86} ${X86_64} ${MIPS64} ${ARM64})
+endif()
----------------
kubamracek wrote:
> fjricci wrote:
> > kubamracek wrote:
> > > Why exactly do we need this change?  Will the 32-bit x86 slice fail to build?
> > Yeah, `__thread` is only supported on 64-bit darwin targets.
> Are you sure?
> 
> ```
> $ uname
> Darwin
> $ echo "__thread int hello; int main() { hello = 42; }" | clang -x c -arch i386 -
> $ ./a.out
> $ # works fine
> ```
  $ echo "__thread int hello; int main() { hello = 42; }" | clang -x c -arch i386 -miphoneos-version-min=8.0 -
  <stdin>:1:1: error: thread-local storage is not supported for the current target
  __thread int hello; int main() { hello = 42; }
  ^

It looks like i386 support isn't added until -miphoneos-version-min=9.0, and I'm not sure that we want to bump the version that high.


================
Comment at: lib/lsan/CMakeLists.txt:19-21
+if(APPLE AND COMPILER_RT_HAS_LSAN)
+  set(LSAN_CFLAGS ${LSAN_CFLAGS} -DCAN_SANITIZE_LEAKS_MAC=1)
+endif()
----------------
kubamracek wrote:
> fjricci wrote:
> > kubamracek wrote:
> > > How about we just unconditionally propagate `CAN_SANITIZE_LEAKS` (no `_MAC`) and remove that ugly ifdef in lsan_common.h?
> > > 
> > > Doesn't this also need to go to lib/asan/CMakeLists.txt?
> > Well, the issue with that is that we build LSanCommon for 32-bit architectures (since asan builds for 32-bit), so we still need the architecture check in lsan_common.h to make sure that it's an empty stub if we're building for 32-bit.
> > 
> > This doesn't need to go into lib/asan/CMakeLists.txt because that just pulls in RTLSanCommon, not the individual sources, and RTLsanCommon will already have the flag.
> Ok, can we add a FIXME comment here saying that this will be removed once LSan is fully working on Darwin?
> 
> Right, the LSan sources will always have `CAN_SANITIZE_LEAKS_MAC`.  But ASan does include headers from `lsan/`, and I don't see how they have have `CAN_SANITIZE_LEAKS_MAC`.  Can you verify that ASan files have are able to see `CAN_SANITIZE_LEAKS == 1`?
Ahh I see. You're right, it's not propogated to the asan sources.


https://reviews.llvm.org/D29783