[cmake] Centralize LLVM_ENABLE_WARNINGS option

[kastiglione]

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.

[lebedev.ri]

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)

[kastiglione]

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

[kastiglione]

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]

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

[JDevlieghere]

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]

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?

[compnerd]

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

[kastiglione]

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.

[JDevlieghere]

In D87243#2261687, @kastiglione wrote:

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]

Add LLVM_ENABLE_WARNINGS to LLVMConfig.cmake.in

[JDevlieghere]

LGTM

[kastiglione]

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

[kastiglione]

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

[compnerd]

LGTM.

clang/CMakeLists.txt

if( CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR )
   set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
   if(WIN32 OR CYGWIN)
     # DLL platform -- put DLLs into bin.
     set(LLVM_SHLIB_OUTPUT_INTDIR ${LLVM_RUNTIME_OUTPUT_INTDIR})
   else()
     set(LLVM_SHLIB_OUTPUT_INTDIR ${LLVM_LIBRARY_OUTPUT_INTDIR})
   endif()
 
-  option(LLVM_ENABLE_WARNINGS "Enable compiler warnings." ON)
   option(LLVM_INSTALL_TOOLCHAIN_ONLY
     "Only include toolchain files in the 'install' target." OFF)
 
   option(LLVM_FORCE_USE_OLD_HOST_TOOLCHAIN
     "Set to ON to force using an old, unsupported host toolchain." OFF)
   option(CLANG_ENABLE_BOOTSTRAP "Generate the clang bootstrap target" OFF)
   option(LLVM_ENABLE_LIBXML2 "Use libxml2 if available." ON)
 

flang/CMakeLists.txt

if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
   # We need a pre-built/installed version of LLVM.
   find_package(LLVM REQUIRED HINTS "${LLVM_CMAKE_PATH}")
   list(APPEND CMAKE_MODULE_PATH ${LLVM_DIR})
 
   # If LLVM links to zlib we need the imported targets so we can too.
   if(LLVM_ENABLE_ZLIB)
     find_package(ZLIB REQUIRED)
   endif()
-  option(LLVM_ENABLE_WARNINGS "Enable compiler warnings." ON)
   option(LLVM_ENABLE_PEDANTIC "Compile with pedantic enabled." ON)
   if(CMAKE_COMPILER_IS_GNUCXX)
     set(USE_NO_MAYBE_UNINITIALIZED 1)
   endif()
 
   include(CMakeParseArguments)
   include(AddLLVM)
   include(HandleLLVMOptions)

libunwind/CMakeLists.txt

if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR OR LIBUNWIND_STANDALONE_BUILD)
   else()
     message(WARNING "UNSUPPORTED LIBUNWIND CONFIGURATION DETECTED: "
                     "llvm-config not found and LLVM_MAIN_SRC_DIR not defined. "
                     "Reconfigure with -DLLVM_CONFIG=path/to/llvm-config "
                     "or -DLLVM_PATH=path/to/llvm-source-root.")
   endif()
 
   if (EXISTS ${LLVM_CMAKE_PATH})
-    # Enable warnings, otherwise -w gets added to the cflags by HandleLLVMOptions.
-    set(LLVM_ENABLE_WARNINGS ON)
     list(APPEND CMAKE_MODULE_PATH "${LLVM_CMAKE_PATH}")
     include("${LLVM_CMAKE_PATH}/AddLLVM.cmake")
     include("${LLVM_CMAKE_PATH}/HandleLLVMOptions.cmake")
   else()
     message(WARNING "Not found: ${LLVM_CMAKE_PATH}")
   endif()
 
   set(PACKAGE_NAME libunwind)

llvm/CMakeLists.txt

 endif()
 
 set(LLVM_TARGETS_TO_BUILD
    ${LLVM_TARGETS_TO_BUILD}
    ${LLVM_EXPERIMENTAL_TARGETS_TO_BUILD})
 list(REMOVE_DUPLICATES LLVM_TARGETS_TO_BUILD)
 
 option(LLVM_ENABLE_PIC "Build Position-Independent Code" ON)
-option(LLVM_ENABLE_WARNINGS "Enable compiler warnings." ON)
 option(LLVM_ENABLE_MODULES "Compile with C++ modules enabled." OFF)
 if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
   option(LLVM_ENABLE_MODULE_DEBUGGING "Compile with -gmodules." ON)
 else()
   option(LLVM_ENABLE_MODULE_DEBUGGING "Compile with -gmodules." OFF)
 endif()
 option(LLVM_ENABLE_LOCAL_SUBMODULE_VISIBILITY "Compile with -fmodules-local-submodule-visibility." ON)
 option(LLVM_ENABLE_LIBCXX "Use libc++ if available." OFF)

llvm/cmake/modules/HandleLLVMOptions.cmake

elseif(MINGW) # FIXME: Also cygwin?
   # Pass -mbig-obj to mingw gas on Win64. COFF has a 2**16 section limit, and
   # on Win64, every COMDAT function creates at least 3 sections: .text, .pdata,
   # and .xdata.
   if (CMAKE_SIZEOF_VOID_P EQUAL 8)
     append("-Wa,-mbig-obj" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
   endif()
 endif()
 
+option(LLVM_ENABLE_WARNINGS "Enable compiler warnings." ON)
+
 if( MSVC )
   include(ChooseMSVCCRT)
 
   # Add definitions that make MSVC much less annoying.
   add_definitions(
     # For some reason MS wants to deprecate a bunch of standard functions...
     -D_CRT_SECURE_NO_DEPRECATE
     -D_CRT_SECURE_NO_WARNINGS

llvm/cmake/modules/LLVMConfig.cmake.in

 set(LLVM_TARGETS_WITH_JIT @LLVM_TARGETS_WITH_JIT@)
 
 @all_llvm_lib_deps@
 
 set(TARGET_TRIPLE "@TARGET_TRIPLE@")
 
 set(LLVM_ABI_BREAKING_CHECKS @LLVM_ABI_BREAKING_CHECKS@)
 
+set(LLVM_ENABLE_WARNINGS @LLVM_ENABLE_WARNINGS@)
+
 set(LLVM_ENABLE_EXPENSIVE_CHECKS @LLVM_ENABLE_EXPENSIVE_CHECKS@)
 
 set(LLVM_ENABLE_ASSERTIONS @LLVM_ENABLE_ASSERTIONS@)
 
 set(LLVM_ENABLE_EH @LLVM_ENABLE_EH@)
 
 set(LLVM_ENABLE_RTTI @LLVM_ENABLE_RTTI@)
 

llvm/runtimes/CMakeLists.txt

if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_CURRENT_SOURCE_DIR})
   include(CheckCCompilerFlag)
 
   # We don't have libc++ (yet).
   set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -nostdlib++")
 
   # Avoid checking whether the compiler is working.
   set(LLVM_COMPILER_CHECKED ON)
 
-  # Enable warnings, otherwise -w gets added to the cflags by HandleLLVMOptions
-  # resulting in unjustified successes by check_cxx_compiler_flag.
-  set(LLVM_ENABLE_WARNINGS ON)
-
   # Handle common options used by all runtimes.
   include(AddLLVM)
   include(HandleLLVMOptions)
   include(FindPythonInterp)
 
   # Remove the -nostdlib++ option we've added earlier.
   string(REPLACE "-nostdlib++" "" CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS}")