| Message ID | yddr2t12l7x.fsf@CeBiTec.Uni-Bielefeld.DE |
|---|---|
| State | New |
| Headers | show |
| Series | [libsanitizer] Fix Mac OS X 10.7 bootstrap (PR sanitizer/82824) | expand |
On Tue, Nov 14, 2017 at 11:44:34AM +0100, Rainer Orth wrote: > 2017-11-13 Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> > > PR sanitizer/82824 > * configure.ac (LSAN_COMMON_SUPPORTED): New conditional. Disable > before *-*-darwin13*. > (WEAK_HOOKS_SUPPORT): New conditional. > * configure: Regenerate. > * asan/Makefile.am (DEFS) [!LSAN_COMMON_SUPPORTED]: Disable > CAN_SANITIZE_LEAKS. > (DEFS) [!LSAN_COMMON_SUPPORTED]: Disable > SANITIZER_SUPPORTS_WEAK_HOOKS. > * asan/Makefile.in: Regenerate. > * lsan/Makefile.am (DEFS) [!LSAN_COMMON_SUPPORTED]: Disable > CAN_SANITIZE_LEAKS. > (DEFS) [!LSAN_COMMON_SUPPORTED]: Disable > SANITIZER_SUPPORTS_WEAK_HOOKS. > * lsan/Makefile.in: Regenerate. > * lsan/lsan_common.h: Allow predefining CAN_SANITIZE_LEAKS. > * sanitizer_common/Makefile.am (DEFS) [!LSAN_COMMON_SUPPORTED]: > Disable SANITIZER_SUPPORTS_WEAK_HOOKS. > * sanitizer_common/Makefile.in: Regenerate. > --- a/libsanitizer/configure.ac > +++ b/libsanitizer/configure.ac > @@ -140,6 +140,24 @@ case "$host" in > esac > AM_CONDITIONAL(USING_MAC_INTERPOSE, $MAC_INTERPOSE) > > +# lsan_common_mac.cc needs VM_MEMORY_OS_ALLOC_ONCE which was only > +# introduced in Mac OS X 10.9/Darwin 13. > +case "$host" in > + *-*-darwin[[1-9]] | *-*-darwin[[1-9]].* | *-*-darwin1[[0-2]]*) > + LSAN_COMMON=false ;; > + *) LSAN_COMMON=true ;; > +esac > +AM_CONDITIONAL(LSAN_COMMON_SUPPORTED, $LSAN_COMMON) > + > +# Before Xcode 4.5, the Darwin linker doesn't properly support undefined > +# weak symbols. > +case "$host" in $host here and above? Shouldn't that be $target instead? That said, I think it would be better to move this stuff except for the AM_CONDITIONAL into configure.tgt similarly to how *_SUPPORTED is handled. And the preexisting: case "$host" in *-*-darwin*) MAC_INTERPOSE=true ; enable_static=no ;; *) MAC_INTERPOSE=false ;; esac AM_CONDITIONAL(USING_MAC_INTERPOSE, $MAC_INTERPOSE) looks wrong too. > --- a/libsanitizer/lsan/lsan_common.h > +++ b/libsanitizer/lsan/lsan_common.h > @@ -20,6 +20,7 @@ > #include "sanitizer_common/sanitizer_stoptheworld.h" > #include "sanitizer_common/sanitizer_symbolizer.h" > > +#ifndef CAN_SANITIZE_LEAKS Obviously better would be to move the Darwin version checks here, but if upstream refuses to do that, we can do it this way too. Note we already have similar thing in ubsan/ubsan_platform.h, where we want to override CAN_SANITIZE_UB from command line and need to patch that header for that, because upstream doesn't allow that :(. > // LeakSanitizer relies on some Glibc's internals (e.g. TLS machinery) thus > // supported for Linux only. Also, LSan doesn't like 32 bit architectures > // because of "small" (4 bytes) pointer size that leads to high false negative > @@ -42,6 +43,7 @@ > #else > #define CAN_SANITIZE_LEAKS 0 > #endif > +#endif // CAN_SANITIZE_LEAKS > > namespace __sanitizer { > class FlagParser; Jakub
Hi Jakub, >> --- a/libsanitizer/configure.ac >> +++ b/libsanitizer/configure.ac >> @@ -140,6 +140,24 @@ case "$host" in >> esac >> AM_CONDITIONAL(USING_MAC_INTERPOSE, $MAC_INTERPOSE) >> >> +# lsan_common_mac.cc needs VM_MEMORY_OS_ALLOC_ONCE which was only >> +# introduced in Mac OS X 10.9/Darwin 13. >> +case "$host" in >> + *-*-darwin[[1-9]] | *-*-darwin[[1-9]].* | *-*-darwin1[[0-2]]*) >> + LSAN_COMMON=false ;; >> + *) LSAN_COMMON=true ;; >> +esac >> +AM_CONDITIONAL(LSAN_COMMON_SUPPORTED, $LSAN_COMMON) >> + >> +# Before Xcode 4.5, the Darwin linker doesn't properly support undefined >> +# weak symbols. >> +case "$host" in > > $host here and above? Shouldn't that be $target instead? AFAIU $host and $target are identical in target libraries, and the majority of cases uses $host right now. > That said, I think it would be better to move this stuff except for the > AM_CONDITIONAL into configure.tgt similarly to how *_SUPPORTED is handled. My reasoning was to have purely static based stuff like supported status or additional compiler/linker flags in configure.tgt, while other tests which might need configure checks go into configure.ac. > And the preexisting: > case "$host" in > *-*-darwin*) MAC_INTERPOSE=true ; enable_static=no ;; > *) MAC_INTERPOSE=false ;; > esac > AM_CONDITIONAL(USING_MAC_INTERPOSE, $MAC_INTERPOSE) > looks wrong too. Wrong in which way? Just the location? >> --- a/libsanitizer/lsan/lsan_common.h >> +++ b/libsanitizer/lsan/lsan_common.h >> @@ -20,6 +20,7 @@ >> #include "sanitizer_common/sanitizer_stoptheworld.h" >> #include "sanitizer_common/sanitizer_symbolizer.h" >> >> +#ifndef CAN_SANITIZE_LEAKS > > Obviously better would be to move the Darwin version checks here, but if > upstream refuses to do that, we can do it this way too. Kostya suggested the same. I see now that sanitizer_common/sanitizer_platform_interceptors.h already uses __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__, so I'll give it a try. > Note we already have similar thing in ubsan/ubsan_platform.h, where we > want to override CAN_SANITIZE_UB from command line and need to patch that > header for that, because upstream doesn't allow that :(. I already mentioned that as a precedent for my lsan_common.h change. Rainer
On Tue, Nov 14, 2017 at 01:57:21PM +0100, Rainer Orth wrote: > >> --- a/libsanitizer/configure.ac > >> +++ b/libsanitizer/configure.ac > >> @@ -140,6 +140,24 @@ case "$host" in > >> esac > >> AM_CONDITIONAL(USING_MAC_INTERPOSE, $MAC_INTERPOSE) > >> > >> +# lsan_common_mac.cc needs VM_MEMORY_OS_ALLOC_ONCE which was only > >> +# introduced in Mac OS X 10.9/Darwin 13. > >> +case "$host" in > >> + *-*-darwin[[1-9]] | *-*-darwin[[1-9]].* | *-*-darwin1[[0-2]]*) > >> + LSAN_COMMON=false ;; > >> + *) LSAN_COMMON=true ;; > >> +esac > >> +AM_CONDITIONAL(LSAN_COMMON_SUPPORTED, $LSAN_COMMON) > >> + > >> +# Before Xcode 4.5, the Darwin linker doesn't properly support undefined > >> +# weak symbols. > >> +case "$host" in > > > > $host here and above? Shouldn't that be $target instead? > > AFAIU $host and $target are identical in target libraries, and the > majority of cases uses $host right now. Didn't know that. Though, e.g. libsanitizer uses $target except for this single MAC_INTERPOSE case. > > That said, I think it would be better to move this stuff except for the > > AM_CONDITIONAL into configure.tgt similarly to how *_SUPPORTED is handled. > > My reasoning was to have purely static based stuff like supported status > or additional compiler/linker flags in configure.tgt, while other tests > which might need configure checks go into configure.ac. But the checks you've added are clearly static, just from parsing the triplet. > > And the preexisting: > > case "$host" in > > *-*-darwin*) MAC_INTERPOSE=true ; enable_static=no ;; > > *) MAC_INTERPOSE=false ;; > > esac > > AM_CONDITIONAL(USING_MAC_INTERPOSE, $MAC_INTERPOSE) > > looks wrong too. > > Wrong in which way? Just the location? I meant for using $host instead of $target (perhaps a non-issue) and for being in configure.ac instead of configure.tgt, where in configure.ac we could have just the AM_CONDITIONAL. Jakub
On Tue, Nov 14, 2017 at 01:57:21PM +0100, Rainer Orth wrote: > > And the preexisting: > > case "$host" in > > *-*-darwin*) MAC_INTERPOSE=true ; enable_static=no ;; > > *) MAC_INTERPOSE=false ;; > > esac > > AM_CONDITIONAL(USING_MAC_INTERPOSE, $MAC_INTERPOSE) > > looks wrong too. > > Wrong in which way? Just the location? > > >> --- a/libsanitizer/lsan/lsan_common.h > >> +++ b/libsanitizer/lsan/lsan_common.h > >> @@ -20,6 +20,7 @@ > >> #include "sanitizer_common/sanitizer_stoptheworld.h" > >> #include "sanitizer_common/sanitizer_symbolizer.h" > >> > >> +#ifndef CAN_SANITIZE_LEAKS > > > > Obviously better would be to move the Darwin version checks here, but if > > upstream refuses to do that, we can do it this way too. > > Kostya suggested the same. I see now that > sanitizer_common/sanitizer_platform_interceptors.h already uses > __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__, so I'll give it a try. Any progress with this? Jakub
Hi Jakub, > On Tue, Nov 14, 2017 at 01:57:21PM +0100, Rainer Orth wrote: >> > And the preexisting: >> > case "$host" in >> > *-*-darwin*) MAC_INTERPOSE=true ; enable_static=no ;; >> > *) MAC_INTERPOSE=false ;; >> > esac >> > AM_CONDITIONAL(USING_MAC_INTERPOSE, $MAC_INTERPOSE) >> > looks wrong too. >> >> Wrong in which way? Just the location? >> >> >> --- a/libsanitizer/lsan/lsan_common.h >> >> +++ b/libsanitizer/lsan/lsan_common.h >> >> @@ -20,6 +20,7 @@ >> >> #include "sanitizer_common/sanitizer_stoptheworld.h" >> >> #include "sanitizer_common/sanitizer_symbolizer.h" >> >> >> >> +#ifndef CAN_SANITIZE_LEAKS >> > >> > Obviously better would be to move the Darwin version checks here, but if >> > upstream refuses to do that, we can do it this way too. >> >> Kostya suggested the same. I see now that >> sanitizer_common/sanitizer_platform_interceptors.h already uses >> __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__, so I'll give it a try. > > Any progress with this? I've been using the following in my tree. Still need to try and get this upstream. Speaking of which, would it be acceptable to do another full libsanitizer merge at this point? My initial Solaris port has landed now, and the necessary changes outside of libsanitizer are almost exclusively in Solaris-specific files. I've already gave it a quick try and found no regressions on Linux/x86_64. Rainer
On Mon, Dec 18, 2017 at 11:21:35AM +0100, Rainer Orth wrote: > I've been using the following in my tree. Still need to try and get > this upstream. Please. If that doesn't work, I think we need to do it in configure, sanitizer_internal_defs.h seems to be included very early, so there is no easy header to override say in include/system/ tree. > Speaking of which, would it be acceptable to do another full > libsanitizer merge at this point? My initial Solaris port has landed > now, and the necessary changes outside of libsanitizer are almost > exclusively in Solaris-specific files. I've already gave it a quick try > and found no regressions on Linux/x86_64. I'm afraid it is too late, we have way too many breakages on the trunk and are seriously behind schedule, and sanitizer merges pretty much always create numerous regressions. Jakub
Hi Jakub, > On Mon, Dec 18, 2017 at 11:21:35AM +0100, Rainer Orth wrote: >> I've been using the following in my tree. Still need to try and get >> this upstream. > > Please. If that doesn't work, I think we need to do it in configure, > sanitizer_internal_defs.h seems to be included very early, so there > is no easy header to override say in include/system/ tree. done now as https://reviews.llvm.org/D41346. Together with the revised version of https://reviews.llvm.org/D39888 this has restored Darwin 11 build and sanitizer testing for me for quite some time. >> Speaking of which, would it be acceptable to do another full >> libsanitizer merge at this point? My initial Solaris port has landed >> now, and the necessary changes outside of libsanitizer are almost >> exclusively in Solaris-specific files. I've already gave it a quick try >> and found no regressions on Linux/x86_64. > > I'm afraid it is too late, we have way too many breakages on the trunk and > are seriously behind schedule, and sanitizer merges pretty much always > create numerous regressions. Understood, just wanted to ask. This gives me more time to work out 64-bit support and a couple of warts. Rainer
Hi Jakub, >> On Mon, Dec 18, 2017 at 11:21:35AM +0100, Rainer Orth wrote: >>> I've been using the following in my tree. Still need to try and get >>> this upstream. >> >> Please. If that doesn't work, I think we need to do it in configure, >> sanitizer_internal_defs.h seems to be included very early, so there >> is no easy header to override say in include/system/ tree. > > done now as https://reviews.llvm.org/D41346. Together with the revised > version of https://reviews.llvm.org/D39888 this has restored Darwin 11 > build and sanitizer testing for me for quite some time. both patches have now been approved upstream and the second one was installed already. Bootstrapped on x86_64-apple-darwin11.4.2. Is this ok now for mainline? Thanks. Rainer
On Sat, Jan 13, 2018 at 05:02:49PM +0100, Rainer Orth wrote: > Hi Jakub, > > >> On Mon, Dec 18, 2017 at 11:21:35AM +0100, Rainer Orth wrote: > >>> I've been using the following in my tree. Still need to try and get > >>> this upstream. > >> > >> Please. If that doesn't work, I think we need to do it in configure, > >> sanitizer_internal_defs.h seems to be included very early, so there > >> is no easy header to override say in include/system/ tree. > > > > done now as https://reviews.llvm.org/D41346. Together with the revised > > version of https://reviews.llvm.org/D39888 this has restored Darwin 11 > > build and sanitizer testing for me for quite some time. > > both patches have now been approved upstream and the second one was > installed already. Bootstrapped on x86_64-apple-darwin11.4.2. Is this > ok now for mainline? > > Thanks. > Rainer > > -- > ----------------------------------------------------------------------------- > Rainer Orth, Center for Biotechnology, Bielefeld University > > > 2018-01-13 Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> > > PR sanitizer/82824 > * lsan/lsan_common_mac.cc: Cherry-pick upstream r322437. Ok, thanks. Jakub
# HG changeset patch # Parent eaaab03d905888004baa739a681a239d4b4e3d29 Fix Mac OS X 10.7 bootstrap diff --git a/libsanitizer/asan/Makefile.am b/libsanitizer/asan/Makefile.am --- a/libsanitizer/asan/Makefile.am +++ b/libsanitizer/asan/Makefile.am @@ -7,6 +7,12 @@ DEFS = -D_GNU_SOURCE -D_DEBUG -D__STDC_C if USING_MAC_INTERPOSE DEFS += -DMAC_INTERPOSE_FUNCTIONS -DMISSING_BLOCKS_SUPPORT endif +if !LSAN_COMMON_SUPPORTED +DEFS += -DCAN_SANITIZE_LEAKS=0 +endif +if !WEAK_HOOKS_SUPPORT +DEFS += -DSANITIZER_SUPPORTS_WEAK_HOOKS=0 +endif AM_CXXFLAGS = -Wall -W -Wno-unused-parameter -Wwrite-strings -pedantic -Wno-long-long -fPIC -fno-builtin -fno-exceptions -fno-rtti -fomit-frame-pointer -funwind-tables -fvisibility=hidden -Wno-variadic-macros -fno-ipa-icf AM_CXXFLAGS += $(LIBSTDCXX_RAW_CXX_CXXFLAGS) AM_CXXFLAGS += -std=gnu++11 diff --git a/libsanitizer/configure.ac b/libsanitizer/configure.ac --- a/libsanitizer/configure.ac +++ b/libsanitizer/configure.ac @@ -140,6 +140,24 @@ case "$host" in esac AM_CONDITIONAL(USING_MAC_INTERPOSE, $MAC_INTERPOSE) +# lsan_common_mac.cc needs VM_MEMORY_OS_ALLOC_ONCE which was only +# introduced in Mac OS X 10.9/Darwin 13. +case "$host" in + *-*-darwin[[1-9]] | *-*-darwin[[1-9]].* | *-*-darwin1[[0-2]]*) + LSAN_COMMON=false ;; + *) LSAN_COMMON=true ;; +esac +AM_CONDITIONAL(LSAN_COMMON_SUPPORTED, $LSAN_COMMON) + +# Before Xcode 4.5, the Darwin linker doesn't properly support undefined +# weak symbols. +case "$host" in + *-*-darwin[[1-9]] | *-*-darwin[[1-9]].* | *-*-darwin1[[0-2]]*) + WEAK_HOOKS=false ;; + *) WEAK_HOOKS=true ;; +esac +AM_CONDITIONAL(WEAK_HOOKS_SUPPORT, $WEAK_HOOKS) + backtrace_supported=yes AC_MSG_CHECKING([for necessary platform features]) diff --git a/libsanitizer/lsan/Makefile.am b/libsanitizer/lsan/Makefile.am --- a/libsanitizer/lsan/Makefile.am +++ b/libsanitizer/lsan/Makefile.am @@ -4,6 +4,12 @@ AM_CPPFLAGS = -I $(top_srcdir)/include - gcc_version := $(shell @get_gcc_base_ver@ $(top_srcdir)/../gcc/BASE-VER) DEFS = -D_GNU_SOURCE -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS +if !LSAN_COMMON_SUPPORTED +DEFS += -DCAN_SANITIZE_LEAKS=0 +endif +if !WEAK_HOOKS_SUPPORT +DEFS += -DSANITIZER_SUPPORTS_WEAK_HOOKS=0 +endif AM_CXXFLAGS = -Wall -W -Wno-unused-parameter -Wwrite-strings -pedantic -Wno-long-long -fPIC -fno-builtin -fno-exceptions -fno-rtti -fomit-frame-pointer -funwind-tables -fvisibility=hidden -Wno-variadic-macros AM_CXXFLAGS += $(LIBSTDCXX_RAW_CXX_CXXFLAGS) AM_CXXFLAGS += -std=gnu++11 diff --git a/libsanitizer/lsan/lsan_common.h b/libsanitizer/lsan/lsan_common.h --- a/libsanitizer/lsan/lsan_common.h +++ b/libsanitizer/lsan/lsan_common.h @@ -20,6 +20,7 @@ #include "sanitizer_common/sanitizer_stoptheworld.h" #include "sanitizer_common/sanitizer_symbolizer.h" +#ifndef CAN_SANITIZE_LEAKS // LeakSanitizer relies on some Glibc's internals (e.g. TLS machinery) thus // supported for Linux only. Also, LSan doesn't like 32 bit architectures // because of "small" (4 bytes) pointer size that leads to high false negative @@ -42,6 +43,7 @@ #else #define CAN_SANITIZE_LEAKS 0 #endif +#endif // CAN_SANITIZE_LEAKS namespace __sanitizer { class FlagParser; diff --git a/libsanitizer/sanitizer_common/Makefile.am b/libsanitizer/sanitizer_common/Makefile.am --- a/libsanitizer/sanitizer_common/Makefile.am +++ b/libsanitizer/sanitizer_common/Makefile.am @@ -4,6 +4,9 @@ AM_CPPFLAGS = -I $(top_srcdir)/include - gcc_version := $(shell @get_gcc_base_ver@ $(top_srcdir)/../gcc/BASE-VER) DEFS = -D_GNU_SOURCE -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS @RPC_DEFS@ +if !WEAK_HOOKS_SUPPORT +DEFS += -DSANITIZER_SUPPORTS_WEAK_HOOKS=0 +endif AM_CXXFLAGS = -Wall -W -Wno-unused-parameter -Wwrite-strings -pedantic -Wno-long-long -fPIC -fno-builtin -fno-exceptions -fno-rtti -fomit-frame-pointer -funwind-tables -fvisibility=hidden -Wno-variadic-macros AM_CXXFLAGS += $(LIBSTDCXX_RAW_CXX_CXXFLAGS) AM_CXXFLAGS += -std=gnu++11
Mac OS X 10.7 bootstrap is currently broken in libsanitizer since lsan/lsan_common_mac.cc uses VM_MEMORY_OS_ALLOC_ONCE which was only introduced in Mac OS X 10.9. Upstream rejected the trivial patch to make the use conditional since that's likely to be unreliable: https://reviews.llvm.org/D39888 Given that the macro is used in a file that's compiled even when lsan isn't supported (as on Mac OS X, probably due to lsan's use of the blocks extension that gcc doesn't support), the reasonable solution seems to be to allow predefining CAN_SANITIZE_LEAKS in lsan/lsan_common.h and disable that in the build system so the affected files are effectively empty. When I tried to do this within LLVM's build system, I ran into so many road blocks with building LLVM trunk on Mac OS X 10.7 (and 10.13) with gcc that I gave up. Perhaps the two-line change to lsan/lsan_common.h can still go upstream, otherwise it would have to be gcc-local like similar code for CAN_SANITIZE_UB in ubsan/ubsan_platform.h. The rest of the patch is gcc-private anyway since it only affects the build system. It consists of two parts: * Defining CAN_SANITIZE_LEAKS=0 for affected Mac OS X versions (everything before Darwin 13/Mac OS X 10.9). * This is not enough, however: before Xcode 4.5, the Darwin ld cannot handle undefined weak symbols which are now assumed to be supported on Darwin since the last libsanitizer import from upstream. Fortunately, one can define SANITIZER_SUPPORTS_WEAK_HOOKS=0 to avoid this, which the patch does as well, again by a OS version test since a proper configure test is probably too involved. Tested by rebuilding libsanitizer on x86_64-apple-darwin11.4.2 and running the {asan,tsan,ubsan}.exp tests for both multilibs: testresults are now en par with Darwin 17. I've also bootstrapped the test on x86_64-pc-linux-gnu to check that neither defines above are enabled. Ok for mainline? Rainer