<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 13 September 2017 at 12:15, Eric Fiselier via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">On Wed, Sep 13, 2017 at 1:02 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="m_840859260227922915h5">On 13 October 2015 at 15:12, Eric Fiselier via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:</div></div><div class="gmail_extra"><div class="gmail_quote"><div><div class="m_840859260227922915h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: ericwf<br>
Date: Tue Oct 13 17:12:02 2015<br>
New Revision: 250235<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=250235&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=250235&view=rev</a><br>
Log:<br>
[libcxx] Capture configuration information when installing the libc++ headers<br>
<br>
Summary:<br>
Hi all,<br>
<br>
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.<br>
<br>
Please read the design documentation added in this patch for a description of how it works.<br>
<br>
Reviewers: mclow.lists, danalbert, jroelofs, EricWF<br>
<br>
Subscribers: vkalintiris, rnk, ed, espositofulvio, asl, eugenis, cfe-commits<br>
<br>
Differential Revision: <a href="http://reviews.llvm.org/D13407" rel="noreferrer" target="_blank">http://reviews.llvm.org/D13407</a><br>
<br>
Added:<br>
    libcxx/trunk/docs/DesignDocs/<br>
    libcxx/trunk/docs/DesignDocs/C<wbr>apturingConfigInfo.rst<br>
    libcxx/trunk/include/__<a href="http://config_site.in" rel="noreferrer" target="_blank">config_<wbr>site.in</a><br>
Modified:<br>
    libcxx/trunk/CMakeLists.txt<br>
    libcxx/trunk/cmake/Modules/Han<wbr>dleLibcxxFlags.cmake<br>
    libcxx/trunk/docs/index.rst<br>
    libcxx/trunk/include/CMakeList<wbr>s.txt<br>
<br>
Modified: libcxx/trunk/CMakeLists.txt<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/CMakeLists.txt?rev=250235&r1=250234&r2=250235&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/libcxx/trunk/CMakeLists.<wbr>txt?rev=250235&r1=250234&r2=25<wbr>0235&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- libcxx/trunk/CMakeLists.txt (original)<br>
+++ libcxx/trunk/CMakeLists.txt Tue Oct 13 17:12:02 2015<br>
@@ -260,13 +260,6 @@ endif()<br>
<br>
 # Feature flags ==============================<wbr>==============================<wbr>===<br>
 define_if(MSVC -D_CRT_SECURE_NO_WARNINGS)<br>
-define_if_not(LIBCXX_ENABLE_G<wbr>LOBAL_FILESYSTEM_NAMESPACE -D_LIBCPP_HAS_NO_GLOBAL_FILESY<wbr>STEM_NAMESPACE)<br>
-define_if_not(LIBCXX_ENABLE_S<wbr>TDIN -D_LIBCPP_HAS_NO_STDIN)<br>
-define_if_not(LIBCXX_ENABLE_S<wbr>TDOUT -D_LIBCPP_HAS_NO_STDOUT)<br>
-define_if_not(LIBCXX_ENABLE_T<wbr>HREADS -D_LIBCPP_HAS_NO_THREADS)<br>
-define_if_not(LIBCXX_ENABLE_M<wbr>ONOTONIC_CLOCK -D_LIBCPP_HAS_NO_MONOTONIC_CLO<wbr>CK)<br>
-define_if_not(LIBCXX_ENABLE_T<wbr>HREAD_UNSAFE_C_FUNCTIONS -D_LIBCPP_HAS_NO_THREAD_UNSAFE<wbr>_C_FUNCTIONS)<br>
-<br>
<br>
 # Sanitizer flags ==============================<wbr>==============================<wbr>=<br>
<br>
@@ -303,6 +296,22 @@ if (LIBCXX_BUILT_STANDALONE)<br>
     message(WARNING "LLVM_USE_SANITIZER is not supported on this platform.")<br>
   endif()<br>
 endif()<br>
