This is an archive of the discontinued LLVM Phabricator instance.

[llvm-driver] Generate symlinks instead of executables for tools
ClosedPublic

Authored by abrachet on Jun 14 2022, 2:39 PM.

Details

Summary

When LLVM_TOOL_LLVM_DRIVER_BUILD is On, create symlinks to llvm instead of creating the executables. Currently this only works for install and not install-distribution, the work for the later will be split up into a second patch.

Diff Detail

Event Timeline

abrachet created this revision.Jun 14 2022, 2:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 2:39 PM
Herald added a subscriber: mgorny. · View Herald Transcript
abrachet requested review of this revision.Jun 14 2022, 2:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 2:39 PM
phosek added inline comments.Jul 11 2022, 4:10 PM
clang/cmake/modules/AddClang.cmake
182

Should we check LLVM_TOOL_LLVM_DRIVER_BUILD here as well?

llvm/cmake/modules/AddLLVM.cmake
865–866

This name isn't particularly descriptive, how about setup_llvm_driver?

865–866

Most of these aren't being used in this function, could we prune this list and only keep the ones that are needed here?

914

Conversely, I think the arguments that are only used in this macro should be parsed here with cmake_parse_arguments.

2016

Should we check LLVM_TOOL_LLVM_DRIVER_BUILD here as well?

llvm/tools/CMakeLists.txt
67

Nit: This is superfluous.

llvm/tools/llvm-driver/CMakeLists.txt
34

The issue you described with subdirectory is a known limitation of CMake when using post build commands.

Do you need ALWAYS_GENERATE here? Have you tried it without that option. That would remove the limitation and I think it should be sufficient.

abrachet updated this revision to Diff 444473.Jul 13 2022, 5:44 PM
abrachet marked 6 inline comments as done.

Since you're working on this again, could you please kindly fix the regressions you've introduced?

https://reviews.llvm.org/D109977#3611683

We can't build clang for over a month now.

Looks like I forgot to click send on these comments.

Since you're working on this again, could you please kindly fix the regressions you've introduced?

https://reviews.llvm.org/D109977#3611683

We can't build clang for over a month now.

I've responded there

llvm/cmake/modules/AddLLVM.cmake
865–866

I don't think setup_llvm_driver makes sense because this is used mostly for add_llvm_executable.

865–866

Sure, that's a lot cleaner. The one hiccup I had was when this is called from add_clang_tool, SUPPORT_PLUGINS was getting propagated down, and there's no clean way (that I could think of) to eat that arg and not pass it to setup_llvm_executable. So I just parse it here even though it is unused. The alternative I could think of is to parse it in add_clang_tool.

llvm/tools/llvm-driver/CMakeLists.txt
34

Do you need ALWAYS_GENERATE here? Have you tried it without that option.

It's been a while so I don't totally remember every reason why I landed here. Though I remember this taking most of my time on the patch. I did start without it because the name made me think it likely wasn't what I wanted but for a couple of reasons I ended up using it.

The first big problem is there is both a library and clang symlink named clang-cpp, which without ALWAYS_GENERATE caused cmake to fail. It's not clear why that library is even called libclang-cpp it was changed in D64278 with no context as to why. It seemed they just changed it without too much fuss so likely it isn't a deal breaker if we really wanted to. But certainly annoying.

Also, if you do ninja llvm-cxxfilt it will build llvm but doesn't create the llvm-cxxfilt symlink. With ALWAYS_GENERATE it creates the symlink as I would expect.

FWIW, add_{clang,lld,flang}_symlink also all opt to use ALWAYS_GENERATE, though I suspect the latter two just copied what clang did.

That would remove the limitation and I think it should be sufficient.

I verified that you are right that I can get away without generate_driver_tool_targets in llvm/tools/CMakeLists.txt instead of hear if I omit the ALWAYS_GENERATE

phosek accepted this revision.Jul 19 2022, 1:23 AM

LGTM

llvm/cmake/modules/AddLLVM.cmake
865–866

You could use https://cmake.org/cmake/help/v3.13/command/list.html#filter to filter it out of ${ARGN} in add_clang_tool before passing that to this macro but I'm not sure if that's cleaner.

865–866

The part I don't like is that it's not clear from the name what's the relationship between add_llvm_executable and setup_llvm_executable and when should I call which one (without knowing the details, I'd assume that I have to first add before I can setup). Maybe generate_llvm_objects would be better? This is very minor though.

llvm/tools/llvm-driver/CMakeLists.txt
34

Also, if you do ninja llvm-cxxfilt it will build llvm but doesn't create the llvm-cxxfilt symlink. With ALWAYS_GENERATE it creates the symlink as I would expect.

Thanks for the explanation, I think this alone is a reason to keep ALWAYS_GENERATE here.

This revision is now accepted and ready to land.Jul 19 2022, 1:23 AM
abrachet updated this revision to Diff 446011.Jul 19 2022, 6:41 PM
abrachet marked 4 inline comments as done.
This revision was landed with ongoing or failed builds.Jul 19 2022, 6:43 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2022, 6:44 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Amir added a subscriber: Amir.Sep 13 2022, 7:05 PM
Amir added inline comments.
llvm/cmake/modules/AddLLVM.cmake
1285

Sorry for a late question but I don't see any use of ARG_DEPENDS - is it intentional or there's an omission somewhere?

abrachet marked an inline comment as done.Sep 13 2022, 7:54 PM
abrachet added inline comments.
llvm/cmake/modules/AddLLVM.cmake
1285

It looks like this was superfluous and likely copied from a place where DEPENDS is used. It's not necessary for depends to be used here, as it wasn't being used before for add_llvm_tool. I'll remove this