Create a __config_site file to capture configuration decisions.

[EricWF]

Summary

This patch takes @jroelof's idea and runs with it. However this patch doesn't require a config_site header. The patch works modifying build-libcxx/include/c++/v1/config by prepending the contents of __config_site. If extra configuration options are needed the test suite uses build-libcxx/include/c++/v1. Otherwise the test suite still uses libcxx/include.

Note that if no extra configuration options are needed then build-libcxx/include/c++/v1/__config is not modified.

[jroelofs]

fix typo

[danalbert]

CMakeLists.txt

304

Did you forget to upload this?

[jroelofs]

@danalbert: sigh, yes.

[mclow.lists]

include/__config

19 ↗

(On Diff #31885)

I'm reluctant to do this; because every include file slows down compilation - for every program that we compile.

However, this may be the right thing to do.

test/std/atomics/libcpp-has-no-threads.pass.cpp

11 ↗

(On Diff #31885)

This is not the right include file.

How about "#include <ciso646>", which defined in the standard (to do nothing), but includes "<__config>"

Really, this test doesn't belong in test/std/, but in test/libcxx, but this change didn't put it there.

[espositofulvio]

include/__config

19 ↗

(On Diff #31885)

I'm with Jonathan here, having config params dealt with this way it's easier and make things more manageable while the price of a slowdown, I think, shouldn't be substantial.

[jroelofs]

include/__config

19 ↗

(On Diff #31885)

Another option would be to rename __config to __config.in, and put the #cmakedefine lines in here. Then the include tree isn't changed.

test/std/atomics/libcpp-has-no-threads.pass.cpp

11 ↗

(On Diff #31885)

I think it'd be better for me to just move the test. What is <ciso646> supposed to be for?

[mclow.lists]

include/__config

19 ↗

(On Diff #31885)

I just realized that this will complicate life for libc++ developers. Today, I can make a change in the checked-out directory, and test it by using clang -I <path to libcxx>. Now, I'll have to actually build and install the headers some where to test. [ Especially when testing against an installed compiler whose libc++ does not have a __config_site file ]

test/std/atomics/libcpp-has-no-threads.pass.cpp

11 ↗

(On Diff #31885)

The standard says that including it does nothing. :-)

It's a great file to include to get all the config information.

[jroelofs]

include/__config

19 ↗

(On Diff #31885)

Now, I'll have to actually build and install the headers some where to test.

Just building is sufficient. The headers get copied to the build directory, along with this new file.

[ Especially when testing against an installed compiler whose libc++ does not have a __config_site file ]

Why does your workflow rely on testing libcxx binaries against installed headers from a _different_ build of the library? That seems very dicey.

[EricWF]

@jroelofs Thanks for this patch. I've needed this for a while.

include/__config

19 ↗

(On Diff #31885)

Just building is sufficient. The headers get copied to the build directory, along with this new file.

Except in "exceptional" configurations the __config_site file should essentially remain empty. We could put an empty __config_site file in the source directory and telling CMake to ignore it. This would prevent the need to rebuild libc++ as frequently for the majority of developers.

[jroelofs]

Add license text to the new file, and move the two has-no-threads tests to test/libcxx.

[EricWF]

I think we can avoid requiring the need to rebuild every time the headers change but it's not the cleanest. We could

1. Have an empty __config_site file in libcxx/include. This __config_site file does not get copied into the build directory.
2. Generate __config_site.in into build-libcxx/include/c++/v1.
3. Point LIT to libcxx/include (not build-libcxx/include/c++/v1) and add the flag -include build-libcxx/include/c++/v1/__config_site to pick up the generated __config_site file.

This way only changes that modify __config_site require rebuilding libc++ AND libcxx/include can be used as the libc++ include root for most configurations.

Another option would be to have LIT itself re-run the build and copy the headers but I don't think this is as good of an option.

Hopefully this all made sense.

@jroelofs What do you think of an approach like this?

[EricWF]

I also just realized that this change will currently likely play havoc with how libc++ and libc++abi build together. In order to build libc++ and libc++abi together we would need to

1. Configure libc++ pointing to the libc++abi headers in order to generate the __config_site file.
2. Configure libc++abi pointing it to the libc++ build directory for the headers.
3. build libc++abi
4. build libc++

I'm not quite sure how this would work for an in-tree build.

However if we do things as I suggested above we can keep the current two step build process.

[jroelofs]

In D11963#227444, @EricWF wrote:

I also just realized that this change will currently likely play havoc with how libc++ and libc++abi build together. In order to build libc++ and libc++abi together we would need to

1. Configure libc++ pointing to the libc++abi headers in order to generate the __config_site file.
2. Configure libc++abi pointing it to the libc++ build directory for the headers.
3. build libc++abi
4. build libc++

I'm not quite sure how this would work for an in-tree build.

This patch, combined with D11964, works the way you describe.

However if we do things as I suggested above we can keep the current two step build process.

[jroelofs]

In D11963#227441, @EricWF wrote:

@jroelofs What do you think of an approach like this?

Having two copies of the __config_site file makes me uncomfortable, but I could put up with that given that they're effectively the same for 99% of people who will want to build this library.

That being said, @mclow.lists raised a few concerns with the overall strategy... I don't want to keep pushing on this patch if his plan is to pocket-veto it.

[EricWF]

In D11963#227951, @jroelofs wrote:

In D11963#227441, @EricWF wrote:

@jroelofs What do you think of an approach like this?

Having two copies of the __config_site file makes me uncomfortable, but I could put up with that given that they're effectively the same for 99% of people who will want to build this library.

That being said, @mclow.lists raised a few concerns with the overall strategy... I don't want to keep pushing on this patch if his plan is to pocket-veto it.

My hackey suggestion makes me uncomfortable too. However, as unfortunate as it is, we need a patch like this (Despite how bad I don't want it). One reason is that libc++ *claims* to support using libsupc++ to provide typeinfo definition. The problem is that libc++ declares typeinfo with a different vtable layout. In order to match libsupc++ we need something like a __config_site header. I think we either need to nuke libsupc++ support or adopt a patch like this.

[EricWF]

In D11963#227933, @jroelofs wrote:

In D11963#227444, @EricWF wrote:

I also just realized that this change will currently likely play havoc with how libc++ and libc++abi build together. In order to build libc++ and libc++abi together we would need to

1. Configure libc++ pointing to the libc++abi headers in order to generate the __config_site file.
2. Configure libc++abi pointing it to the libc++ build directory for the headers.
3. build libc++abi
4. build libc++

I'm not quite sure how this would work for an in-tree build.

This patch, combined with D11964, works the way you describe.

Wouldn't it still be a mess if you build both libc++ and libc++abi out-of-tree? The user would have to be aware of these weird requirements.

[jroelofs]

In D11963#228024, @EricWF wrote:

Wouldn't it still be a mess if you build both libc++ and libc++abi out-of-tree? The user would have to be aware of these weird requirements.

Oh, yeah, true. Do we have any out-of-tree builders to make sure those configurations are supported?

[EricWF]

Stealing this revision. I have a different plan.

[EricWF]

This patch takes @jroelof's idea and runs with it. However this patch doesn't require a __config_site header. The patch works modifying build-libcxx/include/c++/v1/__config by prepending the contents of __config_site. If extra configuration options are needed the test suite uses build-libcxx/include/c++/v1. Otherwise the test suite still uses libcxx/include.

Note that if no extra configuration options are needed then build-libcxx/include/c++/v1/__config is not modified.

[EricWF]

Update diff to remove misc cleanup stuff.

[EricWF]

Copy the headers to the build directory during every build instead of just during CMake configuration.

[jroelofs]

In D11963#234789, @EricWF wrote:

Copy the headers to the build directory during every build instead of just during CMake configuration.

Sounds like this would cause every build to effectively be a full build. I don't think we want that...

If that's the case, then I think we should use the same trick that TableGen uses: write the new file to a temporary, and overwrite it iff they differ.

[EricWF]

In D11963#235148, @jroelofs wrote:

In D11963#234789, @EricWF wrote:

Copy the headers to the build directory during every build instead of just during CMake configuration.

Sounds like this would cause every build to effectively be a full build. I don't think we want that...

If that's the case, then I think we should use the same trick that TableGen uses: write the new file to a temporary, and overwrite it iff they differ.

So that's not what happens but that's because libc++ builds using the headers in the source directory. During the build all of the macros in the __config_site file are manually defined on the command line so that we don't need to use the generated headers. NOTE: The tests still use the generated headers though.

[jroelofs]

In D11963#236045, @EricWF wrote:

So that's not what happens but that's because libc++ builds using the headers in the source directory. During the build all of the macros in the __config_site file are manually defined on the command line so that we don't need to use the generated headers. NOTE: The tests still use the generated headers though.

Oh, I didn't catch that you were setting this up to build from the headers in the source directory.

[EricWF]

In D11963#236376, @jroelofs wrote:

In D11963#236045, @EricWF wrote:

So that's not what happens but that's because libc++ builds using the headers in the source directory. During the build all of the macros in the __config_site file are manually defined on the command line so that we don't need to use the generated headers. NOTE: The tests still use the generated headers though.

Oh, I didn't catch that you were setting this up to build from the headers in the source directory.

Yeah, I worry this patch is very unclear. To sum the steps CMake takes:

Configuration time:

1. Configure CMake and detect if the configuration requires any "__config_site" defines.
2. Add each "__config_site" definition to the command line manually. These are used during the build.
3. Generate a build/__config_site file containing each required definition.
4. If any "__config_site" definitions are required setup the tests to use build/include/c++/v1. Otherwise the tests will use src/include as they currently do.

Build time:

1. Copy src/include to build/include/c++/v1.
2. If any "__config_site" defines are needed prepend build/__config_site to build/include/c++/v1/__config.
3. Build libc++ using the headers in src/include and the "__config_site" definitions added in Configuration step #2. Note that the actual build ignores the generated headers.

Install Time:

1. Do build steps including regenerating build headers.
2. Install headers in build/include/c++/v1.

[jroelofs]

Without the context of this review, this strategy probably won't make any sense. So it's probably worth capturing these notes in a comment that describes why we're doing it that way.

[eugenis]

This would greatly simplify the abi versioning change in D11740. Thanks for working on this!

include/CMakeLists.txt

24

This is creative :)
I don't know the usual cmake conventions, but CopyLibcxxHeaders is very different from other files in cmake/Modules in that it is a standalone script and not an included file. Should it be moved to smth like cmake/Scripts, or named differently?

[eugenis]

Hi Eric, have you had a chance to take another look at this? It is blocking D11740.

[EricWF]

Abandoning in favor of D13407.

CMakeLists.txt

 set(LIBCXX_COMPILER    ${CMAKE_CXX_COMPILER})
 set(LIBCXX_SOURCE_DIR  ${CMAKE_CURRENT_SOURCE_DIR})
 set(LIBCXX_BINARY_DIR  ${CMAKE_CURRENT_BINARY_DIR})
+set(LIBCXX_HEADER_DIR   ${CMAKE_BINARY_DIR}/include/c++/v1/)
 set(LIBCXX_LIBRARY_DIR ${CMAKE_BINARY_DIR}/lib${LIBCXX_LIBDIR_SUFFIX})
 
 set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${LIBCXX_LIBRARY_DIR})

 set(LIBCXX_COMPILE_FLAGS "")
 set(LIBCXX_LINK_FLAGS "")
 set(LIBCXX_LIBRARIES "")
+set(LIBCXX_NEEDS_SITE_CONFIG OFF)
 
 # Configure compiler.
 include(config-ix)

 
 # 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 =============================================================
 
+# Sanitizer flags ==============================================================
 # Configure for sanitizers. If LIBCXX_BUILT_STANDALONE then we have to do
 # the flag translation ourselves. Othewise LLVM's CMakeList.txt will handle it.
 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)
+
+configure_file(
+  include/__config_site.in
+  ${LIBCXX_BINARY_DIR}/__config_site
+  @ONLY)
+
 #===============================================================================
 # Setup Source Code And Tests
 #===============================================================================

cmake/Modules/CopyLibcxxHeaders.cmake

+set(LIBCXX_HEADER_PATTERN
+  PATTERN "*"
+  PATTERN "CMakeLists.txt" EXCLUDE
+  PATTERN ".svn" EXCLUDE
+  PATTERN "__config_site.in" EXCLUDE
+  ${LIBCXX_SUPPORT_HEADER_PATTERN}
+  )
+
+file(COPY .
+  DESTINATION "${LIBCXX_HEADER_DIR}"
+  FILES_MATCHING
+  ${LIBCXX_HEADER_PATTERN}
+  )
+
+if (LIBCXX_NEEDS_SITE_CONFIG)
+  file(COPY   ${LIBCXX_BINARY_DIR}/__config_site DESTINATION ${LIBCXX_HEADER_DIR})
+  file(RENAME ${LIBCXX_HEADER_DIR}/__config_site ${LIBCXX_HEADER_DIR}/__config)
+  file(READ   ${LIBCXX_SOURCE_DIR}/include/__config CONFIG_CONTENTS)
+  file(APPEND ${LIBCXX_HEADER_DIR}/__config "${CONFIG_CONTENTS}")
+endif()

cmake/Modules/HandleLibcxxFlags.cmake

   endif()
 endmacro()
 
+macro(config_define_if condition def)
+  if (${condition})
+    add_definitions(-D${def})
+    set(${def} ON)
+    set(LIBCXX_NEEDS_SITE_CONFIG ON)
+  endif()
+endmacro()
+
+macro(config_define_if_not condition def)
+  if (NOT ${condition})
+    add_definitions(-D${def})
+    set(${def} ON)
+    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)

include/CMakeLists.txt

 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}
   )
 
-file(COPY .
-  DESTINATION "${CMAKE_BINARY_DIR}/include/c++/v1"
-  FILES_MATCHING
-  ${LIBCXX_HEADER_PATTERN}
-  )
+# The headers should be copied to the build directory every time libc++ is built.
+# Because the CMake file(...) commands only run at configuration time we use
+# a cmake script that we can run during every build.
+add_custom_target(libcxx-copy-headers ALL
+  COMMAND
+  ${CMAKE_COMMAND}
+  -DLIBCXX_HEADER_DIR=${LIBCXX_HEADER_DIR}
+  -DLIBCXX_BINARY_DIR=${LIBCXX_BINARY_DIR}
+  -DLIBCXX_SOURCE_DIR=${LIBCXX_SOURCE_DIR}
+  -DLIBCXX_NEEDS_SITE_CONFIG=${LIBCXX_NEEDS_SITE_CONFIG}
+  -P ${LIBCXX_SOURCE_DIR}/cmake/Modules/CopyLibcxxHeaders.cmake
+  WORKING_DIRECTORY ${CMAKE_CURRENT_LIST_DIR})
+
 
 if (LIBCXX_INSTALL_HEADERS)
   install(DIRECTORY .

include/__config_site.in

+//===----------------------------------------------------------------------===//
+//
+//                     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

test/CMakeLists.txt

 pythonize_bool(LIBCXX_GENERATE_COVERAGE)
 pythonize_bool(LIBCXXABI_USE_LLVM_UNWINDER)
 
+if (LIBCXX_NEEDS_SITE_CONFIG)
+  set(LIBCXX_TEST_HEADERS ${LIBCXX_BINARY_DIR}/include/c++/v1)
+endif()
+
 # The tests shouldn't link to any ABI library when it has been linked into
 # libc++ statically.
 if (LIBCXX_ENABLE_STATIC_ABI_LIBRARY)

test/libcxx/test/config.py

         support_path = os.path.join(self.libcxx_src_root, 'test/support')
         self.cxx.compile_flags += ['-I' + support_path]
         self.cxx.compile_flags += ['-include', os.path.join(support_path, 'nasty_macros.hpp')]
-        libcxx_headers = self.get_lit_conf(
-            'libcxx_headers', os.path.join(self.libcxx_src_root, 'include'))
+
+        libcxx_headers = self.get_lit_conf('libcxx_headers')
+        if libcxx_headers:
+            self.lit_config.note('Using libc++ headers in: %s' % libcxx_headers)
+        else:
+            libcxx_headers = os.path.join(self.libcxx_src_root, 'include')
+
         if not os.path.isdir(libcxx_headers):
             self.lit_config.fatal("libcxx_headers='%s' is not a directory."
                                   % libcxx_headers)

         if not enable_global_filesystem_namespace:
             self.config.available_features.add(
                 'libcpp-has-no-global-filesystem-namespace')
-            self.cxx.compile_flags += [
-                '-D_LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE']
 
     def configure_compile_flags_no_stdin(self):
         enable_stdin = self.get_lit_bool('enable_stdin', True)
         if not enable_stdin:
             self.config.available_features.add('libcpp-has-no-stdin')
-            self.cxx.compile_flags += ['-D_LIBCPP_HAS_NO_STDIN']
 
     def configure_compile_flags_no_stdout(self):
         enable_stdout = self.get_lit_bool('enable_stdout', True)
         if not enable_stdout:
             self.config.available_features.add('libcpp-has-no-stdout')
-            self.cxx.compile_flags += ['-D_LIBCPP_HAS_NO_STDOUT']
 
     def configure_compile_flags_no_threads(self):
-        self.cxx.compile_flags += ['-D_LIBCPP_HAS_NO_THREADS']
         self.config.available_features.add('libcpp-has-no-threads')
 
     def configure_compile_flags_no_thread_unsafe_c_functions(self):
         enable_thread_unsafe_c_functions = self.get_lit_bool(
             'enable_thread_unsafe_c_functions', True)
         if not enable_thread_unsafe_c_functions:
-            self.cxx.compile_flags += [
-                '-D_LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS']
             self.config.available_features.add(
                 'libcpp-has-no-thread-unsafe-c-functions')
 
     def configure_compile_flags_no_monotonic_clock(self):
-        self.cxx.compile_flags += ['-D_LIBCPP_HAS_NO_MONOTONIC_CLOCK']
         self.config.available_features.add('libcpp-has-no-monotonic-clock')
 
     def configure_link_flags(self):

test/lit.site.cfg.in

 config.cxx_under_test           = "@LIBCXX_COMPILER@"
 config.libcxx_src_root          = "@LIBCXX_SOURCE_DIR@"
 config.libcxx_obj_root          = "@LIBCXX_BINARY_DIR@"
+config.libcxx_headers           = "@LIBCXX_TEST_HEADERS@"
 config.cxx_library_root         = "@LIBCXX_LIBRARY_DIR@"
 config.enable_exceptions        = "@LIBCXX_ENABLE_EXCEPTIONS@"
 config.enable_rtti              = "@LIBCXX_ENABLE_RTTI@"