[Sanitizers, LSan, Darwin] Allow for lack of VM_MEMORY_OS_ALLOC_ONCE

[ro]

Summary

Some time ago, the sanitizers as of r315899 were imported into gcc mainline. This broke
bootstrap on Darwin 10 and 11, as reported in GCC PR sanitizer/82824
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82824) due to the unconditional use
of VM_MEMORY_OS_ALLOC_ONCE. This was only introduced in Darwin 13/Mac OS X 10.9.

The use of the macro was introduced in r300450.

I couldn't find any statement which Darwin versions are supposed to be supported by
LLVM, but the trivial patch to use the macro only if present allowed the gcc bootstrap
to finish.

So far, I haven't tried building llvm/compiler-rt on Darwin 11. Maybe the patch is
simple enough to go in nonetheless.

[kcc]

Please try to avoid #ifdefs like this -- they make the code unmaintainable.

[fjricci]

Because macOS changes quite a bit from version to version, we (or at least I) don't have strong plans to support LSan outside of 10.11+. Not sure what darwin used to store singleton kernel allocations before this page was introduced, but I assume that just not handling those allocations will lead to a ton of false positives. I'm inclined to let this fail on old versions unless there's someone willing to do the work to make sure they're properly supported. cc @kubamracek

[fjricci]

An alternative could be to just disable the build of LSan on OS X < 10.9 (or even 10.11)

[kcc]

An alternative could be to just disable the build of LSan on OS X < 10.9 (or even 10.11)

That sounds good.

[ro]

Disabling lsan on Mac OS X < 10.9 (that's where I'd start) is going to be uglier than I first thought:

• On closer look, lsan wasn't even enabled inside the gcc tree. The compilation error is coming from the lsan common part. I think this can be addressed by allowing CAN_SANITIZE_LEAKS to be defined from the build environment, as gcc currently does via a local patch for CAN_SANITIZE_UB. This way, I can add -DCAN_SANITIZE_LEAKS=0 for affected Darwin versions if I can figure out how to do this in the cmake framework.

• Besides, lsan needs to be disable completely in lib and test, following the lead of cxxabi_supported in the toplevel CMakeLists.txt.

Would it be enough to just handle the compiler-rt part like this or would I also have to reject -fsanitize=leak in clang?

[ro]

To establish a baseline for an eventual patch along my previous outline, I tried to build llvm trunk on Darwin 11 and 17 with gcc 5 resp. 7. The experience
was so bad (cf. PR 35280) that I'm giving up on this. I'll try to go the -DCAN_SANITIZE_LEAKS route via a gcc-local patch to unbreak bootstrap there.

[ro]

Just for the record, I've now posted a gcc patch for this issue. Perhaps the two-line change to lsan/lsan_common.h to allow predefining
CAN_SANITIZE_LEAKS from the build environment can still go upstream?

[ro]

Drats: here's the posting:

https://gcc.gnu.org/ml/gcc-patches/2017-11/msg01061.html

[kcc]

In D39888#924384, @ro wrote:

Just for the record, I've now posted a gcc patch for this issue. Perhaps the two-line change to lsan/lsan_common.h to allow predefining
CAN_SANITIZE_LEAKS from the build environment can still go upstream?

It's hard to judge the patch by it's description.
Also, why not #define CAN_SANITIZE_LEAKS to depend on the darwin version?

[ro]

I meant to add the URL immediately ;-)

You mean using ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED? Directly in lsan/lsan_common.h? Checking now, I see
that there's a single use in sanitizer_common/sanitizer_platform_interceptors.h already.

I still find it very hard to determine what kind of preprocessor conditional use is and isn't acceptable inside LLVM, so please bear with me.

[dberris]

If I understand correctly, the check should happen at CMake configuration time, so that lsan doesn't even build for versions of OS X that aren't even supported.

[kcc]

In D39888#924537, @dberris wrote:

If I understand correctly, the check should happen at CMake configuration time, so that lsan doesn't even build for versions of OS X that aren't even supported.

I highly prefer conditionals in the code (*platform.h header) as opposed to cmake.
Kuba, any idea how to guard against old versions of darwin?

[kubamracek]

In D39888#924658, @kcc wrote:

In D39888#924537, @dberris wrote:

If I understand correctly, the check should happen at CMake configuration time, so that lsan doesn't even build for versions of OS X that aren't even supported.

I highly prefer conditionals in the code (*platform.h header) as opposed to cmake.
Kuba, any idea how to guard against old versions of darwin?

If the only problem here is that VM_MEMORY_OS_ALLOC_ONCE is not defined in older SDKs, then I suggest to simply copy the definition from the header and define something like kSanitizerVmMemoryOsAllocOnce ourselves (and add an explaining comment, of course). We certainly did that in other parts to avoid depending on the SDK.

