This is an archive of the discontinued LLVM Phabricator instance.

Differential D12644

Using -isysroot on Apple platform
Needs ReviewPublic

Authored by steven_wu on Sep 4 2015, 1:41 PM.

Details

Reviewers
danalbert
Summary

--sysroot might cause inconsistency between the header gets used and the
library gets linked on Apple platform. Use -isysroot instead.

Diff Detail

Event Timeline

steven_wu updated this revision to Diff 34067.Sep 4 2015, 1:41 PM
steven_wu retitled this revision from to Using -isysroot on Apple platform.
steven_wu updated this object.
steven_wu added a reviewer: EricWF.
steven_wu added a subscriber: cfe-commits.
EricWF added a reviewer: danalbert.Sep 4 2015, 1:50 PM
EricWF added a subscriber: danalbert.

@danalbert actually uses this option. I would like him to confirm this change.

steven_wu updated this revision to Diff 34073.Sep 4 2015, 1:54 PM
steven_wu edited edge metadata.

Sorry, made a typo on the previous patch.

A little background information. When the greendragon jenkins make an
system upgrade in the future, it will need this flag as well to build and
test libcxx through cmake/ninja.

EricWF added a comment.Sep 7 2015, 4:50 PM

Why would we not use "-isysroot" on other platforms as well?

steven_wu added a comment.Sep 7 2015, 7:00 PM

That works as well. I am not sure if there is any specific reason for using "--sysroot" so I proposed a safer fix.

EricWF added a comment.Oct 6 2015, 5:21 PM

You need to use the CMake option CMAKE_OSX_SYSROOT to manage the system root because lib/CMakeLists.txt uses "CMAKE_OSX_SYSROOT" to find the correct libc++abi.dylib.
Have you tried that? It is unfortunate that Linux and OS X use different options for the same thing though. Maybe we could set CMAKE_OSX_SYSROOT internally based on LIBCXX_SYSROOT?

EricWF resigned from this revision.Jan 4 2017, 10:18 PM
EricWF removed a reviewer: EricWF.

I'm resigning as a reviewer. I suggested an alternative fix and explained why this fix wasn't correct in a comment above.

Please re-add me as a reviewer if you disagree with my analysis.

Revision Contents

PathSize
6 lines

Diff 34073

CMakeLists.txt

Show First 20 Lines • Show All 203 Lines • ▼ Show 20 Lines
# MSVC only has -X, which disables all default includes; including the crt. # MSVC only has -X, which disables all default includes; including the crt.
# Thus, we do nothing and hope we don't accidentally include any of the C++ # Thus, we do nothing and hope we don't accidentally include any of the C++
# headers # headers
add_compile_flags_if_supported(-nostdinc++) add_compile_flags_if_supported(-nostdinc++)
# Target flags ================================================================ # Target flags ================================================================
add_flags_if(LIBCXX_BUILD_32_BITS -m32) add_flags_if(LIBCXX_BUILD_32_BITS -m32)
add_flags_if(LIBCXX_TARGET_TRIPLE "-target ${LIBCXX_TARGET_TRIPLE}") add_flags_if(LIBCXX_TARGET_TRIPLE "-target ${LIBCXX_TARGET_TRIPLE}")
if (APPLE)
add_flags_if(LIBCXX_SYSROOT "-isysroot ${LIBCXX_SYSROOT}")
else()
add_flags_if(LIBCXX_SYSROOT "--sysroot ${LIBCXX_SYSROOT}") add_flags_if(LIBCXX_SYSROOT "--sysroot ${LIBCXX_SYSROOT}")
endif()
add_flags_if(LIBCXX_GCC_TOOLCHAIN "-gcc-toolchain ${LIBCXX_GCC_TOOLCHAIN}") add_flags_if(LIBCXX_GCC_TOOLCHAIN "-gcc-toolchain ${LIBCXX_GCC_TOOLCHAIN}")
# Warning flags =============================================================== # Warning flags ===============================================================
add_definitions(-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER) add_definitions(-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
add_compile_flags_if_supported( add_compile_flags_if_supported(
-Wall -W -Wwrite-strings -Wall -W -Wwrite-strings
-Wno-unused-parameter -Wno-long-long -Wno-unused-parameter -Wno-long-long
-Werror=return-type) -Werror=return-type)
▲ Show 20 Lines • Show All 96 Lines • Show Last 20 Lines