Page MenuHomePhabricator

Create a __config_site file to capture configuration decisions.
AbandonedPublic

Authored by EricWF on Aug 11 2015, 4:19 PM.

Details

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.

Diff Detail

Event Timeline

jroelofs updated this revision to Diff 31877.Aug 11 2015, 4:19 PM
jroelofs updated this revision to Diff 31878.
jroelofs retitled this revision from to Create a __config_site file to capture configuration decisions..
jroelofs updated this object.
jroelofs added reviewers: mclow.lists, danalbert.
jroelofs added subscribers: EricWF, ed, espositofulvio and 2 others.

fix typo

danalbert added inline comments.Aug 11 2015, 4:34 PM
CMakeLists.txt
304

Did you forget to upload this?

mclow.lists added inline comments.Aug 11 2015, 9:28 PM
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.

asl added a subscriber: asl.Aug 11 2015, 10:51 PM
espositofulvio added inline comments.Aug 12 2015, 1:14 AM
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 added inline comments.Aug 12 2015, 7:19 AM
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 added inline comments.Aug 12 2015, 7:38 AM
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 added inline comments.Aug 12 2015, 8:14 AM
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.

@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 updated this revision to Diff 31949.Aug 12 2015, 9:33 AM

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

EricWF edited edge metadata.Aug 18 2015, 10:56 PM

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?

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.

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 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.

@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.

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.

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 commandeered this revision.Aug 24 2015, 1:36 PM
EricWF edited reviewers, added: jroelofs; removed: EricWF.

Stealing this revision. I have a different plan.

EricWF updated this revision to Diff 32992.Aug 24 2015, 1:47 PM

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 updated this revision to Diff 32997.Aug 24 2015, 2:34 PM
EricWF updated this object.

Update diff to remove misc cleanup stuff.

EricWF updated this revision to Diff 33384.Aug 27 2015, 5:58 PM

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

jroelofs edited edge metadata.Aug 28 2015, 7:37 AM

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.

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.

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.

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.

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 added a subscriber: eugenis.Sep 1 2015, 4:01 PM

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?

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

EricWF abandoned this revision.Oct 2 2015, 7:36 PM

Abandoning in favor of D13407.