This is an archive of the discontinued LLVM Phabricator instance.

[cmake] Partially deduplicate `{llvm,compiler_rt}_check_linker_flag` for runtime libs and llvm
ClosedPublic

Authored by Ericson2314 on Jan 17 2022, 10:37 PM.

Details

Summary

We previously had a few varied definitions of this floating around.

I had tried to make the one installed with LLVM handle all the cases, and then made the others use it, but this ran into issues with HandleOutOfTreeLLVM not working for compiler-rt, and also CMAKE_EXE_LINKER_FLAGS not working right without CMP0056 set to the new behavior.

My compromise solution is this:

  • No not completely deduplicate: the runtime libs will instead use a version that still exists as part of the internal and not installed common shared CMake utilities. This avoids HandleOutOfTreeLLVM or a workaround for compiler-rt.
  • Continue to use CMAKE_REQUIRED_FLAGS, which effects compilation and linking. Maybe this is unnecessary, but it's safer to leave that as a future change. Also means we can avoid CMP0056 for now, to try out later, which is good incrementality too.
  • Call it llvm_check_compiler_linker_flag since it, in fact is about both per its implementation (before and after this patch), so there is no name collision.

In the future, we might still enable CMP0056 and make compiler-rt work with HandleOutOfTreeLLVM, which case we delete llvm_check_compiler_flag and go back to the old way (as these are, in fact, linking related flags), but that I leave for someone else as future work.

The original issue was reported to me in https://reviews.llvm.org/D116521#3248117 as
D116521 made clang and LLVM use the common cmake utils.

Diff Detail

Event Timeline

Ericson2314 created this revision.Jan 17 2022, 10:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2022, 10:37 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Ericson2314 requested review of this revision.Jan 17 2022, 10:37 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 17 2022, 10:37 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
sebastian-ne accepted this revision.Jan 18 2022, 1:01 AM

I verified that this fixes the issue, thanks for the fast fix!

ldionne accepted this revision.Jan 18 2022, 7:19 AM
ldionne added a subscriber: ldionne.

LGTM with rewording.

cmake/Modules/CheckLinkerFlag.cmake
0

After this patch, the two functions have different names so I don't think this comment is relevant.

This revision is now accepted and ready to land.Jan 18 2022, 7:19 AM

I'd prefer going in a different direction, that is figure out how to deduplicate llvm_check_linker_flag rather than duplicating it under a different name.

@phosek Hmm it is indeed closer than I thought. This makes me sympathetic too. This is also a compiler-rt one, and a much more complex openmp one.

LLVM's llvm_check_linker_flag is used by AddLLVM, so it must continue to be installed. But HandleOutOfTreeLLVM will ensure for libcxx and friends that the LLVM modules are indeed available?

I made D117617 as a first step towards a better version. If anyone wants this landed as a stop-gap in the interim, that is fine with me, too.

Dedup instead

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 18 2022, 3:35 PM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript

Fix module including in compiler-rt

Ericson2314 retitled this revision from [cmake] Rename `llvm_check_linker_flag` for runtime libs to avoid conflit to [cmake] Duplicate `{llvm,compiler_rt}_check_linker_flag` for runtime libs and llvm.Jan 18 2022, 3:38 PM
Ericson2314 edited the summary of this revision. (Show Details)
Ericson2314 updated this revision to Diff 401102.EditedJan 18 2022, 10:55 PM

Fix more of the bug, hopefully there are no more commas I added by mistake

Ericson2314 requested review of this revision.Jan 18 2022, 10:56 PM

The latest patch still fixes the regression in llvm (didn’t look at anything else though).

rebase to hopefully fix

compiler-rt/CMakeLists.txt
24–25

@phosek here is compiler-rt using it, as you wanted.

Fix remaining missing C/CXX arguments

