This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Re-add transitive includes that had been removed since LLVM 14
ClosedPublic

Authored by ldionne on Jun 27 2022, 9:10 AM.

Details

Summary

This commit re-adds transitive includes that had been removed by
4cd04d1687f1, c36870c8e79c, a83f4b9cda57, 1458458b558d, 2e2f3158c604,
and 489637e66dd3. This should cover all the includes that had been
removed since LLVM 14 and that would contribute to breaking user code
when releasing LLVM 15.

It is possible to disable the inclusion of these headers by defining
_LIBCPP_REMOVE_TRANSITIVE_INCLUDES. The intent is that vendors will
enable that macro and start fixing downstream issues immediately. We
can then remove the macro (and the transitive includes) by default in
a future release. That way, we will break users only once by removing
transitive includes in bulk instead of doing it bit by bit a every
release, which is more disruptive for users.

Note 1: The set of headers to re-add was found by re-generating the

transitive include test on a checkout of release/14.x, which
provided the list of all transitive includes we used to provide.

Note 2: Several includes of <vector>, <optional>, <array> and <unordered_map>

have been added in this commit. These transitive inclusions were
added when we implemented boyer_moore_searcher in <functional>.

Note 3: This is a best effort patch to try and resolve downstream breakage

caused since branching LLVM 14. I wasn't able to perfectly mirror
transitive includes in LLVM 14 for a few headers like <unordered_map>
and <optional> due to circular dependencies.

Diff Detail

Event Timeline

ldionne created this revision.Jun 27 2022, 9:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 9:10 AM
ldionne requested review of this revision.Jun 27 2022, 9:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2022, 9:10 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.Jun 27 2022, 9:12 AM
libcxx/include/random
1728

Note that I have a follow-up patch ready to go that consolidates all those into the _LIBCPP_REMOVE_TRANSITIVE_INCLUDES block (in all headers).

A said before I'm not completely happy we have to take this approach, but I understand why it's important for some users of libc++. So I don't have objections against this approach. But I really would like the CI to test the new macro.

libcxx/docs/ReleaseNotes.rst
103

Can we use _LIBCPP_REMOVE_TRANSITIVE_INCLUDES in the CI, that way we know that removing the includes guarded by this macro doesn't bit rot.

ldionne updated this revision to Diff 440313.Jun 27 2022, 10:53 AM
ldionne marked an inline comment as done.

Add CI job.

philnik added inline comments.
libcxx/cmake/caches/Generic-no-transitive-includes.cmake
1

I think the right way would be to remove the transitive includes by default in the CI and disable them in one runner.

libcxx/include/algorithm
1263–1268

I would put the includes below the standard-mandated ones, but I guess it doesn't make a big difference, since we plan to remove them anyways.

ldionne updated this revision to Diff 440375.Jun 27 2022, 1:50 PM

Fix circular dependency issues, reduce the difference with LLVM 14.

libcxx/cmake/caches/Generic-no-transitive-includes.cmake
1

IMO the CI should mirror the state of the library as we're going to release/ship it. It makes it more likely that we'll catch errors that we would otherwise ship. Since we want to ship with the transitive includes enabled by default, all CI jobs should have them by default.

The added CI job only exists to ensure that the code doesn't bit rot and that we can remove the transitive includes in the future without additional work.

libcxx/include/algorithm
1263–1268

I'll keep as-is, because changing this would be a sizeable amount of work for little benefit, especially since I have other follow-up patches locally that would need a ton of rebasing. I'll be happy to do it if there's a technical reason to, but I can't think of one right now. LMK if you do.

philnik added inline comments.Jun 27 2022, 2:33 PM
libcxx/cmake/caches/Generic-no-transitive-includes.cmake
1

I would normally agree, but in this case I don't see how it could break anything. We have tests to ensure that we can include everything in a single TU, a Modules build and more to ensure all the includes works together properly. I would see it the way same as _LIBCPP_HAS_NO_INCOMPLETE_RANGES - we work on it and check it in the CI, but we don't ship it yet (plus we have a CI job for the "disabled" configuration).

libcxx/include/algorithm
1263–1268

It's just preference - the stuff we don't want to include shouldn't provide dependencies for other headers. This is of course a moot point with a modules build. So yeah, I don't care that much (especially if it would cause a lot of work for you).

ldionne accepted this revision.Jun 27 2022, 7:18 PM
ldionne added inline comments.
libcxx/cmake/caches/Generic-no-transitive-includes.cmake
1

Yeah, I can see the parallel with _LIBCPP_HAS_NO_INCOMPLETE_RANGES, however I do think that this option leads to complexity and brittleness (for example we have to turn it ON on our release branches while it's OFF on main). Concretely, I think having a CMake cache for LLVM releases would solve most of my concerns about this, except for the need to have a CMake-level configuration option for this knob: If we change it to _LIBCPP_ENABLE_BACKWARDS_COMPAT_TRANSITIVE_INCLUDES and make it undefined by default, we'll need a setting in __config_site to define it by default for LLVM releases and for most vendors. I'd like to avoid this additional complexity.

Also, to further make a distinction with _LIBCPP_HAS_NO_INCOMPLETE_RANGES, once we move to -funstable, _LIBCPP_HAS_NO_INCOMPLETE_RANGES will be disabled by default (unlike right now), so it will be consistent with the approach proposed here. I'll land this patch now because I want to see how it affects downstreams immediately, but I don't consider this discussion to be closed -- I'm open to discussion on the default ON/OFF.

This revision is now accepted and ready to land.Jun 27 2022, 7:18 PM
sberg added a subscriber: sberg.Jun 27 2022, 11:43 PM
This comment was removed by sberg.

[sorry for the noise; my previous, removed comment was due to some local modifications on my end]