Page MenuHomePhabricator

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

Authored by ro on Nov 10 2017, 1:13 AM.

Details

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.

Diff Detail

Event Timeline

ro created this revision.Nov 10 2017, 1:13 AM
kcc added a reviewer: kuba.Nov 10 2017, 7:32 AM
kcc edited edge metadata.

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

fjricci requested changes to this revision.Nov 10 2017, 7:38 AM
fjricci added a reviewer: kubamracek.
fjricci added a subscriber: kubamracek.

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

This revision now requires changes to proceed.Nov 10 2017, 7:38 AM

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

kcc added a comment.Nov 10 2017, 9:09 AM

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

That sounds good.

ro added a comment.Nov 11 2017, 2:27 AM

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 added a comment.Nov 12 2017, 12:09 PM

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 added a comment.Nov 14 2017, 2:48 AM

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?

kcc added a comment.Nov 14 2017, 3:06 AM
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 added a comment.Nov 14 2017, 3:25 AM

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.

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 added a comment.Nov 14 2017, 7:23 AM

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 edited edge metadata.Nov 14 2017, 8:41 AM
In D39888#924658, @kcc 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 added a comment.Nov 15 2017, 6:57 AM

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 added a comment.Nov 15 2017, 6:59 AM
In D39888#924658, @kcc 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 added a comment.Nov 15 2017, 7:01 AM

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 updated this revision to Diff 123023.Nov 15 2017, 7:05 AM
ro edited edge metadata.

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.

In D39888#926079, @ro 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 added a comment.Nov 15 2017, 8:14 AM

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 added a project: Restricted Project.Nov 20 2017, 5:13 AM
ro set the repository for this revision to rCRT Compiler Runtime.Nov 30 2017, 12:12 PM
ro retitled this revision from [lsan] Only use VM_MEMORY_OS_ALLOC_ONCE on Darwin versions that support it to [Sanitizers, LSan, Darwin] Provide fallback definition of VM_MEMORY_OS_ALLOC_ONCE.Dec 18 2017, 4:11 AM
Herald added a subscriber: Restricted Project. · View Herald TranscriptDec 18 2017, 4:11 AM
ro added a comment.Dec 18 2017, 4:12 AM
In D39888#926171, @ro 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 updated this revision to Diff 127331.Dec 18 2017, 4:13 AM

This version just provides a fallback definition of VM_MEMORY_OS_ALLOC_ONCE.

ro added a comment.Dec 18 2017, 4:14 AM
In D39888#926171, @ro 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 added a comment.Dec 22 2017, 5:11 AM

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.

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 retitled this revision from [Sanitizers, LSan, Darwin] Provide fallback definition of VM_MEMORY_OS_ALLOC_ONCE to [Sanitizers, LSan, Darwin] Allow for lack of VM_MEMORY_OS_ALLOC_ONCE.Jan 12 2018, 8:50 AM

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 updated this revision to Diff 129638.Jan 12 2018, 8:53 AM

Reworked according to review comments.

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

fjricci accepted this revision.Jan 12 2018, 9:03 AM

lgtm

This revision is now accepted and ready to land.Jan 12 2018, 9:03 AM
ro added a comment.Jan 13 2018, 6:33 AM

Thanks. Could you please commit it for me?

This revision was automatically updated to reflect the committed changes.
This comment was removed by fjricci.