On 13 September 2017 at 12:15, Eric Fiselier via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> 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? > What I had in mind was a multilib system that shares the include/ directory (other than a few factored-out target-specific files) across all architectures and configurations, and uses (say) different libc++ ABIs for different architectures (and, naturally, different dylibs). That said, having a different __config for each such target is really only a minor problem. It's not like we want to support people using the same __config_site for different libc++ versions, so the fact that it contains libc++ source code in addition to config settings is a nuisance at worst. 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/__c >>> onfig_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 > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