+<br>
+# Configuration file flags ==============================<wbr>=======================<br>
+config_define_if_not(LIBCXX_E<wbr>NABLE_GLOBAL_FILESYSTEM_NAMESP<wbr>ACE _LIBCPP_HAS_NO_GLOBAL_FILESYST<wbr>EM_NAMESPACE)<br>
+config_define_if_not(LIBCXX_E<wbr>NABLE_STDIN _LIBCPP_HAS_NO_STDIN)<br>
+config_define_if_not(LIBCXX_E<wbr>NABLE_STDOUT _LIBCPP_HAS_NO_STDOUT)<br>
+config_define_if_not(LIBCXX_E<wbr>NABLE_THREADS _LIBCPP_HAS_NO_THREADS)<br>
+config_define_if_not(LIBCXX_E<wbr>NABLE_MONOTONIC_CLOCK _LIBCPP_HAS_NO_MONOTONIC_CLOCK<wbr>)<br>
+config_define_if_not(LIBCXX_E<wbr>NABLE_THREAD_UNSAFE_C_FUNCTION<wbr>S _LIBCPP_HAS_NO_THREAD_UNSAFE_C<wbr>_FUNCTIONS)<br>
+<br>
+if (LIBCXX_NEEDS_SITE_CONFIG)<br>
+  configure_file(<br>
+    include/__<a href="http://config_site.in" rel="noreferrer" target="_blank">config_site.in</a><br>
+    ${LIBCXX_BINARY_DIR}/__config_<wbr>site<br>
+    @ONLY)<br>
+endif()<br>
+<br>
 #============================<wbr>==============================<wbr>=====================<br>
 # Setup Source Code And Tests<br>
 #============================<wbr>==============================<wbr>=====================<br>
<br>
Modified: libcxx/trunk/cmake/Modules/Han<wbr>dleLibcxxFlags.cmake<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/cmake/Modules/HandleLibcxxFlags.cmake?rev=250235&r1=250234&r2=250235&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/libcxx/trunk/cmake/Modul<wbr>es/HandleLibcxxFlags.cmake?rev<wbr>=250235&r1=250234&r2=250235&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- libcxx/trunk/cmake/Modules/Han<wbr>dleLibcxxFlags.cmake (original)<br>
+++ libcxx/trunk/cmake/Modules/Han<wbr>dleLibcxxFlags.cmake Tue Oct 13 17:12:02 2015<br>
@@ -49,6 +49,22 @@ macro(define_if_not condition def)<br>
   endif()<br>
 endmacro()<br>
<br>
+macro(config_define_if condition def)<br>
+  if (${condition})<br>
+    set(${def} ON)<br>
+    add_definitions(-D${def})<br>
+    set(LIBCXX_NEEDS_SITE_CONFIG ON)<br>
+  endif()<br>
+endmacro()<br>
+<br>
+macro(config_define_if_not condition def)<br>
+  if (NOT ${condition})<br>
+    set(${def} ON)<br>
+    add_definitions(-D${def})<br>
+    set(LIBCXX_NEEDS_SITE_CONFIG ON)<br>
+  endif()<br>
+endmacro()<br>
+<br>
 # Add a specified list of flags to both 'LIBCXX_COMPILE_FLAGS' and<br>
 # 'LIBCXX_LINK_FLAGS'.<br>
 macro(add_flags)<br>
