[libcxx] r260235 - Introduce a cmake module to figure out whether we need to link with libatomic.
Vasileios Kalintiris via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 16 14:06:46 PST 2016
Hi Ben,
> FYI, this change and the LLVM version have each broken my libcxx
> builds. I cross compile from Linux x64 to Hexagon, and my host machine
> doesn't have <atomic>. The LLVM change was particularly offensive
> because I wasn't planning on building LLVM itself.
There's no change on the LLVM side, ie. I abandoned the review request you have in mind after discussion with the reviewers.
> I have since worked around the issue by making my cmake invocation longer.
>
> I'm not sure what you can do for the LLVM check, but if you can use the
> libcxx headers directly for the <atomic> check, while still using all
> the custom flags and tools that I pass on the command line, then I
> should be able to remove the explicit setting of
> "LIBCXX_HAVE_CXX_ATOMICS_WITHOUT_LIB" in the future.
You shouldn't have any problems with the latest commit (r260961): "Issue a warning instead of fatal errors when checks for libatomic fail." I suppose that you aren't using the latest trunk, right?
- Vasileios
________________________________________
From: cfe-commits [cfe-commits-bounces at lists.llvm.org] on behalf of Craig, Ben via cfe-commits [cfe-commits at lists.llvm.org]
Sent: 16 February 2016 21:09
To: cfe-commits at lists.llvm.org
Subject: Re: [libcxx] r260235 - Introduce a cmake module to figure out whether we need to link with libatomic.
FYI, this change and the LLVM version have each broken my libcxx
builds. I cross compile from Linux x64 to Hexagon, and my host machine
doesn't have <atomic>. The LLVM change was particularly offensive
because I wasn't planning on building LLVM itself.
I have since worked around the issue by making my cmake invocation longer.
I'm not sure what you can do for the LLVM check, but if you can use the
libcxx headers directly for the <atomic> check, while still using all
the custom flags and tools that I pass on the command line, then I
should be able to remove the explicit setting of
"LIBCXX_HAVE_CXX_ATOMICS_WITHOUT_LIB" in the future.
On 2/16/2016 2:09 PM, Vasileios Kalintiris via cfe-commits wrote:
> Hi Hans,
>
>> Or is this comment on PR26059 all that needs to be done for 3.8?
> That's correct. I wrote that comment in order to clarify which bits we should merge in 3.8.
>
> The latest commit:
>
> - [libcxx] r260961 - Issue a warning instead of fatal errors when checks for libatomic fail."
>
> makes sure that my changes don't break older configurations that used to work previously, by emitting a warning message instead of a fatal error.
>
> I had to turn the error to warning because there are cases where the host compiler doesn't provide the <cstdint> and <atomic> headers when bootstrapping llvm.
>
> The correct solution would be to use the headers provided from libc++ itself. However, I wanted to take the safe route for the 3.8 branch and provide the elegant solution on the trunk after having the proper discussion with the libc++ devs.
>
> Thanks,
> Vasileios
>
> ________________________________________
> From: hwennborg at google.com [hwennborg at google.com] on behalf of Hans Wennborg [hans at chromium.org]
> Sent: 16 February 2016 19:53
> To: Vasileios Kalintiris
> Cc: Eric Fiselier; Daniel Sanders; mclow.lists at gmail.com; cfe-commits at lists.llvm.org
> Subject: Re: [libcxx] r260235 - Introduce a cmake module to figure out whether we need to link with libatomic.
>
> Or is this comment on PR26059 all that needs to be done for 3.8?
>
> "Bug 26369, which has been fixed with r260961, requires the following
> commits to get merged on the release branch:
>
> - [libcxx] r260515 - Re-commit "Introduce a cmake module to figure out
> whether we need to link with libatomic."
> - [libcxx] r260524 - Fix r260515 - Correct typos in CMake changes
> - [libcxx] r260531 - Rename CheckLibcxxAtomic.cmake variable result
> names so they don't clash with LLVM
> - [libcxx] r260961 - Issue a warning instead of fatal errors when
> checks for libatomic fail."
>
> Thanks,
> Hans
>
> On Tue, Feb 16, 2016 at 11:47 AM, Hans Wennborg <hans at chromium.org> wrote:
>> Do I understand correctly that there are still issues here's that are not fixed?
>>
>> (I'm trying to understand if there is something here to merge for 3.8,
>> but I'm having trouble following these commits.)
>>
>> On Tue, Feb 16, 2016 at 6:44 AM, Vasileios Kalintiris
>> <Vasileios.Kalintiris at imgtec.com> wrote:
>>> I changed the type of message from fatal_error to warning in r260961. While
>>> the test for atomics works fine in most cases, it fails because we include
>>> <cstdint> and <atomic>, and the user's host compiler might not provide them
>>> during a bootstrap (see PR26631 and PR26622).
>>>
>>> Does anyone have any idea how to tackle this problem? As suggested by the
>>> bug reports, we could include the headers provided by libc++. However, I'm
>>> not sure whether we are supposed to do that during configuration time.
>>>
>>> - Vasileios
>>>
>>>
>>> ________________________________
>>> From: Eric Fiselier [eric at efcs.ca]
>>> Sent: 11 February 2016 16:08
>>> To: Daniel Sanders
>>> Cc: Vasileios Kalintiris; hans at chromium.org; mclow.lists at gmail.com;
>>> cfe-commits at lists.llvm.org
>>> Subject: Re: [libcxx] r260235 - Introduce a cmake module to figure out
>>> whether we need to link with libatomic.
>>>
>>>> we need to rename HAVE_CXX_ATOMICS_WITH_LIB to something like
>>>> LIBCXX_HAVE_CXX_ATOMICS_WITH_LIB.
>>> Fixed in r260531.
>>>
>>> I think we will eventually want to merge the following commits, assuming
>>> they don't regress the build, especially with the -gcc-toolchain option.
>>>
>>> - [libcxx] r260515 - Re-commit "Introduce a cmake module to figure out
>>> whether we need to link with libatomic."
>>> - [libcxx] r260524 - Fix r260515 - Correct typos in CMake changes
>>> - [libcxx] r260531 - Rename CheckLibcxxAtomic.cmake variable result names so
>>> they don't clash with LLVM
>>>
>>> @Marshall Any objections?
>>>
>>> On Thu, Feb 11, 2016 at 2:18 AM, Daniel Sanders via cfe-commits
>>> <cfe-commits at lists.llvm.org> wrote:
>>>> Hi,
>>>>
>>>> In my latests rc2+patches build I've also found that we need to rename
>>>> HAVE_CXX_ATOMICS_WITH_LIB to something like
>>>> LIBCXX_HAVE_CXX_ATOMICS_WITH_LIB. It's currently re-using the result of
>>>> LLVM's check which doesn't include 64-bit atomics.
>>>> ________________________________________
>>>> From: Vasileios Kalintiris
>>>> Sent: 09 February 2016 23:50
>>>> To: hans at chromium.org
>>>> Cc: cfe-commits at lists.llvm.org; Daniel Sanders; mclow.lists at gmail.com
>>>> Subject: RE: [libcxx] r260235 - Introduce a cmake module to figure out
>>>> whether we need to link with libatomic.
>>>>
>>>> Hi Hans,
>>>>
>>>> Please wait before merging this as it breaks LLVM bootstraps when using
>>>> the -gcc-toolchain option and the system's default gcc installation does not
>>>> provide libatomic. We have to check LIBCXX_GCC_TOOLCHAIN in our test. I'll
>>>> create a patch tomorrow and I'll let you know when it's okay to merge them.
>>>> In the meantime, I reverted it in r260323.
>>>>
>>>> - Vasileios
>>>> ________________________________________
>>>> From: Vasileios Kalintiris
>>>> Sent: 09 February 2016 18:56
>>>> To: hans at chromium.org
>>>> Cc: cfe-commits at lists.llvm.org; Daniel Sanders; mclow.lists at gmail.com
>>>> Subject: RE: [libcxx] r260235 - Introduce a cmake module to figure out
>>>> whether we need to link with libatomic.
>>>>
>>>> Hi Hans,
>>>>
>>>> Can we merge this on the 3.8 release branch? It fixes libcxx builds on
>>>> MIPS by linking with libatomic when needed. Also, all the x86_64 and ARM
>>>> buildbots for libcxx look good.
>>>>
>>>> Thanks,
>>>> Vasileios
>>>>
>>>> ________________________________________
>>>> From: cfe-commits [cfe-commits-bounces at lists.llvm.org] on behalf of
>>>> Vasileios Kalintiris via cfe-commits [cfe-commits at lists.llvm.org]
>>>> Sent: 09 February 2016 17:00
>>>> To: cfe-commits at lists.llvm.org
>>>> Subject: [libcxx] r260235 - Introduce a cmake module to figure out whether
>>>> we need to link with libatomic.
>>>>
>>>> Author: vkalintiris
>>>> Date: Tue Feb 9 11:00:38 2016
>>>> New Revision: 260235
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=260235&view=rev
>>>> Log:
>>>> Introduce a cmake module to figure out whether we need to link with
>>>> libatomic.
>>>>
>>>> Summary:
>>>> This fixes the tests under std/atomics for 32-bit MIPS CPUs where the
>>>> 8-byte atomic operations call into the libatomic library.
>>>>
>>>> Reviewers: dsanders, mclow.lists, EricWF, jroelofs, joerg
>>>>
>>>> Subscribers: cfe-commits
>>>>
>>>> Differential Revision: http://reviews.llvm.org/D16613
>>>>
>>>> Added:
>>>> libcxx/trunk/cmake/Modules/CheckLibcxxAtomic.cmake
>>>> Modified:
>>>> libcxx/trunk/cmake/config-ix.cmake
>>>> libcxx/trunk/lib/CMakeLists.txt
>>>> libcxx/trunk/test/CMakeLists.txt
>>>> libcxx/trunk/test/libcxx/test/target_info.py
>>>> libcxx/trunk/test/lit.site.cfg.in
>>>>
>>>> Added: libcxx/trunk/cmake/Modules/CheckLibcxxAtomic.cmake
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/cmake/Modules/CheckLibcxxAtomic.cmake?rev=260235&view=auto
>>>>
>>>> ==============================================================================
>>>> --- libcxx/trunk/cmake/Modules/CheckLibcxxAtomic.cmake (added)
>>>> +++ libcxx/trunk/cmake/Modules/CheckLibcxxAtomic.cmake Tue Feb 9 11:00:38
>>>> 2016
>>>> @@ -0,0 +1,38 @@
>>>> +INCLUDE(CheckCXXSourceCompiles)
>>>> +
>>>> +# Sometimes linking against libatomic is required for atomic ops, if
>>>> +# the platform doesn't support lock-free atomics.
>>>> +#
>>>> +# We could modify LLVM's CheckAtomic module and have it check for 64-bit
>>>> +# atomics instead. However, we would like to avoid careless uses of
>>>> 64-bit
>>>> +# atomics inside LLVM over time on 32-bit platforms.
>>>> +
>>>> +function(check_cxx_atomics varname)
>>>> + set(OLD_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS})
>>>> + set(CMAKE_REQUIRED_FLAGS "-std=c++11")
>>>> + check_cxx_source_compiles("
>>>> +#include <cstdint>
>>>> +#include <atomic>
>>>> +std::atomic<uintptr_t> x;
>>>> +std::atomic<uintmax_t> y;
>>>> +int main() {
>>>> + return x + y;
>>>> +}
>>>> +" ${varname})
>>>> + set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQUIRED_FLAGS})
>>>> +endfunction(check_cxx_atomics)
>>>> +
>>>> +check_cxx_atomics(HAVE_CXX_ATOMICS_WITHOUT_LIB)
>>>> +# If not, check if the library exists, and atomics work with it.
>>>> +if(NOT HAVE_CXX_ATOMICS_WITHOUT_LIB)
>>>> + check_library_exists(atomic __atomic_fetch_add_8 "" HAVE_LIBATOMIC)
>>>> + if(HAVE_LIBATOMIC)
>>>> + list(APPEND CMAKE_REQUIRED_LIBRARIES "atomic")
>>>> + check_cxx_atomics(HAVE_CXX_ATOMICS_WITH_LIB)
>>>> + if (NOT HAVE_CXX_ATOMICS_WITH_LIB)
>>>> + message(FATAL_ERROR "Host compiler must support std::atomic!")
>>>> + endif()
>>>> + else()
>>>> + message(FATAL_ERROR "Host compiler appears to require libatomic, but
>>>> cannot find it.")
>>>> + endif()
>>>> +endif()
>>>>
>>>> Modified: libcxx/trunk/cmake/config-ix.cmake
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/cmake/config-ix.cmake?rev=260235&r1=260234&r2=260235&view=diff
>>>>
>>>> ==============================================================================
>>>> --- libcxx/trunk/cmake/config-ix.cmake (original)
>>>> +++ libcxx/trunk/cmake/config-ix.cmake Tue Feb 9 11:00:38 2016
>>>> @@ -1,5 +1,6 @@
>>>> include(CheckLibraryExists)
>>>> include(CheckCXXCompilerFlag)
>>>> +include(CheckLibcxxAtomic)
>>>>
>>>> # Check compiler flags
>>>>
>>>> @@ -17,3 +18,7 @@ check_library_exists(c fopen "" LIBCXX_H
>>>> check_library_exists(m ccos "" LIBCXX_HAS_M_LIB)
>>>> check_library_exists(rt clock_gettime "" LIBCXX_HAS_RT_LIB)
>>>> check_library_exists(gcc_s __gcc_personality_v0 "" LIBCXX_HAS_GCC_S_LIB)
>>>> +
>>>> +if (NOT HAVE_CXX_ATOMICS_WITHOUT_LIB)
>>>> + set(LIBCXX_HAS_ATOMIC_LIB True)
>>>> +endif()
>>>>
>>>> Modified: libcxx/trunk/lib/CMakeLists.txt
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/lib/CMakeLists.txt?rev=260235&r1=260234&r2=260235&view=diff
>>>>
>>>> ==============================================================================
>>>> --- libcxx/trunk/lib/CMakeLists.txt (original)
>>>> +++ libcxx/trunk/lib/CMakeLists.txt Tue Feb 9 11:00:38 2016
>>>> @@ -79,6 +79,7 @@ add_library_flags_if(LIBCXX_HAS_C_LIB c)
>>>> add_library_flags_if(LIBCXX_HAS_M_LIB m)
>>>> add_library_flags_if(LIBCXX_HAS_RT_LIB rt)
>>>> add_library_flags_if(LIBCXX_HAS_GCC_S_LIB gcc_s)
>>>> +add_library_flags_if(LIBCXX_HAS_ATOMIC_LIB atomic)
>>>>
>>>> # Setup flags.
>>>> add_flags_if_supported(-fPIC)
>>>>
>>>> Modified: libcxx/trunk/test/CMakeLists.txt
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/CMakeLists.txt?rev=260235&r1=260234&r2=260235&view=diff
>>>>
>>>> ==============================================================================
>>>> --- libcxx/trunk/test/CMakeLists.txt (original)
>>>> +++ libcxx/trunk/test/CMakeLists.txt Tue Feb 9 11:00:38 2016
>>>> @@ -15,6 +15,7 @@ pythonize_bool(LIBCXX_ENABLE_SHARED)
>>>> pythonize_bool(LIBCXX_BUILD_32_BITS)
>>>> pythonize_bool(LIBCXX_GENERATE_COVERAGE)
>>>> pythonize_bool(LIBCXXABI_USE_LLVM_UNWINDER)
>>>> +pythonize_bool(LIBCXX_HAS_ATOMIC_LIB)
>>>>
>>>> # The tests shouldn't link to any ABI library when it has been linked
>>>> into
>>>> # libc++ statically or via a linker script.
>>>>
>>>> Modified: libcxx/trunk/test/libcxx/test/target_info.py
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/libcxx/test/target_info.py?rev=260235&r1=260234&r2=260235&view=diff
>>>>
>>>> ==============================================================================
>>>> --- libcxx/trunk/test/libcxx/test/target_info.py (original)
>>>> +++ libcxx/trunk/test/libcxx/test/target_info.py Tue Feb 9 11:00:38 2016
>>>> @@ -172,6 +172,9 @@ class LinuxLocalTI(DefaultTargetInfo):
>>>> flags += ['-lunwind', '-ldl']
>>>> else:
>>>> flags += ['-lgcc_s', '-lgcc']
>>>> + use_libatomic = self.full_config.get_lit_bool('use_libatomic',
>>>> False)
>>>> + if use_libatomic:
>>>> + flags += ['-latomic']
>>>> san = self.full_config.get_lit_conf('use_sanitizer', '').strip()
>>>> if san:
>>>> # The libraries and their order are taken from the
>>>>
>>>> Modified: libcxx/trunk/test/lit.site.cfg.in
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/lit.site.cfg.in?rev=260235&r1=260234&r2=260235&view=diff
>>>>
>>>> ==============================================================================
>>>> --- libcxx/trunk/test/lit.site.cfg.in (original)
>>>> +++ libcxx/trunk/test/lit.site.cfg.in Tue Feb 9 11:00:38 2016
>>>> @@ -20,6 +20,7 @@ config.generate_coverage = "@LIBC
>>>> config.target_info = "@LIBCXX_TARGET_INFO@"
>>>> config.executor = "@LIBCXX_EXECUTOR@"
>>>> config.llvm_unwinder = "@LIBCXXABI_USE_LLVM_UNWINDER@"
>>>> +config.use_libatomic = "@LIBCXX_HAS_ATOMIC_LIB@"
>>>>
>>>> # Let the main config do the real work.
>>>> lit_config.load_config(config, "@LIBCXX_SOURCE_DIR@/test/lit.cfg")
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
_______________________________________________
cfe-commits mailing list
cfe-commits at lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list