Skip to content

[libcxx] Fix include directory order #65859

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 11, 2023
Merged

[libcxx] Fix include directory order #65859

merged 1 commit into from
Sep 11, 2023

Conversation

abrachet
Copy link
Member

@abrachet abrachet commented Sep 9, 2023

Arch dir needs to go first.

@abrachet abrachet requested a review from a team as a code owner September 9, 2023 20:22
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 9, 2023
abrachet added a commit to abrachet/llvm-project that referenced this pull request Sep 10, 2023
@abrachet
[Fuchsia] Disable libcxx timezone database
b75d844 
tzdb is currently broken when cross compiling from non Linux
to Linux. Lets just disable it totally in our toolchain for now.
We should remove this when llvm#65859 lands.
@abrachet abrachet mentioned this pull request Sep 10, 2023
Merged
abrachet added a commit that referenced this pull request Sep 10, 2023
@abrachet
[Fuchsia] Disable libcxx timezone database (#65870)
89471d5 
tzdb is currently broken when cross compiling from non Linux to Linux.
Lets just disable it totally in our toolchain for now. We should remove
this when #65859 lands.
mordante
mordante requested changes Sep 10, 2023
View reviewed changes
Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abrachet Can you write a more descriptive commit message. Please explain why the current order is wrong.

Requesting changes since I want to understand the patch before it's merged. When it's important it should be commented in the code too.

@abrachet abrachet force-pushed the fix-libcxx-include-order branch from 55f2ae4 to 5c19cb8 Compare September 10, 2023 17:49
@abrachet
Copy link
Member Author

@abrachet Can you write a more descriptive commit message. Please explain why the current order is wrong.

Updated the commit message

@mordante
Copy link
Member

Can you add this part
"It's important that the arch directory be included first so that
its header files which interpose on the default include dir
be included instead of the default ones."

as comment in the code too?

@abrachet
[libcxx] Fix include directory order
e88e2a8 
It's important that the arch directory be included first so that
its header files which interpose on the default include dir
be included instead of the default ones. The clang driver [1] does
this when not building with -nostdinc, the libcxx build should
do the same.

We found this after https://reviews.llvm.org/D154282 when cross
compiling from non Linux to Linux. If the host machine was not
Linux, _LIBCPP_HAS_NO_TIME_ZONE_DATABASE would be defined in
the default include dir __config_site, while it was undefined
in the arch specific one causing build failures.
@abrachet abrachet force-pushed the fix-libcxx-include-order branch from 5c19cb8 to e88e2a8 Compare September 11, 2023 12:52
@abrachet
Copy link
Member Author

Can you add this part "It's important that the arch directory be included first so that its header files which interpose on the default include dir be included instead of the default ones."

as comment in the code too?

Done

mordante
mordante approved these changes Sep 11, 2023
View reviewed changes
Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please make sure that when landing the PR message is updated to the message of the commit. (I'm not sure whether GitHub does that automatically.)

@abrachet abrachet merged commit 62bec3d into llvm:main Sep 11, 2023
@abrachet abrachet deleted the fix-libcxx-include-order branch September 11, 2023 16:36
@EricWF
Copy link
Member

EricWF commented Sep 12, 2023

It was my understanding that libc++ didn't have any duplicate headers in the include/ and target include directory.
For example, the __config_site should only ever be in one.

Are you guys doing something weird?

@abrachet
Copy link
Member Author

When APPLE __config_site gets generated in the default include dir [1], while other platforms will generate it in the arch dir [2]. We were cross compiling Mac -> Linux

[1] https://github.com/llvm/llvm-project/blob/main/libcxx/CMakeLists.txt#L438
[2] https://github.com/llvm/llvm-project/blob/main/libcxx/CMakeLists.txt#L422

@petrhosek
Copy link
Member

This is only an issue on Darwin; on other platforms we install the target specific files such as __config_site into the per-target directory, i.e. include/<target>/c++/v1/__config_site, but on Darwin there is no per-target directory because Darwin supports multiple targets with a single set of files so __config_site is installed in include/c++/v1/__config_site.

@michaelrj-google michaelrj-google mentioned this pull request Sep 12, 2023
Merged
@vzakhari vzakhari mentioned this pull request Sep 12, 2023
Closed
ZijunZhaoCCK pushed a commit to ZijunZhaoCCK/llvm-project that referenced this pull request Sep 19, 2023
@abrachet @ZijunZhaoCCK
[Fuchsia] Disable libcxx timezone database (llvm#65870)
7c3465d 
tzdb is currently broken when cross compiling from non Linux to Linux.
Lets just disable it totally in our toolchain for now. We should remove
this when llvm#65859 lands.
ZijunZhaoCCK pushed a commit to ZijunZhaoCCK/llvm-project that referenced this pull request Sep 19, 2023
@abrachet @ZijunZhaoCCK
[libcxx] Fix include directory order (llvm#65859)
6b822c2 
It's important that the arch directory be included first so that
its header files which interpose on the default include dir
be included instead of the default ones. The clang driver [1] does
this when not building with -nostdinc, the libcxx build should
do the same.

We found this after https://reviews.llvm.org/D154282 when cross
compiling from non Linux to Linux. If the host machine was not
Linux, _LIBCPP_HAS_NO_TIME_ZONE_DATABASE would be defined in
the default include dir __config_site, while it was undefined
in the arch specific one causing build failures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrhosek petrhosek petrhosek approved these changes

@mordante mordante mordante approved these changes

@ldionne ldionne Awaiting requested review from ldionne

Assignees
No one assigned
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@abrachet @mordante @EricWF @petrhosek @llvmbot