<br>
Added: libcxx/trunk/docs/DesignDocs/C<wbr>apturingConfigInfo.rst<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/docs/DesignDocs/CapturingConfigInfo.rst?rev=250235&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/libcxx/trunk/docs/Design<wbr>Docs/CapturingConfigInfo.rst?r<wbr>ev=250235&view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- libcxx/trunk/docs/DesignDocs/C<wbr>apturingConfigInfo.rst (added)<br>
+++ libcxx/trunk/docs/DesignDocs/C<wbr>apturingConfigInfo.rst Tue Oct 13 17:12:02 2015<br>
@@ -0,0 +1,88 @@<br>
+=============================<wbr>==========================<br>
+Capturing configuration information during installation<br>
+=============================<wbr>==========================<br>
+<br>
+.. contents::<br>
+   :local:<br>
+<br>
+The Problem<br>
+===========<br>
+<br>
+Currently the libc++ supports building the library with a number of different<br>
+configuration options.  Unfortunately all of that configuration information is<br>
+lost when libc++ is installed. In order to support "persistent"<br>
+configurations libc++ needs a mechanism to capture the configuration options<br>
+in the INSTALLED headers.<br>
+<br>
+<br>
+Design Goals<br>
+============<br>
+<br>
+* The solution should not INSTALL any additional headers. We don't want an extra<br>
+  #include slowing everybody down.<br>
+<br>
+* The solution should not unduly affect libc++ developers. The problem is limited<br>
+  to installed versions of libc++ and the solution should be as well.<br>
+<br>
+* The solution should not modify any existing headers EXCEPT during installation.<br>
+  It makes developers lives harder if they have to regenerate the libc++ headers<br>
+  every time they are modified.<br>
+<br>
+* The solution should not make any of the libc++ headers dependant on<br>
+  files generated by the build system. The headers should be able to compile<br>
+  out of the box without any modification.<br>
+<br>
+* The solution should not have ANY effect on users who don't need special<br>
+  configuration options. The vast majority of users will never need this so it<br>
+  shouldn't cost them.<br>
+<br>
+<br>
+The Solution<br>
+============<br>
+<br>
+When you first configure libc++ using CMake we check to see if we need to<br>
+capture any options. If we haven't been given any "persistent" options then<br>
+we do NOTHING.<br>
+<br>
+Otherwise we create a custom installation rule that modifies the installed __config<br>
+header. The rule first generates a dummy "__config_site" header containing the required<br>
+#defines. The contents of the dummy header are then prependend to the installed<br>
+__config header. By manually prepending the files we avoid the cost of an<br>
+extra #include and we allow the __config header to be ignorant of the extra<br>
+ configuration all together. An example "__config" header generated when<br>
+-DLIBCXX_ENABLE_THREADS=OFF is given to CMake would look something like:<br>
+<br>
+.. code-block:: cpp<br>
+<br>
+  //===-------------------------<wbr>------------------------------<wbr>---------------===//<br>
+  //<br>
+  //                     The LLVM Compiler Infrastructure<br>
+  //<br>
+  // This file is dual licensed under the MIT and the University of Illinois Open<br>
+  // Source Licenses. See LICENSE.TXT for details.<br>
+  //<br>
+  //===-------------------------<wbr>------------------------------<wbr>---------------===//<br>
+<br>
+  #ifndef _LIBCPP_CONFIG_SITE<br>
+  #define _LIBCPP_CONFIG_SITE<br>
+<br>
+  /* #undef _LIBCPP_HAS_NO_GLOBAL_FILESYST<wbr>EM_NAMESPACE */<br>
+  /* #undef _LIBCPP_HAS_NO_STDIN */<br>
+  /* #undef _LIBCPP_HAS_NO_STDOUT */<br>
+  #define _LIBCPP_HAS_NO_THREADS<br>
+  /* #undef _LIBCPP_HAS_NO_MONOTONIC_CLOCK */<br>
+  /* #undef _LIBCPP_HAS_NO_THREAD_UNSAFE_C<wbr>_FUNCTIONS */<br>
+<br>
+  #endif<br>
+  // -*- C++ -*-<br>
+  //===-------------------------<wbr>-- __config ------------------------------<wbr>---===//<br>
+  //<br>
+  //                     The LLVM Compiler Infrastructure<br>
+  //<br>
+  // This file is dual licensed under the MIT and the University of Illinois Open<br>
+  // Source Licenses. See LICENSE.TXT for details.<br>
+  //<br>
+  //===-------------------------<wbr>------------------------------<wbr>---------------===//<br>
+<br>
+  #ifndef _LIBCPP_CONFIG<br>
+  #define _LIBCPP_CONFIG<br>
<br>
Modified: libcxx/trunk/docs/index.rst<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/docs/index.rst?rev=250235&r1=250234&r2=250235&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/libcxx/trunk/docs/index.<wbr>rst?rev=250235&r1=250234&r2=25<wbr>0235&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- libcxx/trunk/docs/index.rst (original)<br>
+++ libcxx/trunk/docs/index.rst Tue Oct 13 17:12:02 2015<br>
@@ -124,6 +124,12 @@ A full list of currently open libc++ bug<br>
 Design Documents<br>
 ----------------<br>
