This is an archive of the discontinued LLVM Phabricator instance.

[CMake]: Properly handle the LTO cache arguments for MinGW
ClosedPublic

Authored by thieta on May 24 2020, 10:08 AM.

Details

Summary

There where a few problems with the handling originally discussed in this issue here: https://reviews.llvm.org/D80425

We want to make sure that LINKER_IS_LLD_LINK is properly set - in this case it shouldn't be set when building for MinGW.

Then we want to make the test for it correct and finally include the option to build with thinlto cache since the MinGW driver now supports that.

Note that this will break when building with LTO and a older lld than what ships before the addition of that option in the tree. We could add logic for checking if the linker flags are correct - but I think when using LTO multi-stage builds are not uncommon.

Diff Detail

Event Timeline

thieta created this revision.May 24 2020, 10:08 AM
mstorsjo accepted this revision.May 24 2020, 11:09 AM

LGTM

Note that this will break when building with LTO and a older lld than what ships before the addition of that option in the tree. We could add logic for checking if the linker flags are correct - but I think when using LTO multi-stage builds are not uncommon.

Well, wasn't it the case that lto mingw builds were broken to begin with? So either nobody did it, or they already patched around it (and will need to adjust their patch)?

llvm/cmake/modules/HandleLLVMOptions.cmake
959

I'd feel more comfortable with parentheses here

This revision is now accepted and ready to land.May 24 2020, 11:09 AM
mati865 added inline comments.May 24 2020, 12:05 PM
llvm/cmake/modules/HandleLLVMOptions.cmake
959

AND takes precedence over OR so this code is in fact:

elseif(UNIX OR (MINGW AND LLVM_USE_LINKER STREQUAL "lld"))

So parentheses around UNIX OR MINGW are necessary.

thieta added a comment.EditedMay 24 2020, 12:43 PM

Well, wasn't it the case that lto mingw builds were broken to begin with? So either nobody did it, or they already patched around it (and will need to adjust their patch)?

Yes of course! Thanks for pointing that out.

thieta marked an inline comment as done.May 24 2020, 12:44 PM
thieta added inline comments.
llvm/cmake/modules/HandleLLVMOptions.cmake
959

I will fix this!

thieta updated this revision to Diff 265939.May 24 2020, 1:25 PM

Updated with parenthesis - feel free to commit if it looks good to you. Same as before Tobias Hieta tobias@hieta.se.

thieta marked 2 inline comments as done.May 24 2020, 1:25 PM
mati865 accepted this revision.May 24 2020, 2:10 PM
This revision was automatically updated to reflect the committed changes.