Skip to content
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

[CMake] Correctly handle LLVM_ENABLE_RUNTIMES in targets #69869

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

petrhosek
Copy link
Member

When a target sets LLVM_ENABLE_RUNTIMES, we should only generate proxy targets for those runtimes rather than using the global list which may contain runtimes that are not supported by that particular target.

@petrhosek petrhosek force-pushed the runtimes-enable-runtimes-passthrough branch from 431934b to 244b1ab Compare October 23, 2023 02:22
Copy link
Collaborator

@smeenai smeenai left a comment

Choose a reason for hiding this comment

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

LGTM

When a target sets LLVM_ENABLE_RUNTIMES, we should only generate proxy
targets for those runtimes rather than using the global list which may
contain runtimes that are not supported by that particular target.
@petrhosek petrhosek force-pushed the runtimes-enable-runtimes-passthrough branch from 244b1ab to 87b62dd Compare October 24, 2023 05:59
@petrhosek petrhosek merged commit 0fc8f0b into llvm:main Oct 25, 2023
3 checks passed
@JDevlieghere
Copy link
Member

Looks like this broke the cxx target: https://green.lab.llvm.org/green/view/LLDB/job/as-lldb-cmake/8459/

$ cmake /Users/jonas/llvm/llvm-project/llvm \
                                       -G Ninja \
                                       -DCMAKE_BUILD_TYPE='RelWithDebInfo' \
                                       -DLLVM_ENABLE_ASSERTIONS:BOOL=ON \
                                       -DLLVM_ENABLE_PROJECTS='llvm;clang' \
                                       -DLLVM_ENABLE_RUNTIMES='libcxx;libcxxabi'
...
$ ninja cxx
ninja: error: unknown target 'cxx', did you mean 'lli'?

@ldionne
Copy link
Member

ldionne commented Oct 25, 2023

@petrhosek I kind of missed that change, but can you give an example of what this change is doing? What is an example of a "proxy" target?

@JDevlieghere
Copy link
Member

@ldionne pointed me at the runtimes target (which seems like the right thing to do regardless of this change) so I've changed the lldb build to depend on that (dfa3570).

@petrhosek
Copy link
Member Author

@ldionne I ran into this issue when trying to build LLVM libc in the bootstrapping build. I need to include libc in LLVM_ENABLE_RUNTIMES to ensure that the host tools used by libc—that is libc-hdrgen—are built, but I only want build libc for theriscv32-unknown-elf target so I exclude it from all RUNTIMES_<target>_LLVM_ENABLE_RUNTIMES (with the exception of RUNTIMES_riscv32-unknown-elf_LLVM_ENABLE_RUNTIMES).

The issue is that the logic in runtimes prior to this patch would always iterate over the global LLVM_ENABLE_RUNTIMES and set up the proxy targets for the child builds. Concretely, in my example libc would depend on libc-x86_64-unknown-linux-gnu which is a proxy for the libc target in the runtimes-x86_64-unknown-linux-gnu-bins build, except that there's no libc target in that build because RUNTIMES_x86_64-unknown-linux-gnu_LLVM_ENABLE_RUNTIMES doesn't include libc and that leads to a build failure.

When we first implemented the bootstrapping (aka runtimes build), we only supported the global LLVM_ENABLE_RUNTIMES as the assumption was that every target would build the same set of runtimes, which is not the case in practice so we introduced support for RUNTIMES_<target>_LLVM_ENABLE_RUNTIMES but we never really considered interactions between the two which is what this issue is about.

@JDevlieghere sorry about the breakage, that was unintentional. I'll try to reproduce your failure locally and see if it's a problem with this change.

function(_get_runtime_name name out_var)
string(FIND ${name} "lib" idx)
if(idx EQUAL 0 AND NOT ${name} STREQUAL "libc")
string(SUBSTRING ${name} 3 -1 entry)
Copy link
Contributor

Choose a reason for hiding this comment

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

In response to the issue seen by @JDevlieghere where the cxx and cxxabi targets are not present:

I believe entry needs changed to name for the SUBSTRING on line 177.

When using -DLLVM_ENABLE_RUNTIMES='libcxx;libcxxabi', ${name} is being returned from the function instead of the SUBSTRING ${entry}.

With your original patch the targets remain libcxx and libcxxabi.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching that, I fixed it in fb619b3.

zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Oct 26, 2023
When a target sets LLVM_ENABLE_RUNTIMES, we should only generate proxy
targets for those runtimes rather than using the global list which may
contain runtimes that are not supported by that particular target.
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Oct 26, 2023
Fix and reland

When a target sets LLVM_ENABLE_RUNTIMES, we should only generate proxy
targets for those runtimes rather than using the global list which may
contain runtimes that are not supported by that particular target.

Change-Id: I90971ced1b6c0188bc9652d960e48390124a32a7
petrhosek added a commit that referenced this pull request Oct 27, 2023
While extracting the existing functionality into a function, one of the
variable usages wasn't correctly updated.
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.

5 participants