<br>
+.. toctree::<br>
+   :maxdepth: 1<br>
+<br>
+   DesignDocs/CapturingConfigInf<wbr>o<br>
+<br>
+<br>
 * `<atomic> design <<a href="http://libcxx.llvm.org/atomic_design.html" rel="noreferrer" target="_blank">http://libcxx.llvm.org/atomic<wbr>_design.html</a>>`_<br>
 * `<type_traits> design <<a href="http://libcxx.llvm.org/type_traits_design.html" rel="noreferrer" target="_blank">http://libcxx.llvm.org/type_t<wbr>raits_design.html</a>>`_<br>
 * `Status of debug mode <<a href="http://libcxx.llvm.org/debug_mode.html" rel="noreferrer" target="_blank">http://libcxx.llvm.org/debug_<wbr>mode.html</a>>`_<br>
<br>
Modified: libcxx/trunk/include/CMakeList<wbr>s.txt<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/CMakeLists.txt?rev=250235&r1=250234&r2=250235&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/libcxx/trunk/include/CMa<wbr>keLists.txt?rev=250235&r1=2502<wbr>34&r2=250235&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- libcxx/trunk/include/CMakeList<wbr>s.txt (original)<br>
+++ libcxx/trunk/include/CMakeList<wbr>s.txt Tue Oct 13 17:12:02 2015<br>
@@ -1,10 +1,12 @@<br>
 if (NOT LIBCXX_INSTALL_SUPPORT_HEADERS<wbr>)<br>
   set(LIBCXX_SUPPORT_HEADER_PAT<wbr>TERN PATTERN "support" EXCLUDE)<br>
 endif()<br>
+<br>
 set(LIBCXX_HEADER_PATTERN<br>
   PATTERN "*"<br>
   PATTERN "CMakeLists.txt" EXCLUDE<br>
   PATTERN ".svn" EXCLUDE<br>
+  PATTERN "__<a href="http://config_site.in" rel="noreferrer" target="_blank">config_site.in</a>" EXCLUDE<br>
   ${LIBCXX_SUPPORT_HEADER_PATTE<wbr>RN}<br>
   )<br>
<br>
@@ -22,4 +24,29 @@ if (LIBCXX_INSTALL_HEADERS)<br>
     ${LIBCXX_HEADER_PATTERN}<br>
     PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ<br>
   )<br>
