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

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

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

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

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

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

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

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

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

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

 option(LIBCXX_ENABLE_THREAD_UNSAFE_C_FUNCTIONS "Build libc++ with support for thread-unsafe C functions" ON)
 option(LIBCXX_ENABLE_MONOTONIC_CLOCK
   "Build libc++ with support for a monotonic clock.
-   This option may only be used when LIBCXX_ENABLE_THREADS=OFF." ON)
+   This option may only be set to OFF when LIBCXX_ENABLE_THREADS=OFF." ON)
 
 # Misc options ----------------------------------------------------------------
 option(LIBCXX_ENABLE_PEDANTIC "Compile with pedantic enabled." ON)

 
 # 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 =====================================================
+if (NOT LIBCXX_ENABLE_GLOBAL_FILESYSTEM_NAMESPACE)
+  set(_LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE TRUE)
+endif()
+if (NOT LIBCXX_ENABLE_STDIN)
+  set(_LIBCPP_HAS_NO_STDIN TRUE)
+endif()
+if (NOT LIBCXX_ENABLE_STDOUT)
+  set(_LIBCPP_HAS_NO_STDOUT TRUE)
+endif()
+if (NOT LIBCXX_ENABLE_THREADS)
+  set(_LIBCPP_HAS_NO_THREADS TRUE)
+endif()
+if (NOT LIBCXX_ENABLE_MONOTONIC_CLOCK)
+  set(_LIBCPP_HAS_NO_MONOTONIC_CLOCK TRUE)
+endif()
+if (NOT LIBCXX_ENABLE_THREAD_UNSAFE_C_FUNCTIONS)
+  set(_LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS TRUE)
+endif()
+
 #===============================================================================
+# Setup configuration file
+#===============================================================================
+
+configure_file(
+  include/__config_site.in
+  ${CMAKE_BINARY_DIR}/include/c++/v1/__config_site
+  @ONLY)
+
+#===============================================================================
 # Setup Source Code And Tests
 #===============================================================================
-include_directories(include)
+include_directories(include
+                    "${CMAKE_BINARY_DIR}/include/c++/v1")
 add_subdirectory(include)
 add_subdirectory(lib)
 if (LIBCXX_INCLUDE_TESTS)

include/CMakeLists.txt

   PATTERN "*"
   PATTERN "CMakeLists.txt" EXCLUDE
   PATTERN ".svn" EXCLUDE
+  PATTERN "__config_site.in" EXCLUDE
   ${LIBCXX_SUPPORT_HEADER_PATTERN}
   )
 

include/__config

 #pragma GCC system_header
 #endif
 
+#include <__config_site>
+
 #ifdef __GNUC__
 #define _GNUC_VER (__GNUC__ * 100 + __GNUC_MINOR__)
 #else

test/libcxx/test/config.py

         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', os.path.join(self.cxx_library_root, '..', 'include', 'c++', 'v1'))
         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/std/atomics/libcpp-has-no-threads.pass.cpp

 //===----------------------------------------------------------------------===//
 // XFAIL: libcpp-has-no-threads
 
+#include <__config_site>
+
 #ifdef _LIBCPP_HAS_NO_THREADS
 #error This should be XFAIL'd for the purpose of detecting that the LIT feature\
  'libcpp-has-no-threads' is available iff _LIBCPP_HAS_NO_THREADS is defined

test/std/utilities/date.time/tested_elsewhere.pass.cpp

     static_assert((std::is_same<decltype(std::difftime(t,t)), double>::value), "");
     static_assert((std::is_same<decltype(std::mktime(&tm)), std::time_t>::value), "");
     static_assert((std::is_same<decltype(std::time(&t)), std::time_t>::value), "");
+#ifndef _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS$
     static_assert((std::is_same<decltype(std::asctime(&tm)), char*>::value), "");
     static_assert((std::is_same<decltype(std::ctime(&t)), char*>::value), "");
     static_assert((std::is_same<decltype(std::gmtime(&t)), std::tm*>::value), "");
     static_assert((std::is_same<decltype(std::localtime(&t)), std::tm*>::value), "");
+#endif
     static_assert((std::is_same<decltype(std::strftime(str,s,"",&tm)), std::size_t>::value), "");
 }