[ro]

In D39888#924537, @dberris wrote:

If I understand correctly, the check should happen at CMake configuration time, so that lsan doesn't even build for versions of OS X that aren't even supported.

That's what currently is happending for most of the lsan support in the gcc tree: it isn't build on Mac OS X at all, perhaps due to blocks
support missing in gcc. OTOH, that is guarded with MISSING_BLOCKS_SUPPORT, so it might even work.

However, the code at hand is in the common part of lsan which (except for CAN_SANITIZE_LEAKS) is always compiled.

It should be possible to disable all of lsan by setting COMPILER_RT_HAS_LSAN in cmake/config-ix.cmake, but I've just failed
miserably doing that when trying to disable tsan for OS X < 10.9 in LLVM 3.9.1.

[ro]

In D39888#924658, @kcc wrote:

In D39888#924537, @dberris wrote:

If I understand correctly, the check should happen at CMake configuration time, so that lsan doesn't even build for versions of OS X that aren't even supported.

I highly prefer conditionals in the code (*platform.h header) as opposed to cmake.

What's the advantage of doing that? Checks (in cmake) like SANITIZER_MIN_OSX_VERSION VERSION_LESS "10.9" should work
just fine.

Kuba, any idea how to guard against old versions of darwin?

In C, one can check ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED. I've just successfully done so (again, in the gcc
tree). Will upload the patch doing so here shortly...

[ro]

In D39888#924761, @kubamracek wrote:

If the only problem here is that VM_MEMORY_OS_ALLOC_ONCE is not defined in older SDKs, then I suggest to simply copy the definition from the header and define something like kSanitizerVmMemoryOsAllocOnce ourselves (and add an explaining comment, of course). We certainly did that in other parts to avoid depending on the SDK.

But what does this buy us? AFAICS, the underlying functionality only appeared in OS X 10.9 and no way/amount of providing the macros
will introduce the kernel side of that.

[ro]

gcc patch (C only, no build system parts needed) to disable lsan common parts and
disable use of weak definitions before OS X 10.9. Just for reference.

[kubamracek]

In D39888#926079, @ro wrote:

In D39888#924761, @kubamracek wrote:

If the only problem here is that VM_MEMORY_OS_ALLOC_ONCE is not defined in older SDKs, then I suggest to simply copy the definition from the header and define something like kSanitizerVmMemoryOsAllocOnce ourselves (and add an explaining comment, of course). We certainly did that in other parts to avoid depending on the SDK.

But what does this buy us? AFAICS, the underlying functionality only appeared in OS X 10.9 and no way/amount of providing the macros
will introduce the kernel side of that.

As said previously, LSan is only tested on 10.11+ and I don't think anyone is going to support it on older macOS versions. If you just want to get it to compile (and not worry about it it not working), copying the constant from the header is the easiest way to go. But it's up to you.

// Before Xcode 4.5, the Darwin linker doesn't reliably support undefined
// weak symbols.  Mac OS X 10.9/Darwin 13 is the first release only supported
// by Xcode >= 4.5.
#elif SANITIZER_MAC && \
    __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ >= 1090 && !SANITIZER_GO

Can you explain? Is this the same problem? Another problem? If it's not specific to LSan then please move to a separate patch.

Secondarily, I believe the code should still be working even when VM_MEMORY_OS_ALLOC_ONCE is not provided by the kernel: We will simply not find any VM_MEMORY_OS_ALLOC_ONCE regions and move on.

[ro]

In D39888#926120, @kubamracek wrote:

As said previously, LSan is only tested on 10.11+ and I don't think anyone is going to support it on older macOS versions. If you just want to get it to compile (and not worry about it it not working), copying the constant from the header is the easiest way to go. But it's up to you.

So the effect of copying the constant or wrapping the code using it in #ifdef as my original patch did is effectively the same. While I don't
understand the opposition to the #undef route, I'm fine either way. It had been my understanding from some comments early in this
thread that the code wouldn't work reliably without VM_MEMORY_OS_ALLOC_ONCE support. However, my test results in the gcc
tree have been that there were just a hand of failures. I still haven't been able to build any current llvm version on OS X 10.7. While
testing there would be more thorough, I cannot really justify the inordinate amount of time I'm spending there...

