This is an archive of the discontinued LLVM Phabricator instance.

lld: Let find_package(LLD) work
ClosedPublic

Authored by arsenm on May 4 2020, 7:37 AM.

Details

Summary

Install a cmake config file. Copied exactly from how clang exports.

I'm slightly confused since it looks like the build is manually
generating a very similar *Targets.cmake to what gets auto-generated
by export(). After generating the build, I see:

build/tools/lld/cmake/modules/CMakeFiles/Export/lib/cmake/lld/LLDTargets.cmake
build/lib/cmake/lld/LLDTargets.cmake
build/tools/clang/cmake/modules/CMakeFiles/Export/lib/cmake/clang/ClangTargets.cmake
build/lib/cmake/clang/ClangTargets.cmake

The cmake generated one "wins" and is what I see in the installed
tree. I'm guessing this is somehow for the benefit of old versions
of cmake?

I also wasn't sure whether the canonical capitalization is "lld" or
"LLD". The project() is still calling this lld, but most places seemed
to capitalize it.

Diff Detail

Event Timeline

arsenm created this revision.May 4 2020, 7:37 AM
labath resigned from this revision.May 4 2020, 8:02 AM

I'm afraid I don't know anything about this stuff.

arsenm added a subscriber: lld.May 4 2020, 6:20 PM
davezarzycki resigned from this revision.May 5 2020, 4:31 AM
smeenai accepted this revision.May 13 2020, 2:52 PM
smeenai added a reviewer: phosek.

LGTM, given it's basically just matching what clang already does.

I'm slightly confused since it looks like the build is manually
generating a very similar *Targets.cmake to what gets auto-generated
by export(). After generating the build, I see:

build/tools/lld/cmake/modules/CMakeFiles/Export/lib/cmake/lld/LLDTargets.cmake
build/lib/cmake/lld/LLDTargets.cmake
build/tools/clang/cmake/modules/CMakeFiles/Export/lib/cmake/clang/ClangTargets.cmake
build/lib/cmake/clang/ClangTargets.cmake

The cmake generated one "wins" and is what I see in the installed
tree. I'm guessing this is somehow for the benefit of old versions
of cmake?

I believe both the CMakeFiles version and the lib version are generated by CMake. The first one should just be an internal CMake file.

lld/CMakeLists.txt
224

Looks like you got rid of the EOF newline

This revision is now accepted and ready to land.May 13 2020, 2:52 PM

LGTM, given it's basically just matching what clang already does.

I'm slightly confused since it looks like the build is manually
generating a very similar *Targets.cmake to what gets auto-generated
by export(). After generating the build, I see:

build/tools/lld/cmake/modules/CMakeFiles/Export/lib/cmake/lld/LLDTargets.cmake
build/lib/cmake/lld/LLDTargets.cmake
build/tools/clang/cmake/modules/CMakeFiles/Export/lib/cmake/clang/ClangTargets.cmake
build/lib/cmake/clang/ClangTargets.cmake

The cmake generated one "wins" and is what I see in the installed
tree. I'm guessing this is somehow for the benefit of old versions
of cmake?

I believe both the CMakeFiles version and the lib version are generated by CMake. The first one should just be an internal CMake file.

Any comment on "lld" vs "LLD"? Should the project() name be capitalized?

LGTM, given it's basically just matching what clang already does.

I'm slightly confused since it looks like the build is manually
generating a very similar *Targets.cmake to what gets auto-generated
by export(). After generating the build, I see:

build/tools/lld/cmake/modules/CMakeFiles/Export/lib/cmake/lld/LLDTargets.cmake
build/lib/cmake/lld/LLDTargets.cmake
build/tools/clang/cmake/modules/CMakeFiles/Export/lib/cmake/clang/ClangTargets.cmake
build/lib/cmake/clang/ClangTargets.cmake

The cmake generated one "wins" and is what I see in the installed
tree. I'm guessing this is somehow for the benefit of old versions
of cmake?

I believe both the CMakeFiles version and the lib version are generated by CMake. The first one should just be an internal CMake file.

Any comment on "lld" vs "LLD"? Should the project() name be capitalized?

Not sure ... I think LLD is more common, but e.g. the directory is named lld. Adding a bunch of LLD COFF and ELF folks to see if they have any opinions there.

The only effect the project() name would have would be the various variables that CMake sets based on it (https://cmake.org/cmake/help/latest/command/project.html), but I don't think we're using any of those.

MaskRay added a comment.EditedMay 13 2020, 4:37 PM

I usually just follow the executable name and call it lld. In patch subjects, I follow pre-git layout and just use [ELF] and don't bother adding [lld] .
I see LLD is common, too. LLD is the string lld-link and ld.lld --version print:

% ld.lld --version
LLD 11.0.0 (compatible with GNU linkers)

I think either is ok.


For this patch, what does it intend to achieve? find_package(LLD), link against lldCOFF, lldELF etc, and call lld::elf::link, lld::wasm::link?

It'd be nice if you have an out-of-tree project or something which demonstrates that find_package(LLD) works, if you do want to use lld this way.

I usually just follow the executable name and call it lld. In patch subjects, I follow pre-git layout and just use [ELF] and don't bother adding [lld] .
I see LLD is common, too. LLD is the string lld-link and ld.lld --version print:

% ld.lld --version
LLD 11.0.0 (compatible with GNU linkers)

I think either is ok.


For this patch, what does it intend to achieve? find_package(LLD), link against lldCOFF, lldELF etc, and call lld::elf::link, lld::wasm::link?

It'd be nice if you have an out-of-tree project or something which demonstrates that find_package(LLD) works, if you do want to use lld this way.

This is to fix this

MaskRay added inline comments.May 13 2020, 10:42 PM
lld/cmake/modules/CMakeLists.txt
33

Does this need an indentation?

39

Not indented?

44

This does not need a line wrap.

lld/cmake/modules/LLDConfig.cmake.in
18

Is this used?

arsenm marked 5 inline comments as done.May 14 2020, 6:44 AM
arsenm added inline comments.
lld/cmake/modules/CMakeLists.txt
33

None of this is indented, although it does look like it is in phabricator for some reason.

lld/cmake/modules/LLDConfig.cmake.in
18

Don't think so. It looks like clang sets this property up itself for generated headers. The tablegen uses in lld look like they only produce private .inc files

arsenm closed this revision.May 14 2020, 7:01 AM
arsenm marked 2 inline comments as done.
lld/cmake/modules/LLDConfig.cmake.in
18

Removed this