On Wed, Sep 13, 2017 at 1:02 PM, Richard Smith <rich...@metafoo.co.uk> wrote:
> On 13 October 2015 at 15:12, Eric Fiselier via cfe-commits < > cfe-commits@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@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
