lib/Header: Simplify CMakeLists.txt

[tstellar]

Summary

Replace cut and pasted code with cmake macros and reduce the number of
install commands. This fixes an issue where the headers were being
installed twice.

This clean up should also make future modifications easier, like
adding a cmake option to install header files into a custom resource
directory.

[smeenai]

LGTM, though I generally prefer functions to macros.

clang/lib/Headers/CMakeLists.txt

127–130

I'd prefer this be converted to a function and the append to out_files be handled explicitly via PARENT_SCOPE (you could either hardcode the name or take in a parameter for the name of the list to append to). Macros pollute variables and their parameters behave weirdly.

138

This one can trivially be a function instead of a macro, right?

[tstellar]

Use macros instead of functions.

[tstellar]

In D58537#1411657, @tstellar wrote:

Use macros instead of functions.

I mean "Use functions instead of macros."

[smeenai]

LGTM, thanks!

clang/lib/Headers/CMakeLists.txt

136

I don't think this comment is necessary – this is a pretty common CMake pattern, and I think the PARENT_SCOPE is enough of a search hint for people who aren't familiar with it.

[tstellar]

Fix an issue with the generated arm headers that I discovered after
doing some more testing. Also, remove comment.

[smeenai]

@tstellar are you planning to land this soon? It'll conflict with D58791, but I'm not planning to land that for another few days, so I can rebase on top of this one.

[tstellar]

In D58537#1415534, @smeenai wrote:

@tstellar are you planning to land this soon? It'll conflict with D58791, but I'm not planning to land that for another few days, so I can rebase on top of this one.

Yes, I'll push this later today.

[vzakhari]

cfe/trunk/lib/Headers/CMakeLists.txt

168 ↗

(On Diff #189010)

This change results in a use of CMAKE_CFG_INTDIR, which expands to $(Configuration) inside cmake_install.cmake, when using Visual Studio generator. CMake cannot reasonably expand $(Configuration), so Visual Studio builds are broken for me after this change-set.

Can we revert to installation from FILES relative to the current source dir? This will require keeping a separate list of source files in copy_header_to_output_dir(), and using this list in the install() command.

I do understand that the intention was, probably, to copy headers files into output_dir along with creating some structure inside output_dir, and then installing the whole output_dir verbatim to the install dir. It will be hard to achieve this with the change I suggest, but can we fix Visual Studio builds in any other way?

FWIW, vcproj invokes the installation with the following command: cmake -DBUILD_TYPE=$(Configuration) -P cmake_install.cmake
CMake could have expanded CMAKE_CFG_INTDIR as ${BUILD_TYPE} instead of $(Configuration) inside cmake_install.cmake.

[smeenai]

@tstellar ping. Someone appears to be running into this on the CMake mailing list too: https://cmake.org/pipermail/cmake/2019-April/069359.html

[tstellar]

In D58537#1474824, @smeenai wrote:

@tstellar ping. Someone appears to be running into this on the CMake mailing list too: https://cmake.org/pipermail/cmake/2019-April/069359.html

Sorry, I missed this. I will take a look.

[tstellar]

cfe/trunk/lib/Headers/CMakeLists.txt

168 ↗

(On Diff #189010)

This change results in a use of CMAKE_CFG_INTDIR, which expands to $(Configuration) inside cmake_install.cmake, when using Visual Studio generator. CMake cannot reasonably expand $(Configuration), so Visual Studio builds are broken for me after this change-set.

Prior to this change we were installing the arm generated files from ${CMAKE_CURRENT_BINARY_DIR}. Do we really need to use ${LLVM_LIBRARY_OUTPUT_INTDIR} as the base for output_dir? Could we use ${CMAKE_CURRENT_BINARY_DIR} instead? That seems like it would fix this issue.

FWIW, vcproj invokes the installation with the following command: cmake -DBUILD_TYPE=$(Configuration) -P cmake_install.cmake

CMake could have expanded CMAKE_CFG_INTDIR as ${BUILD_TYPE} instead of $(Configuration) inside cmake_install.cmake.

Are the vc project files part of the LLVM source tree?

[vzakhari]

cfe/trunk/lib/Headers/CMakeLists.txt

168 ↗

(On Diff #189010)

As I understand, ${CMAKE_CURRENT_BINARY_DIR} is the same directory for all (Debug, Release) builds for the multiconfiguration builders. I am not sure if it is possible to run Debug and Release builds in parallel, which would imply parallel access to the generated files. Maybe it is a potential problem, but we have it even now inside clang_generate_header() function.

VC project files are generated by CMake, that is why I said CMake could have used ${BUILD_TYPE} in the cmake_install.cmake files instead of $(Configuration).

I guess just using ${CMAKE_CURRENT_BINARY_DIR} as the output_dir will solve the current problem. It should work as long as Debug and Release versions of the header files are functionally equivalent, and noone runs Debug and Release builds in parallel.

[tstellar]

Can you test D61054?

cfe/trunk/lib/Headers/CMakeLists.txt

168 ↗

(On Diff #189010)

I guess just using ${CMAKE_CURRENT_BINARY_DIR} as the output_dir will solve the current problem. It should work as long as Debug and Release versions of the header files are functionally equivalent, and noone runs Debug and Release builds in parallel.

Ok, the generated files were already installing from ${CMAKE_CURRENT_BINARY_DIR}, so if that is a problem than this was already broken before this patch. The header files should be equivalent for all build types anyway.

[vzakhari]

In D58537#1476570, @tstellar wrote:

Can you test D61054?

I will do it ASAP, but I am currently having problems with my build system. I guess it is OK to proceed with this change. I will send an update, when I try it. Thank you for fixing this!

clang/lib/Headers/CMakeLists.txt

 
 set(cuda_wrapper_files
   cuda_wrappers/algorithm
   cuda_wrappers/complex
   cuda_wrappers/new
 )
 
 set(output_dir ${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION}/include)
-
-# Generate arm_neon.h
-clang_tablegen(arm_neon.h -gen-arm-neon
-  -I ${CLANG_SOURCE_DIR}/include/clang/Basic/
-  SOURCE ${CLANG_SOURCE_DIR}/include/clang/Basic/arm_neon.td)
-# Generate arm_fp16.h
-clang_tablegen(arm_fp16.h -gen-arm-fp16
-  -I ${CLANG_SOURCE_DIR}/include/clang/Basic/
-  SOURCE ${CLANG_SOURCE_DIR}/include/clang/Basic/arm_fp16.td)
-
 set(out_files)
-foreach( f ${files} ${cuda_wrapper_files} )
-  set( src ${CMAKE_CURRENT_SOURCE_DIR}/${f} )
-  set( dst ${output_dir}/${f} )
+
+function(copy_header_to_output_dir src_dir file)
+  set(src ${src_dir}/${file})
+  set(dst ${output_dir}/${file})
   add_custom_command(OUTPUT ${dst}
     DEPENDS ${src}
     COMMAND ${CMAKE_COMMAND} -E copy_if_different ${src} ${dst}
-    COMMENT "Copying clang's ${f}...")
+    COMMENT "Copying clang's ${file}...")
   list(APPEND out_files ${dst})
+  # The list function only updates out_files in the current scope.  We need
+  # call set in order to also update the variable for the parent's scope.
+  # The parent in this case is the caller of the function.
+  set(out_files ${out_files} PARENT_SCOPE)
+endfunction(copy_header_to_output_dir)
+
+function(clang_generate_header td_option td_file file_list)
+  clang_tablegen(${out_file} ${td_option}
+  -I ${CLANG_SOURCE_DIR}/include/clang/Basic/
+  SOURCE ${CLANG_SOURCE_DIR}/include/clang/Basic/${td_file})
+
+  copy_header_to_output_dir(${CMAKE_CURRENT_BINARY_DIR} out_file file_list)
+endfunction(clang_generate_header)
+
+
+# Copy header files from the source directory to the build directory
+foreach( f ${files} ${cuda_wrapper_files} )
+  copy_header_to_output_dir(${CMAKE_CURRENT_SOURCE_DIR} ${f})
 endforeach( f )
 
-add_custom_command(OUTPUT ${output_dir}/arm_neon.h
-  DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h
-  COMMAND ${CMAKE_COMMAND} -E copy_if_different ${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h ${output_dir}/arm_neon.h
-  COMMENT "Copying clang's arm_neon.h...")
-list(APPEND out_files ${output_dir}/arm_neon.h)
-add_custom_command(OUTPUT ${output_dir}/arm_fp16.h
-  DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/arm_fp16.h
-  COMMAND ${CMAKE_COMMAND} -E copy_if_different ${CMAKE_CURRENT_BINARY_DIR}/arm_fp16.h ${output_dir}/arm_fp16.h
-  COMMENT "Copying clang's arm_fp16.h...")
-list(APPEND out_files ${output_dir}/arm_fp16.h)
+# Generate header files and copy them to the build directory
+# Generate arm_neon.h
+clang_generate_header(-gen-arm-neon arm_neon.td arm_neon.h)
+# Generate arm_fp16.h
+clang_generate_header(-gen-arm-fp16 arm_fp16.td arm_fp16.h)
 
 add_custom_target(clang-headers ALL DEPENDS ${out_files})
 set_target_properties(clang-headers PROPERTIES
   FOLDER "Misc"
   RUNTIME_OUTPUT_DIRECTORY "${output_dir}")
 
-install(
-  FILES ${files} ${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h
-  COMPONENT clang-headers
-  PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
-  DESTINATION lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}/include)
+set(header_install_dir lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION})
 
 install(
-  FILES ${files} ${CMAKE_CURRENT_BINARY_DIR}/arm_fp16.h
-  COMPONENT clang-headers
-  PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
-  DESTINATION lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}/include)
-
-install(
-  FILES ${cuda_wrapper_files}
-  COMPONENT clang-headers
-  PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
-  DESTINATION lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}/include/cuda_wrappers)
+  DIRECTORY ${output_dir}
+  DESTINATION ${header_install_dir}
+  COMPONENT clang-headers)
 
 if (NOT LLVM_ENABLE_IDE)
   add_llvm_install_targets(install-clang-headers
                            DEPENDS clang-headers
                            COMPONENT clang-headers)
 endif()