[clang][lld][cmake] Simplify header dirs

[Ericson2314]

Summary

We don't need to recompute the list LLVMConfig.cmake provides us.

When LLVM is being built, the list is two elements long: generated headers and headers from source.

When LLVM is already built,the list is one element long: the installed header directory containing both generated and hand-written sources.

[sebastian-ne]

clang/CMakeLists.txt

81

Do we need to add "${LLVM_BINARY_DIR}/include" here as well?

[Ericson2314]

clang/CMakeLists.txt

81

Pretty sure we shouldn't be cause that should be one of the elements of the list when LLVM is also being built in this CMake invocation.

[sebastian-ne]

Looks good to me, I left three questions inline.

clang/CMakeLists.txt

81

The quotation marks went missing, is this intended? (Same in lld/CMakeLists.txt)

91

The quotation marks went missing, is this intended? (Same in lld/CMakeLists.txt)

133

The quotation marks went missing, is this intended? (Same in lld/CMakeLists.txt)

[Ericson2314]

@sebastian-ne I didn't quote it because it is a list, but this might be shell script habit sneaking through. I am not sure what is best for CMake.

Checking now, https://stackoverflow.com/questions/35847655/when-should-i-quote-cmake-variables says "If your variable contains a list you normally don't add quotes" but I am not comfortable with "normally don't add"-style reasoning :).

[Ericson2314]

I tested this in my distro build setup, and reread the LLVM source to make sure the list was as I expected.

[tstellar]

When LLVM is being built, the list is two elements long: generated headers and headers from source.

All these changes are guarded by if (CLANG_BUILT_STANDALONE), which means LLVM should already be built. What build configuration are you using where you needed this change?

[Ericson2314]

@tstellar There wasn't a configuration necessitating these changes.

The CMake config file currently distinguishes a main include dir and secondary include dir, which feels to me like it is leaking the implementation details of which headers are generated or not. I was hoping by replacing the instances where those are used with the list one, we'd eventually be able to deprecate the two underlying variables and just have the list one left.

clang/CMakeLists.txt

if(CLANG_BUILT_STANDALONE)
   find_package(LLVM REQUIRED HINTS "${LLVM_CMAKE_DIR}")
   list(APPEND CMAKE_MODULE_PATH "${LLVM_DIR}")
 
   # We can't check LLVM_CONFIG here, because find_package(LLVM ...) also sets
   # LLVM_CONFIG.
   if (NOT LLVM_CONFIG_FOUND)
     # Pull values from LLVMConfig.cmake.  We can drop this once the llvm-config
     # path is removed.
