Skip to content

[runtimes] Pass-through CLANG_RESOURCE_DIR to cmake invocations #73185

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.

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

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

tstellar
Copy link
Collaborator

compiler-rt and libomp need access to this variable in order to install their libraries and headers to the path that clang expects.

compiler-rt and libomp need access to this variable in order to install
their libraries and headers to the path that clang expects.
Copy link
Contributor

@kwk kwk left a comment

Choose a reason for hiding this comment

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

This looks good to me and for the day that I'm testing it brings down the number of failing tests from 53 to 23.

@petrhosek
Copy link
Member

What's issue you're seeing without this change? Projects like compiler-rt and libomp use https://github.com/llvm/llvm-project/blob/a8837b49c1c1650357d7d8f2957c903b3a5ab039/cmake/Modules/GetClangResourceDir.cmake to retrieve the Clang resource directory which will construct it manually when CLANG_RESOURCE_DIR isn't set and I haven't seen any issue being reported.

@tstellar
Copy link
Collaborator Author

@petrhosek This issue we've seen is some test case failures caused by clang and compiler-rt disagreeing on where the clang resource directory is.

If you do something like cmake -DCLANG_RESOURCE_DIR=../lib/clang/18, clang will use this path, but when compiler-rt calls get_clang_resource_dir, CLANG_RESOURCE_DIR is not set (because it's a different CMake invocation), so it tries to construct it's own path and ends up with something different.

Copy link
Member

@petrhosek petrhosek left a comment

Choose a reason for hiding this comment

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

That makes sense. I'd prefer having a list of passthrough variables and then include CLANG_RESOURCE_DIR in that list as a more generic mechanism rather than using COMMON_CMAKE_ARGS but I'm also fine landing this change as is and refactoring it later.

@tstellar tstellar merged commit 2e838c8 into llvm:main Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants