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

[Kuba (Brecka) Mracek via Phabricator via llvm-commits]

kubamracek 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()
----------------
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
```


================
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()
----------------
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`?


https://reviews.llvm.org/D29783