-    set(MAIN_INCLUDE_DIR "${LLVM_INCLUDE_DIR}")
+    set(INCLUDE_DIRS ${LLVM_INCLUDE_DIRS})
     set(LLVM_OBJ_DIR "${LLVM_BINARY_DIR}")
     # N.B. this is just a default value, the CACHE PATHs below can be overriden.
     set(MAIN_SRC_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../llvm")
     set(TOOLS_BINARY_DIR "${LLVM_TOOLS_BINARY_DIR}")
     set(LIBRARY_DIR "${LLVM_LIBRARY_DIR}")
+  else()
+    set(INCLUDE_DIRS "${LLVM_BINARY_DIR}/include" "${MAIN_INCLUDE_DIR}")
   endif()
 
-  set(LLVM_MAIN_INCLUDE_DIR "${MAIN_INCLUDE_DIR}" CACHE PATH "Path to llvm/include")
+  set(LLVM_INCLUDE_DIRS ${INCLUDE_DIRS} CACHE PATH "Path to llvm/include and any other header dirs needed")
   set(LLVM_BINARY_DIR "${LLVM_OBJ_ROOT}" CACHE PATH "Path to LLVM build tree")
   set(LLVM_MAIN_SRC_DIR "${MAIN_SRC_DIR}" CACHE PATH "Path to LLVM source tree")
   set(LLVM_TOOLS_BINARY_DIR "${TOOLS_BINARY_DIR}" CACHE PATH "Path to llvm/bin")
   set(LLVM_LIBRARY_DIR "${LIBRARY_DIR}" CACHE PATH "Path to llvm/lib")
 
   find_program(LLVM_TABLEGEN_EXE "llvm-tblgen" ${LLVM_TOOLS_BINARY_DIR}
     NO_DEFAULT_PATH)
 
if(CLANG_BUILT_STANDALONE)
   set(PACKAGE_VERSION "${LLVM_PACKAGE_VERSION}")
   set(BUG_REPORT_URL "${LLVM_PACKAGE_BUGREPORT}" CACHE STRING
     "Default URL where bug reports are to be submitted.")
 
   if (NOT DEFINED LLVM_INCLUDE_TESTS)
     set(LLVM_INCLUDE_TESTS ON)
   endif()
 
-  include_directories("${LLVM_BINARY_DIR}/include" "${LLVM_MAIN_INCLUDE_DIR}")
+  include_directories(${LLVM_INCLUDE_DIRS})
   link_directories("${LLVM_LIBRARY_DIR}")
 
   set( CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin )
   set( CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX} )
   set( CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX} )
 
   if(LLVM_INCLUDE_TESTS)
     find_package(Python3 ${LLVM_MINIMUM_PYTHON_VERSION} REQUIRED

lld/CMakeLists.txt

if(LLD_BUILT_STANDALONE)
   find_package(LLVM REQUIRED HINTS "${LLVM_CMAKE_DIR}")
   list(APPEND CMAKE_MODULE_PATH "${LLVM_DIR}")
 
   # We can't check LLVM_CONFIG here, because find_package(LLVM ...) also sets
   # LLVM_CONFIG.
   if (NOT LLVM_CONFIG_FOUND)
     # Pull values from LLVMConfig.cmake.  We can drop this once the llvm-config
     # path is removed.
-    set(MAIN_INCLUDE_DIR "${LLVM_INCLUDE_DIR}")
+    set(INCLUDE_DIRS ${LLVM_INCLUDE_DIRS})
     set(LLVM_OBJ_DIR "${LLVM_BINARY_DIR}")
     # N.B. this is just a default value, the CACHE PATHs below can be overridden.
     set(MAIN_SRC_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../llvm")
+  else()
+    set(INCLUDE_DIRS "${LLVM_BINARY_DIR}/include" "${MAIN_INCLUDE_DIR}")
   endif()
 
-  set(LLVM_MAIN_INCLUDE_DIR "${MAIN_INCLUDE_DIR}" CACHE PATH "Path to llvm/include")
+  set(LLVM_INCLUDE_DIRS ${INCLUDE_DIRS} CACHE PATH "Path to llvm/include and any other header dirs needed")
   set(LLVM_BINARY_DIR "${LLVM_OBJ_ROOT}" CACHE PATH "Path to LLVM build tree")
   set(LLVM_MAIN_SRC_DIR "${MAIN_SRC_DIR}" CACHE PATH "Path to LLVM source tree")
 
   find_program(LLVM_TABLEGEN_EXE "llvm-tblgen" ${LLVM_TOOLS_BINARY_DIR}
     NO_DEFAULT_PATH)
 
   # They are used as destination of target generators.
   set(LLVM_RUNTIME_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin)
   set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
 
   include(AddLLVM)
   include(TableGen)
   include(HandleLLVMOptions)
   include(GetErrcMessages)
   include(CheckAtomic)
 
   set(PACKAGE_VERSION "${LLVM_PACKAGE_VERSION}")
 
-  include_directories("${LLVM_BINARY_DIR}/include" ${LLVM_INCLUDE_DIRS})
+  include_directories(${LLVM_INCLUDE_DIRS})
   link_directories(${LLVM_LIBRARY_DIRS})
 
   if(LLVM_INCLUDE_TESTS)
     find_package(Python3 ${LLVM_MINIMUM_PYTHON_VERSION} REQUIRED
       COMPONENTS Interpreter)
 
     # Check prebuilt llvm/utils.
     if(EXISTS ${LLVM_TOOLS_BINARY_DIR}/FileCheck${CMAKE_EXECUTABLE_SUFFIX}