My sed was looking for (" but these weren't quoted before.

Fix STREQUAL which is actually infix

phosek added inline comments.Jan 19 2022, 11:56 PM
compiler-rt/CMakeLists.txt
24–25

Thanks, can we land this separately?

compiler-rt/cmake/config-ix.cmake
168–169

This is not C++ specific so C should be enough.

169

This is not C++ specific so C should be enough.

173

This is not C++ specific so C should be enough.

184

This is not C++ specific so C should be enough.

187

This is not C++ specific so C should be enough.

433

This is not C++ specific so C should be enough.

libunwind/cmake/config-ix.cmake
37

This flag is C++ specific so this should be CXX.

runtimes/CMakeLists.txt
113

This flag is C++ specific so this should be CXX.

Ericson2314 marked an inline comment as done.Jan 20 2022, 11:39 AM

@phosek I agree with your fixing what language to do each flag, but do note I was purposely matching the existing dubious categorizations. Might it be better to first land this as-is, and then make a separate patch to clean up the languages?

compiler-rt/CMakeLists.txt
24–25

Done in D117815

phosek accepted this revision.Jan 20 2022, 12:32 PM

@phosek I agree with your fixing what language to do each flag, but do note I was purposely matching the existing dubious categorizations. Might it be better to first land this as-is, and then make a separate patch to clean up the languages?

That's fine with me as well.

This revision is now accepted and ready to land.Jan 20 2022, 12:32 PM

Hi, I suspect that this patch (or D117815) led to the build failure we're seeing on our builders (https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8824480583333895809):

CMake Error at /b/s/w/ir/x/w/llvm-llvm-project/libunwind/src/CMakeLists.txt:99 (message):
  Compiler doesn't support generation of unwind tables if exception support
  is disabled.  Building libunwind DSO with runtime dependency on C++ ABI
  library is not supported.


-- Configuring incomplete, errors occurred!
See also "/b/s/w/ir/x/w/staging/llvm_build/runtimes/runtimes-x86_64-unknown-fuchsia+asan-bins/CMakeFiles/CMakeOutput.log".
See also "/b/s/w/ir/x/w/staging/llvm_build/runtimes/runtimes-x86_64-unknown-fuchsia+asan-bins/CMakeFiles/CMakeError.log".
FAILED: runtimes/runtimes-x86_64-unknown-fuchsia+asan-stamps/runtimes-x86_64-unknown-fuchsia+asan-configure /b/s/w/ir/x/w/staging/llvm_build/runtimes/runtimes-x86_64-unknown-fuchsia+asan-stamps/runtimes-x86_64-unknown-fuchsia+asan-configure

Maybe @phosek knows if we're missing something like a cmake flag on our end or if there's something in this patch causing it.

Perhaps the difference betwen CMAKE_EXE_LINKER_FLAGS and CMAKE_REQUIRED_FLAGS is important? Also, if it is not possible to link until these libraries are built (e.g. in a cross setting) maybe I am not sure what happens.

If it was neessary these flags aren't just checked re linking, maybe we need to make a similar compat wrapper around https://cmake.org/cmake/help/latest/module/CheckCompilerFlag.html and use that instead?

I have been auto-emailed with some build failures, but they seem unrelated (see https://reviews.llvm.org/rG60f61918795b91f3fc9d310bc0957b75783f826b) so haven't taken any action doing a revert yet.

Ericson2314 retitled this revision from [cmake] Duplicate `{llvm,compiler_rt}_check_linker_flag` for runtime libs and llvm to [cmake] Deduplicate `{llvm,compiler_rt}_check_linker_flag` for runtime libs and llvm.Jan 20 2022, 6:17 PM

Perhaps the difference betwen CMAKE_EXE_LINKER_FLAGS and CMAKE_REQUIRED_FLAGS is important? Also, if it is not possible to link until these libraries are built (e.g. in a cross setting) maybe I am not sure what happens.

If it was neessary these flags aren't just checked re linking, maybe we need to make a similar compat wrapper around https://cmake.org/cmake/help/latest/module/CheckCompilerFlag.html and use that instead?

CMAKE_EXE_LINKER_FLAGS is only passed to try_compile (which is what check_${lang}_compiler_flag uses internally when CMP0056 policy is set to NEW which we don't do in LLVM.

We either need to set this policy, or change llvm_check_linker_flag to use CMAKE_REQUIRED_FLAGS instead CMAKE_EXE_LINKER_FLAGS.

Can we revert this change until we figure the best path forward?

Ericson2314 reopened this revision.Jan 21 2022, 4:51 PM
This revision is now accepted and ready to land.Jan 21 2022, 4:51 PM

New attempt splitting the difference between last 2

Ericson2314 edited the summary of this revision. (Show Details)Jan 21 2022, 5:06 PM

Fix and simplify CMAKE_REQUIRED_DEFINITIONS

Ericson2314 edited the summary of this revision. (Show Details)Jan 21 2022, 6:43 PM

Restore compromise a bit more

Ericson2314 retitled this revision from [cmake] Deduplicate `{llvm,compiler_rt}_check_linker_flag` for runtime libs and llvm to [cmake] Partially deduplicate `{llvm,compiler_rt}_check_linker_flag` for runtime libs and llvm.Jan 21 2022, 6:57 PM
Ericson2314 edited the summary of this revision. (Show Details)
Ericson2314 edited the summary of this revision. (Show Details)

Fix includes

Fix more issues

What's the reason for keeping both functions instead of changing llvm_check_linker_flag to use CMAKE_REQUIRED_FLAGS?

Ericson2314 added a comment.EditedJan 24 2022, 12:12 PM

What's the reason for keeping both functions instead of changing llvm_check_linker_flag to use CMAKE_REQUIRED_FLAGS?

  1. Abundance of caution / taking baby steps. No on had yet complained about how LLVM/Clang work yet pre the name collision.
  1. I was planning on not trying to hack in compiler-rt using llvm/cmake/modules until HandleOutOfTreeLLVM there was fixed to handle that for us more cleanly. Conversely, compiler-rt can already use cmake/Modules just fine.

@phosek does that reasoning check out to you?

@phosek does that reasoning check out to you?

I'd still like to avoid having multiple similar copies of this function in the tree.

I'd prefer starting with a smaller change that only modified llvm/cmake/modules/LLVMCheckLinkerFlag.cmake to use CMAKE_REQUIRED_FLAGS (it can also include the other changes you've made in this patch).

We can land that and if everything is working fine, I'd update this change to move llvm/cmake/modules/LLVMCheckLinkerFlag.cmake to cmake/Modules/CheckLinkerFlag.cmake and use it from everywhere except for compiler-rt which is going to need a more major cleanup.

@phosek, this patch fixes a regression that was introduced with D116521.
Could we fix this regression first with a simple patch that does not risk to be reverted again and do further refactorings afterwards?
Our downstream gcc build is broken and we’d like to re-enable it rather sooner than later.

@phosek

I'd update this change to move llvm/cmake/modules/LLVMCheckLinkerFlag.cmake to cmake/Modules/CheckLinkerFlag.cmake

Note we cannot make that move because this is referred to from installed cmake modules, whereas cmake/ is internal only and never installed.

I am a bit inclined to agree with @sebastian-ne. I don't like a bug I made staying open, and I rather get this done which at least moves us in the right direction and doesn't add any *new* sources of duplication.

We can still do the things you want to improve the situation (leaving compiler-rt) after.

kuhar added a subscriber: kuhar.Jan 28 2022, 8:40 AM

@phosek, this patch fixes a regression that was introduced with D116521.
Could we fix this regression first with a simple patch that does not risk to be reverted again and do further refactorings afterwards?
Our downstream gcc build is broken and we’d like to re-enable it rather sooner than later.

+1