Page MenuHomePhabricator

[cmake] Centralize LLVM_ENABLE_WARNINGS option
ClosedPublic

Authored by kastiglione on Sep 7 2020, 10:00 AM.

Details

Summary

Configure default value of LLVM_ENABLE_WARNINGS in HandleLLVMOptions.cmake.

LLVM_ENABLE_WARNINGS is documented as ON by default, but HandleLLVMOptions assumes the default has been set somewhere else. If it has not been explicitly set, then HandleLLVMOptions implicitly uses OFF as a default.

This removes the various option() declarations in favor of a single declaration in HandleLLVMOptions. This will prevent the unwanted use of -w that is mentioned in a couple of the comments.

Diff Detail

Event Timeline

kastiglione created this revision.Sep 7 2020, 10:00 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
kastiglione requested review of this revision.Sep 7 2020, 10:00 AM
kastiglione edited the summary of this revision. (Show Details)Sep 7 2020, 10:11 AM
kastiglione added a reviewer: JDevlieghere.

This is not nessesairy correct, at least the clang can be built in standalone mode, not as part of monorepo checkout (i.e. it's clang/cmakelists.txt is the root cmakelists, not llvm/cmakelists.txt's)

@lebedev.ri clang/CMakeLists.txt contains include(HandleLLVMOptions) for its standalone build, I think this covers the issue you're pointing out

The LLVM_ENABLE_WARNINGS variable is read only within HandleLLVMOptions.cmake. Outside declarations/defaults have effect only when HandleLLVMOptions is loaded, one way or another.

DavidTruby accepted this revision.EditedSep 8 2020, 6:48 AM

LGTM and seems to work from the flang side. Wait for another review with more experience though please :)

teemperor added a subscriber: beanz.Sep 8 2020, 6:57 AM

You need to add LLVM_ENABLE_WARNINGS to LLVMConfig.cmake.in so that the standalone builds know what value was set in the LLVM build. I think with the current patch the other projects won't inherit the value and just default to ON?

kastiglione added a comment.EditedSep 8 2020, 11:44 AM

If an LLVM install disabled LLVM_ENABLE_WARNINGS, should other builds inherit that? I would think no, but is there a precedent for that that to be the case?

The change itself is fine, but what about downstream projects which define the option? Will that trigger a warning?

If another project defines LLVM_ENABLE_WARNINGS before loading HandleLLVMOptions, it seems correct to me that the first one is used. This change ensures the default value of ON is setup at the last possible opportunity, before LLVM_ENABLE_WARNINGS is read and acted on.

To answer the question, according to a small sample, there's no warning in the case of redundant option()s.

If an LLVM install disabled LLVM_ENABLE_WARNINGS, should other builds inherit that? I would think no, but is there a precedent for that that to be the case?

Yes, most of the LLVM_ENABLE_* options work that way. I guess you could argue that some of those are different because LLVM is the one "detecting" whether they should be on, such as LLVM_ENABLE_ZLIB. But LLVM_ENABLE_EXPENSIVE_CHECKS and LLVM_ENABLE_ASSERTIONS seem conceptually very similar to LLVM_ENABLE_WARNINGS so (in my opinion) it should behave the same.

kastiglione edited the summary of this revision. (Show Details)

Add LLVM_ENABLE_WARNINGS to LLVMConfig.cmake.in

@compnerd Saleem, what do you think? (see also my reply to you)

for context this caused standalone swift-lldb builds to have warnings disabled via -w

compnerd accepted this revision.Sep 11 2020, 1:19 PM

LGTM.

This revision is now accepted and ready to land.Sep 11 2020, 1:19 PM
This revision was automatically updated to reflect the committed changes.