[libcxx] r250235 - [libcxx] Capture configuration information when installing the libc++ headers
Eric Fiselier via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 13 12:15:28 PDT 2017
On Wed, Sep 13, 2017 at 1:02 PM, Richard Smith <richard at metafoo.co.uk>
wrote:
> On 13 October 2015 at 15:12, Eric Fiselier via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> Author: ericwf
>> Date: Tue Oct 13 17:12:02 2015
>> New Revision: 250235
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=250235&view=rev
>> Log:
>> [libcxx] Capture configuration information when installing the libc++
>> headers
>>
>> Summary:
>> Hi all,
>>
>> This patch is a successor to D11963. However it has changed dramatically
>> and I felt it would be best to start a new review thread.
>>
>> Please read the design documentation added in this patch for a
>> description of how it works.
>>
>> Reviewers: mclow.lists, danalbert, jroelofs, EricWF
>>
>> Subscribers: vkalintiris, rnk, ed, espositofulvio, asl, eugenis,
>> cfe-commits
>>
>> Differential Revision: http://reviews.llvm.org/D13407
>>
>> Added:
>> libcxx/trunk/docs/DesignDocs/
>> libcxx/trunk/docs/DesignDocs/CapturingConfigInfo.rst
>> libcxx/trunk/include/__config_site.in
>> Modified:
>> libcxx/trunk/CMakeLists.txt
>> libcxx/trunk/cmake/Modules/HandleLibcxxFlags.cmake
>> libcxx/trunk/docs/index.rst
>> libcxx/trunk/include/CMakeLists.txt
>>
>> Modified: libcxx/trunk/CMakeLists.txt
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/CMakeLists.
>> txt?rev=250235&r1=250234&r2=250235&view=diff
>> ============================================================
>> ==================
>> --- libcxx/trunk/CMakeLists.txt (original)
>> +++ libcxx/trunk/CMakeLists.txt Tue Oct 13 17:12:02 2015
>> @@ -260,13 +260,6 @@ endif()
>>
>> # Feature flags ==============================
>> =================================
>> define_if(MSVC -D_CRT_SECURE_NO_WARNINGS)
>> -define_if_not(LIBCXX_ENABLE_GLOBAL_FILESYSTEM_NAMESPACE
>> -D_LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE)
>> -define_if_not(LIBCXX_ENABLE_STDIN -D_LIBCPP_HAS_NO_STDIN)
>> -define_if_not(LIBCXX_ENABLE_STDOUT -D_LIBCPP_HAS_NO_STDOUT)
>> -define_if_not(LIBCXX_ENABLE_THREADS -D_LIBCPP_HAS_NO_THREADS)
>> -define_if_not(LIBCXX_ENABLE_MONOTONIC_CLOCK
>> -D_LIBCPP_HAS_NO_MONOTONIC_CLOCK)
>> -define_if_not(LIBCXX_ENABLE_THREAD_UNSAFE_C_FUNCTIONS
>> -D_LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS)
>> -
>>
>> # Sanitizer flags ==============================
>> ===============================
>>
>> @@ -303,6 +296,22 @@ if (LIBCXX_BUILT_STANDALONE)
>> message(WARNING "LLVM_USE_SANITIZER is not supported on this
>> platform.")
>> endif()
>> endif()
>> +
>> +# Configuration file flags ==============================
>> =======================
>> +config_define_if_not(LIBCXX_ENABLE_GLOBAL_FILESYSTEM_NAMESPACE
>> _LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE)
>> +config_define_if_not(LIBCXX_ENABLE_STDIN _LIBCPP_HAS_NO_STDIN)
>> +config_define_if_not(LIBCXX_ENABLE_STDOUT _LIBCPP_HAS_NO_STDOUT)
>> +config_define_if_not(LIBCXX_ENABLE_THREADS _LIBCPP_HAS_NO_THREADS)
>> +config_define_if_not(LIBCXX_ENABLE_MONOTONIC_CLOCK
>> _LIBCPP_HAS_NO_MONOTONIC_CLOCK)
>> +config_define_if_not(LIBCXX_ENABLE_THREAD_UNSAFE_C_FUNCTIONS
>> _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS)
>> +
>> +if (LIBCXX_NEEDS_SITE_CONFIG)
>> + configure_file(
>> + include/__config_site.in
>> + ${LIBCXX_BINARY_DIR}/__config_site
>> + @ONLY)
>> +endif()
>> +
>> #==========================================================
>> =====================
>> # Setup Source Code And Tests
>> #==========================================================
>> =====================
>>
>> Modified: libcxx/trunk/cmake/Modules/HandleLibcxxFlags.cmake
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/cmake/Modul
>> es/HandleLibcxxFlags.cmake?rev=250235&r1=250234&r2=250235&view=diff
>> ============================================================
>> ==================
>> --- libcxx/trunk/cmake/Modules/HandleLibcxxFlags.cmake (original)
>> +++ libcxx/trunk/cmake/Modules/HandleLibcxxFlags.cmake Tue Oct 13
>> 17:12:02 2015
>> @@ -49,6 +49,22 @@ macro(define_if_not condition def)
>> endif()
>> endmacro()
>>
>> +macro(config_define_if condition def)
>> + if (${condition})
>> + set(${def} ON)
>> + add_definitions(-D${def})
>> + set(LIBCXX_NEEDS_SITE_CONFIG ON)
>> + endif()
>> +endmacro()
>> +
>> +macro(config_define_if_not condition def)
>> + if (NOT ${condition})
>> + set(${def} ON)
>> + add_definitions(-D${def})
>> + set(LIBCXX_NEEDS_SITE_CONFIG ON)
>> + endif()
>> +endmacro()
>> +
>> # Add a specified list of flags to both 'LIBCXX_COMPILE_FLAGS' and
>> # 'LIBCXX_LINK_FLAGS'.
>> macro(add_flags)
>>
>> Added: libcxx/trunk/docs/DesignDocs/CapturingConfigInfo.rst
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/docs/Design
>> Docs/CapturingConfigInfo.rst?rev=250235&view=auto
>> ============================================================
>> ==================
>> --- libcxx/trunk/docs/DesignDocs/CapturingConfigInfo.rst (added)
>> +++ libcxx/trunk/docs/DesignDocs/CapturingConfigInfo.rst Tue Oct 13
>> 17:12:02 2015
>> @@ -0,0 +1,88 @@
>> +=======================================================
>> +Capturing configuration information during installation
>> +=======================================================
>> +
>> +.. contents::
>> + :local:
>> +
>> +The Problem
>> +===========
>> +
>> +Currently the libc++ supports building the library with a number of
>> different
>> +configuration options. Unfortunately all of that configuration
>> information is
>> +lost when libc++ is installed. In order to support "persistent"
>> +configurations libc++ needs a mechanism to capture the configuration
>> options
>> +in the INSTALLED headers.
>> +
>> +
>> +Design Goals
>> +============
>> +
>> +* The solution should not INSTALL any additional headers. We don't want
>> an extra
>> + #include slowing everybody down.
>> +
>> +* The solution should not unduly affect libc++ developers. The problem
>> is limited
>> + to installed versions of libc++ and the solution should be as well.
>> +
>> +* The solution should not modify any existing headers EXCEPT during
>> installation.
>> + It makes developers lives harder if they have to regenerate the libc++
>> headers
>> + every time they are modified.
>> +
>> +* The solution should not make any of the libc++ headers dependant on
>> + files generated by the build system. The headers should be able to
>> compile
>> + out of the box without any modification.
>> +
>> +* The solution should not have ANY effect on users who don't need special
>> + configuration options. The vast majority of users will never need this
>> so it
>> + shouldn't cost them.
>> +
>> +
>> +The Solution
>> +============
>> +
>> +When you first configure libc++ using CMake we check to see if we need to
>> +capture any options. If we haven't been given any "persistent" options
>> then
>> +we do NOTHING.
>> +
>> +Otherwise we create a custom installation rule that modifies the
>> installed __config
>> +header. The rule first generates a dummy "__config_site" header
>> containing the required
>> +#defines. The contents of the dummy header are then prependend to the
>> installed
>> +__config header. By manually prepending the files we avoid the cost of an
>> +extra #include and we allow the __config header to be ignorant of the
>> extra
>> + configuration all together. An example "__config" header generated when
>> +-DLIBCXX_ENABLE_THREADS=OFF is given to CMake would look something like:
>> +
>> +.. code-block:: cpp
>> +
>> + //===-------------------------------------------------------
>> ---------------===//
>> + //
>> + // The LLVM Compiler Infrastructure
>> + //
>> + // This file is dual licensed under the MIT and the University of
>> Illinois Open
>> + // Source Licenses. See LICENSE.TXT for details.
>> + //
>> + //===-------------------------------------------------------
>> ---------------===//
>> +
>> + #ifndef _LIBCPP_CONFIG_SITE
>> + #define _LIBCPP_CONFIG_SITE
>> +
>> + /* #undef _LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE */
>> + /* #undef _LIBCPP_HAS_NO_STDIN */
>> + /* #undef _LIBCPP_HAS_NO_STDOUT */
>> + #define _LIBCPP_HAS_NO_THREADS
>> + /* #undef _LIBCPP_HAS_NO_MONOTONIC_CLOCK */
>> + /* #undef _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS */
>> +
>> + #endif
>> + // -*- C++ -*-
>> + //===--------------------------- __config
>> ---------------------------------===//
>> + //
>> + // The LLVM Compiler Infrastructure
>> + //
>> + // This file is dual licensed under the MIT and the University of
>> Illinois Open
>> + // Source Licenses. See LICENSE.TXT for details.
>> + //
>> + //===-------------------------------------------------------
>> ---------------===//
>> +
>> + #ifndef _LIBCPP_CONFIG
>> + #define _LIBCPP_CONFIG
>>
>> Modified: libcxx/trunk/docs/index.rst
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/docs/index.
>> rst?rev=250235&r1=250234&r2=250235&view=diff
>> ============================================================
>> ==================
>> --- libcxx/trunk/docs/index.rst (original)
>> +++ libcxx/trunk/docs/index.rst Tue Oct 13 17:12:02 2015
>> @@ -124,6 +124,12 @@ A full list of currently open libc++ bug
>> Design Documents
>> ----------------
>>
>> +.. toctree::
>> + :maxdepth: 1
>> +
>> + DesignDocs/CapturingConfigInfo
>> +
>> +
>> * `<atomic> design <http://libcxx.llvm.org/atomic_design.html>`_
>> * `<type_traits> design <http://libcxx.llvm.org/type_traits_design.html
>> >`_
>> * `Status of debug mode <http://libcxx.llvm.org/debug_mode.html>`_
>>
>> Modified: libcxx/trunk/include/CMakeLists.txt
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/CMa
>> keLists.txt?rev=250235&r1=250234&r2=250235&view=diff
>> ============================================================
>> ==================
>> --- libcxx/trunk/include/CMakeLists.txt (original)
>> +++ libcxx/trunk/include/CMakeLists.txt Tue Oct 13 17:12:02 2015
>> @@ -1,10 +1,12 @@
>> if (NOT LIBCXX_INSTALL_SUPPORT_HEADERS)
>> set(LIBCXX_SUPPORT_HEADER_PATTERN PATTERN "support" EXCLUDE)
>> endif()
>> +
>> set(LIBCXX_HEADER_PATTERN
>> PATTERN "*"
>> PATTERN "CMakeLists.txt" EXCLUDE
>> PATTERN ".svn" EXCLUDE
>> + PATTERN "__config_site.in" EXCLUDE
>> ${LIBCXX_SUPPORT_HEADER_PATTERN}
>> )
>>
>> @@ -22,4 +24,29 @@ if (LIBCXX_INSTALL_HEADERS)
>> ${LIBCXX_HEADER_PATTERN}
>> PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
>> )
>> +
>> + if (LIBCXX_NEEDS_SITE_CONFIG)
>> + set(UNIX_CAT cat)
>> + if (WIN32)
>> + set(UNIX_CAT type)
>> + endif()
>> + # Generate and install a custom __config header. The new header is
>> created
>> + # by prepending __config_site to the current __config header.
>> + add_custom_command(OUTPUT ${LIBCXX_BINARY_DIR}/__generated_config
>> + COMMAND ${CMAKE_COMMAND} -E copy ${LIBCXX_BINARY_DIR}/__config_site
>> ${LIBCXX_BINARY_DIR}/__generated_config
>> + COMMAND ${UNIX_CAT} ${LIBCXX_SOURCE_DIR}/include/__config >>
>> ${LIBCXX_BINARY_DIR}/__generated_config
>> + DEPENDS ${LIBCXX_SOURCE_DIR}/include/__config
>> + ${LIBCXX_BINARY_DIR}/__config_site
>> + )
>>
>
> Hi Eric, sorry for the very late review comments!
>
> This seems like a bad approach for dealing with configuration.
> Concatenating multiple files into a single header like this breaks include
> guard detection; for a file like __config, which is fairly long and
> intended to be included dozens of times into each compilation, that's
> particularly bad. It also means that sharing a libc++ installation across
> configurations requires duplicating the entire __config file, not just the
> parts that actually change between configurations.
>
Ouch, breaking the include guard detection really sucks. I'll make sure
that gets fixed.
I'm not too concerned about duplicating the `__config` file across
configurations. For the most part __config_site options are ABI breaking,
so changing any of the options typically means requiring a new dylib. Where
do you envision this use case occurring?
>
> As an alternative, how about this:
>
> Provide a dummy __config_site in the svn include directory, and make
> __config unconditionally #include __config_site. On install, configure the
> __config_site.in file to produce an installed __config_site rather than
> copying the dummy one.
>
> Would that address your needs?
>
>
That would. That solution was initially rejected to avoid the cost of
adding an extra include (You know; the whole libc++ uses monolithic headers
philosophy). I've never actually produced a decent test that demonstrates
if this cost is real or imagined, but it is clear now that it can't be
worse that breaking include guard detection. Thoughts?
@Marshall does this sound good to you?
This solution would be much simpler and cleaner in the end.
> + # Add a target that executes the generation commands.
>> + add_custom_target(generate_config_header ALL
>> + DEPENDS ${LIBCXX_BINARY_DIR}/__generated_config)
>> + # Install the generated header as __config.
>> + install(FILES ${LIBCXX_BINARY_DIR}/__generated_config
>> + DESTINATION include/c++/v1
>> + PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
>> + RENAME __config
>> + COMPONENT libcxx)
>> + endif()
>> +
>> endif()
>>
>> Added: libcxx/trunk/include/__config_site.in
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__
>> config_site.in?rev=250235&view=auto
>> ============================================================
>> ==================
>> --- libcxx/trunk/include/__config_site.in (added)
>> +++ libcxx/trunk/include/__config_site.in Tue Oct 13 17:12:02 2015
>> @@ -0,0 +1,20 @@
>> +//===------------------------------------------------------
>> ----------------===//
>> +//
>> +// The LLVM Compiler Infrastructure
>> +//
>> +// This file is dual licensed under the MIT and the University of
>> Illinois Open
>> +// Source Licenses. See LICENSE.TXT for details.
>> +//
>> +//===------------------------------------------------------
>> ----------------===//
>> +
>> +#ifndef _LIBCPP_CONFIG_SITE
>> +#define _LIBCPP_CONFIG_SITE
>> +
>> +#cmakedefine _LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE
>> +#cmakedefine _LIBCPP_HAS_NO_STDIN
>> +#cmakedefine _LIBCPP_HAS_NO_STDOUT
>> +#cmakedefine _LIBCPP_HAS_NO_THREADS
>> +#cmakedefine _LIBCPP_HAS_NO_MONOTONIC_CLOCK
>> +#cmakedefine _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS
>> +
>> +#endif
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170913/319ca4b1/attachment-0001.html>
More information about the cfe-commits
mailing list