+<br>
+  if (LIBCXX_NEEDS_SITE_CONFIG)<br>
+    set(UNIX_CAT cat)<br>
+    if (WIN32)<br>
+      set(UNIX_CAT type)<br>
+    endif()<br>
+    # Generate and install a custom __config header. The new header is created<br>
+    # by  prepending __config_site to the current __config header.<br>
+    add_custom_command(OUTPUT ${LIBCXX_BINARY_DIR}/__generat<wbr>ed_config<br>
+      COMMAND ${CMAKE_COMMAND} -E copy ${LIBCXX_BINARY_DIR}/__config_<wbr>site ${LIBCXX_BINARY_DIR}/__generat<wbr>ed_config<br>
+      COMMAND ${UNIX_CAT} ${LIBCXX_SOURCE_DIR}/include/_<wbr>_config >> ${LIBCXX_BINARY_DIR}/__generat<wbr>ed_config<br>
+      DEPENDS ${LIBCXX_SOURCE_DIR}/include/_<wbr>_config<br>
+              ${LIBCXX_BINARY_DIR}/__config_<wbr>site<br>
+    )<br></blockquote><div><br></div></div></div><div>Hi Eric, sorry for the very late review comments!</div><div><br></div><div>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.</div></div></div></div></blockquote><div><br></div></div></div><div>Ouch, breaking the include guard detection really sucks. I'll make sure that gets fixed.</div><div><br></div><div>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?</div></div></div></div></blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>As an alternative, how about this:</div><div><br></div><div>Provide a dummy __config_site in the svn include directory, and make __config unconditionally #include __config_site. On install, configure the __<a href="http://config_site.in" target="_blank">config_site.in</a> file to produce an installed __config_site rather than copying the dummy one.</div><div><br></div><div>Would that address your needs?</div><div><div class="m_840859260227922915h5"><div> <br></div></div></div></div></div></div></blockquote><div><br></div></span><div>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?</div><div><br></div><div>@Marshall does this sound good to you?</div><div><br></div><div>This solution would be much simpler and cleaner in the end.</div><div><div class="h5"><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="m_840859260227922915h5"><div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+    # Add a target that executes the generation commands.<br>
+    add_custom_target(generate_con<wbr>fig_header ALL<br>
+      DEPENDS ${LIBCXX_BINARY_DIR}/__generat<wbr>ed_config)<br>
+    # Install the generated header as __config.<br>
+    install(FILES ${LIBCXX_BINARY_DIR}/__generat<wbr>ed_config<br>
+      DESTINATION include/c++/v1<br>
+      PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ<br>
+      RENAME __config<br>
+      COMPONENT libcxx)<br>
+  endif()<br>
+<br>
 endif()<br>
<br>
Added: libcxx/trunk/include/__<a href="http://config_site.in" rel="noreferrer" target="_blank">config_<wbr>site.in</a><br>
URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__config_site.in?rev=250235&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/libcxx/trunk/include/__c<wbr>onfig_site.in?rev=250235&view=<wbr>auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- libcxx/trunk/include/__<a href="http://config_site.in" rel="noreferrer" target="_blank">config_<wbr>site.in</a> (added)<br>
+++ libcxx/trunk/include/__<a href="http://config_site.in" rel="noreferrer" target="_blank">config_<wbr>site.in</a> Tue Oct 13 17:12:02 2015<br>
@@ -0,0 +1,20 @@<br>
+//===------------------------<wbr>------------------------------<wbr>----------------===//<br>
+//<br>
+//                     The LLVM Compiler Infrastructure<br>
+//<br>
+// This file is dual licensed under the MIT and the University of Illinois Open<br>
+// Source Licenses. See LICENSE.TXT for details.<br>
+//<br>
+//===------------------------<wbr>------------------------------<wbr>----------------===//<br>
+<br>
+#ifndef _LIBCPP_CONFIG_SITE<br>
+#define _LIBCPP_CONFIG_SITE<br>
+<br>
+#cmakedefine _LIBCPP_HAS_NO_GLOBAL_FILESYST<wbr>EM_NAMESPACE<br>
+#cmakedefine _LIBCPP_HAS_NO_STDIN<br>
+#cmakedefine _LIBCPP_HAS_NO_STDOUT<br>
+#cmakedefine _LIBCPP_HAS_NO_THREADS<br>
+#cmakedefine _LIBCPP_HAS_NO_MONOTONIC_CLOCK<br>
+#cmakedefine _LIBCPP_HAS_NO_THREAD_UNSAFE_C<wbr>_FUNCTIONS<br>
+<br>
+#endif<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
<br>______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div>