So if you're fine with some way of disabling either the lsan common code (e.g. via the CAN_SANITIZE_LEAKS route) or just avoiding
the undefined VM_MEMORY_OS_ALLOC_ONCE (via #ifdef or a private definition), I'll provide a patch.

Before Xcode 4.5, the Darwin linker doesn't reliably support undefined
weak symbols. Mac OS X 10.9/Darwin 13 is the first release only supported
// by Xcode >= 4.5.
#elif SANITIZER_MAC && \

__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ >= 1090 && !SANITIZER_GO

Can you explain? Is this the same problem? Another problem? If it's not specific to LSan then please move to a separate patch.

This is a separate problem indeed, which I hit only after some workaround for the first was in place. Between the previous and current
import of libsanitizer into gcc, the sanitizer code started to assume that undefined weak symbols work on OS X, which isn't true for
older versions.

Secondarily, I believe the code should still be working even when VM_MEMORY_OS_ALLOC_ONCE is not provided by the kernel: We will simply not find any VM_MEMORY_OS_ALLOC_ONCE regions and move on.

This agrees with the 10.7 testresults I get (although on a limited test set).

[ro]

In D39888#926171, @ro wrote:

In D39888#926120, @kubamracek wrote:

Before Xcode 4.5, the Darwin linker doesn't reliably support undefined
weak symbols. Mac OS X 10.9/Darwin 13 is the first release only supported
// by Xcode >= 4.5.
#elif SANITIZER_MAC && \

__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ >= 1090 && !SANITIZER_GO

Can you explain? Is this the same problem? Another problem? If it's not specific to LSan then please move to a separate patch.

This is a separate problem indeed, which I hit only after some workaround for the first was in place. Between the previous and current
import of libsanitizer into gcc, the sanitizer code started to assume that undefined weak symbols work on OS X, which isn't true for
older versions.

I've now created https://reviews.llvm.org/D39888 for this.

[ro]

This version just provides a fallback definition of VM_MEMORY_OS_ALLOC_ONCE.

[ro]

In D39888#926171, @ro wrote:

In D39888#926120, @kubamracek wrote:

Secondarily, I believe the code should still be working even when VM_MEMORY_OS_ALLOC_ONCE is not provided by the kernel: We will simply not find any VM_MEMORY_OS_ALLOC_ONCE regions and move on.

This agrees with the 10.7 testresults I get (although on a limited test set).

The new patch does just that.

[ro]

ping

I know it's not yet been a week, but I'm away until new year. Maybe someone has a chance to look at it?

Thanks.

[fjricci]

I think it would be preferable to define this as a separate constant (perhaps kSanitizerVmMemoryOsAllocOnce, like @kubamracek suggested), which is set to VM_MEMORY_OS_ALLOC_ONCE if defined, and some other value (that 73 perhaps?) otherwise. Makes it clear that the constant used by the code isn't necessarily the same as the one set by the system.

[ro]

In D39888#971254, @fjricci wrote:

I think it would be preferable to define this as a separate constant (perhaps kSanitizerVmMemoryOsAllocOnce, like @kubamracek suggested), which is set to VM_MEMORY_OS_ALLOC_ONCE if defined, and some other value (that 73 perhaps?) otherwise. Makes it clear that the constant used by the code isn't necessarily the same as the one set by the system.

73 *is* the Mac OS X 10.9 value of VM_MEMORY_OS_ALLOC_ONCE. On older systems, it will never be returned by the kernel, we just need a way to
have the code compile. I don't care which way it's done actually.

[ro]

Reworked according to review comments.

Tested (inside the gcc tree) on x86_64-apple-darwin11.4.2.

[fjricci]

lgtm

[ro]

Thanks. Could you please commit it for me?

[fjricci]

Will do.

[fjricci]

This comment was removed by fjricci.

compiler-rt/trunk/lib/lsan/lsan_common_mac.cc

 
 #include "sanitizer_common/sanitizer_allocator_internal.h"
 #include "lsan_allocator.h"
 
 #include <pthread.h>
 
 #include <mach/mach.h>
 
+// Only introduced in Mac OS X 10.9.
+#ifdef VM_MEMORY_OS_ALLOC_ONCE
+static const int kSanitizerVmMemoryOsAllocOnce = VM_MEMORY_OS_ALLOC_ONCE;
+#else
+static const int kSanitizerVmMemoryOsAllocOnce = 73;
+#endif
+
 namespace __lsan {
 
 typedef struct {
   int disable_counter;
   u32 current_thread_id;
   AllocatorCache cache;
 } thread_local_data_t;
 
while (err == KERN_SUCCESS) {
     struct vm_region_submap_info_64 info;
     err = vm_region_recurse_64(port, &address, &size, &depth,
                                (vm_region_info_t)&info, &count);
 
     uptr end_address = address + size;
 
     // libxpc stashes some pointers in the Kernel Alloc Once page,
     // make sure not to report those as leaks.
-    if (info.user_tag == VM_MEMORY_OS_ALLOC_ONCE) {
+    if (info.user_tag == kSanitizerVmMemoryOsAllocOnce) {
       ScanRangeForPointers(address, end_address, frontier, "GLOBAL",
                            kReachable);
 
       // Recursing over the full memory map is very slow, break out
       // early if we don't need the full iteration.
       if (!flags()->use_root_regions || !root_regions->size())
         break;
     }