[cmake] Refactor DIA SDK detection into FindDIASDK module #160354
Conversation
This consolidates the DIA SDK detection logic from
{llvm,compiler-rt}/cmake/config-ix.cmake into a new centralized,
reusable FindDIASDK.cmake module.
In addition to code deduplication, it also helps to avoid hard-coded
references to the DIA SDK location in LLVMExports.cmake, hence allowing
a pre-built LLVM distribution for Windows to be used on another host
without requiring the DIA SDK location to be the same.
Fixes llvm#86250.
Fixes llvm#100372.
Fixes llvm#111829.
Fixes llvm#152268.
Signed-off-by: Ruoyu Zhong <[email protected]>
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
cmake
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-debuginfo Author: Ruoyu Zhong (ZhongRuoyu) ChangesThis consolidates the DIA SDK detection logic from In addition to code deduplication, it also helps to avoid hard-coded references to the DIA SDK location in Fixes #86250. CC @petrhosek (LLVM CMake maintainer). Full diff: https://github.com/llvm/llvm-project/pull/160354.diff 5 Files Affected:
diff --git a/compiler-rt/cmake/config-ix.cmake b/compiler-rt/cmake/config-ix.cmake
index 67db4383ec3dc..dd89b445a04a8 100644
--- a/compiler-rt/cmake/config-ix.cmake
+++ b/compiler-rt/cmake/config-ix.cmake
@@ -728,16 +728,9 @@ if (MSVC)
set(LLVM_WINSYSROOT "" CACHE STRING
"If set, argument to clang-cl's /winsysroot")
- if (LLVM_WINSYSROOT)
- set(MSVC_DIA_SDK_DIR "${LLVM_WINSYSROOT}/DIA SDK" CACHE PATH
- "Path to the DIA SDK")
- else()
- set(MSVC_DIA_SDK_DIR "$ENV{VSINSTALLDIR}DIA SDK" CACHE PATH
- "Path to the DIA SDK")
- endif()
-
# See if the DIA SDK is available and usable.
- if (IS_DIRECTORY ${MSVC_DIA_SDK_DIR})
+ find_package(DIASDK)
+ if (DIASDK_FOUND)
set(CAN_SYMBOLIZE 1)
else()
set(CAN_SYMBOLIZE 0)
diff --git a/llvm/cmake/config-ix.cmake b/llvm/cmake/config-ix.cmake
index ed2bfa6df68f4..dcef7aefc65ec 100644
--- a/llvm/cmake/config-ix.cmake
+++ b/llvm/cmake/config-ix.cmake
@@ -625,32 +625,11 @@ if( MSVC )
set(LLVM_WINSYSROOT "" CACHE STRING
"If set, argument to clang-cl's /winsysroot")
- if (LLVM_WINSYSROOT)
- set(MSVC_DIA_SDK_DIR "${LLVM_WINSYSROOT}/DIA SDK" CACHE PATH
- "Path to the DIA SDK")
- else()
- set(MSVC_DIA_SDK_DIR "$ENV{VSINSTALLDIR}DIA SDK" CACHE PATH
- "Path to the DIA SDK")
- endif()
-
- # See if the DIA SDK is available and usable.
- # Due to a bug in MSVC 2013's installation software, it is possible
- # for MSVC 2013 to write the DIA SDK into the Visual Studio 2012
- # install directory. If this happens, the installation is corrupt
- # and there's nothing we can do. It happens with enough frequency
- # though that we should handle it. We do so by simply checking that
- # the DIA SDK folder exists. Should this happen you will need to
- # uninstall VS 2012 and then re-install VS 2013.
- if (IS_DIRECTORY "${MSVC_DIA_SDK_DIR}")
- set(HAVE_DIA_SDK 1)
- else()
- set(HAVE_DIA_SDK 0)
- endif()
-
+ find_package(DIASDK)
option(LLVM_ENABLE_DIA_SDK "Use MSVC DIA SDK for debugging if available."
- ${HAVE_DIA_SDK})
+ ${DIASDK_FOUND})
- if(LLVM_ENABLE_DIA_SDK AND NOT HAVE_DIA_SDK)
+ if(LLVM_ENABLE_DIA_SDK AND NOT DIASDK_FOUND)
message(FATAL_ERROR "DIA SDK not found. If you have both VS 2012 and 2013 installed, you may need to uninstall the former and re-install the latter afterwards.")
endif()
else()
diff --git a/llvm/cmake/modules/FindDIASDK.cmake b/llvm/cmake/modules/FindDIASDK.cmake
new file mode 100644
index 0000000000000..3eea4353a0896
--- /dev/null
+++ b/llvm/cmake/modules/FindDIASDK.cmake
@@ -0,0 +1,74 @@
+# Finds the Microsoft DIA SDK and sets DIASDK_FOUND and related variables.
+#
+# This module is intended to be used both internally by LLVM's build system and
+# by consuming projects when loading LLVMConfig.cmake.
+#
+# LLVM_WINSYSROOT may be set for locating the DIA SDK.
+#
+# If successful, the following variables will be defined:
+# DIASDK_FOUND
+# DIASDK_INCLUDE_DIR
+# DIASDK_LIBRARIES
+#
+# Additionally, the following import target will be defined:
+# DIASDK::Diaguids
+
+if(NOT WIN32)
+ set(DIASDK_FOUND FALSE)
+ return()
+endif()
+
+if(LLVM_WINSYSROOT)
+ set(MSVC_DIA_SDK_DIR "${LLVM_WINSYSROOT}/DIA SDK" CACHE PATH
+ "Path to the DIA SDK")
+else()
+ set(MSVC_DIA_SDK_DIR "$ENV{VSINSTALLDIR}DIA SDK" CACHE PATH
+ "Path to the DIA SDK")
+endif()
+
+find_path(DIASDK_INCLUDE_DIR
+ NAMES dia2.h
+ PATHS "${MSVC_DIA_SDK_DIR}/include"
+ NO_DEFAULT_PATH
+ NO_CMAKE_FIND_ROOT_PATH
+)
+
+if(IS_DIRECTORY "${MSVC_DIA_SDK_DIR}")
+ set(_DIA_SDK_LIB_DIR "${MSVC_DIA_SDK_DIR}/lib")
+
+ if("$ENV{VSCMD_ARG_TGT_ARCH}" STREQUAL "arm64")
+ set(_DIA_SDK_LIB_DIR "${_DIA_SDK_LIB_DIR}/arm64")
+ elseif("$ENV{VSCMD_ARG_TGT_ARCH}" STREQUAL "arm")
+ set(_DIA_SDK_LIB_DIR "${_DIA_SDK_LIB_DIR}/arm")
+ elseif(CMAKE_SIZEOF_VOID_P EQUAL 8)
+ set(_DIA_SDK_LIB_DIR "${_DIA_SDK_LIB_DIR}/amd64")
+ endif()
+
+ find_library(DIASDK_LIBRARIES
+ NAMES diaguids
+ PATHS "${_DIA_SDK_LIB_DIR}"
+ NO_DEFAULT_PATH
+ NO_CMAKE_FIND_ROOT_PATH
+ )
+endif()
+
+include(FindPackageHandleStandardArgs)
+find_package_handle_standard_args(
+ DIASDK
+ FOUND_VAR
+ DIASDK_FOUND
+ REQUIRED_VARS
+ DIASDK_INCLUDE_DIR
+ DIASDK_LIBRARIES
+)
+mark_as_advanced(DIASDK_INCLUDE_DIR DIASDK_LIBRARIES)
+
+if(DIASDK_FOUND)
+ if(NOT TARGET DIASDK::Diaguids)
+ add_library(DIASDK::Diaguids UNKNOWN IMPORTED)
+ set_target_properties(DIASDK::Diaguids PROPERTIES
+ IMPORTED_LOCATION "${DIASDK_LIBRARIES}"
+ INTERFACE_INCLUDE_DIRECTORIES "${DIASDK_INCLUDE_DIR}"
+ )
+ endif()
+endif()
diff --git a/llvm/cmake/modules/LLVMConfig.cmake.in b/llvm/cmake/modules/LLVMConfig.cmake.in
index 70c807abea98a..cadd3f44b6e56 100644
--- a/llvm/cmake/modules/LLVMConfig.cmake.in
+++ b/llvm/cmake/modules/LLVMConfig.cmake.in
@@ -95,6 +95,9 @@ endif()
set(LLVM_WITH_Z3 @LLVM_WITH_Z3@)
set(LLVM_ENABLE_DIA_SDK @LLVM_ENABLE_DIA_SDK@)
+if(LLVM_ENABLE_DIA_SDK)
+ find_package(DIASDK)
+endif()
set(LLVM_NATIVE_ARCH @LLVM_NATIVE_ARCH@)
diff --git a/llvm/lib/DebugInfo/PDB/CMakeLists.txt b/llvm/lib/DebugInfo/PDB/CMakeLists.txt
index b42fae41992e9..afde28914dacd 100644
--- a/llvm/lib/DebugInfo/PDB/CMakeLists.txt
+++ b/llvm/lib/DebugInfo/PDB/CMakeLists.txt
@@ -4,17 +4,9 @@ macro(add_pdb_impl_folder group)
endmacro()
if(LLVM_ENABLE_DIA_SDK)
- include_directories(SYSTEM ${MSVC_DIA_SDK_DIR}/include)
- set(LIBPDB_LINK_FOLDERS "${MSVC_DIA_SDK_DIR}\\lib")
-
- if ("$ENV{VSCMD_ARG_TGT_ARCH}" STREQUAL "arm64")
- set(LIBPDB_LINK_FOLDERS "${LIBPDB_LINK_FOLDERS}\\arm64")
- elseif ("$ENV{VSCMD_ARG_TGT_ARCH}" STREQUAL "arm")
- set(LIBPDB_LINK_FOLDERS "${LIBPDB_LINK_FOLDERS}\\arm")
- elseif (CMAKE_SIZEOF_VOID_P EQUAL 8)
- set(LIBPDB_LINK_FOLDERS "${LIBPDB_LINK_FOLDERS}\\amd64")
- endif()
- file(TO_CMAKE_PATH "${LIBPDB_LINK_FOLDERS}\\diaguids.lib" LIBPDB_ADDITIONAL_LIBRARIES)
+ find_package(DIASDK REQUIRED)
+ include_directories(SYSTEM "${DIASDK_INCLUDE_DIR}")
+ set(LIBPDB_ADDITIONAL_LIBRARIES DIASDK::Diaguids)
add_pdb_impl_folder(DIA
DIA/DIADataStream.cpp
|
|
Sorry I accidentally included an incorrect reference, opening again |
…itionalSimp. In some cases, safe-divisor selects can be hoisted out of the vector loop. Catching all cases in the legacy cost model isn't possible, in particular checking if all conditions guarding a division are loop invariant. Instead, check in planContainsAdditionalSimplifications if there are any hoisted safe-divisor selects. If so, don't compare to the more inaccurate legacy cost model. Fixes llvm/llvm-project#160354. Fixes llvm/llvm-project#160356.
|
FYI @Nerixyz, does this impact lldb too? |
I don't think so. The PDB plugin only depends on |
compnerd
left a comment
compnerd
left a comment
There was a problem hiding this comment.
This definitely is a good cleanup of the handling and using a custom Find module is moving towards the current best practices for CMake usage.
| if(NOT WIN32) | ||
| set(DIASDK_FOUND FALSE) | ||
| return() | ||
| endif() |
There was a problem hiding this comment.
Does this need to be restricted to WIN32?
There was a problem hiding this comment.
It doesn't have to, because DIA SDK will not be found anyway if we're not building for WIN32 (which, despite its name, covers the entire set of Windows targets). As far as this project is concerned, all find(DIASDK) logic is guarded by LLVM_ENABLE_DIA_SDK (as it should), which in turn is conditioned on MSVC, so I don't really expect this early return to take effect.
Open to remove/update this if you find this unnecessary/inaccurate.
There was a problem hiding this comment.
I don't think that conditional on MSVC is correct, this is not dependent on the MSVC generator nor the MSVC compiler, but rather if we are building for Windows. That said, the guard here is just avoid the unnecessary stats, which is a nice touch.
There was a problem hiding this comment.
There seems to be some old discussion about this back when the relevant part of the code was introduced:
https://reviews.llvm.org/D26255#596333:
Whether or not to useWIN32orMSC_VERis always a tricky question. I would probably useMSC_VERin this case. BecauseWIN32can be defined if you're using GCC / MinGW for example, and DIA won't won't work under those scenarios.
I agree that technically it's only the target platform that's relevant, but having played with it a bit more, it seems that the problem is not with DIA SDK, but rather with LLVM's DIA implementation (see cffff26 and its commit message). It relies on the ATL (<atlbase.h>) which is largely dependent on MS syntax like SEH __try/__except. (Though it could technically work if the non-MSVC-like compiler happens to support it?)
There was a problem hiding this comment.
The reason I raised this is the cross-compiling scenario—that is building LLVM for Windows on other platforms—which is something we're actively experimenting with. However, based on my experiments WIN32 should be set if CMake is configured correctly so this check shouldn't pose a problem.
There was a problem hiding this comment.
Thanks for the context and confirmation! Yes, I believe that should be the case, which is suggested by CMake's documentation on this variable 1. I'll keep the WIN32 guard here then.
Footnotes
It is technically possible that MSVC_DIA_SDK_DIR could not be inferred from LLVM_WINSYSROOT or VSINSTALLDIR. In that case, don't attempt to set it to a broken path at all, and emit a diagnostic message instead. Signed-off-by: Ruoyu Zhong <[email protected]>
| set(MSVC_DIA_SDK_DIR "$ENV{VSINSTALLDIR}DIA SDK" CACHE PATH | ||
| "Path to the DIA SDK") | ||
| elseif(NOT DEFINED MSVC_DIA_SDK_DIR) | ||
| message(STATUS "MSVC_DIA_SDK_DIR not set, and could not be inferred. DIA SDK " |
There was a problem hiding this comment.
IMO probably not, because the usage of find(DIASDK) may be speculative and I would defer to the caller to emit that warning if necessary.
By "speculative" I mean a user without DIA SDK installed may have relevant variables unset, yet control still enters this module from this part of the code:
llvm-project/llvm/cmake/config-ix.cmake
Lines 628 to 630 in 46c98b8
|
Gentle ping -- happy to address any additional concerns. Otherwise, it would be nice to land this and backport it to the 21.x release branch as this helps to fix a long-standing issue in the official distribution tarballs that affected a non-trivial number of users. I don't have commit access so I'll need your help to merge this. I'd appreciate your endorsement when I fulfill the requirements for commit access application. |
7 participants
This consolidates the DIA SDK detection logic from
{llvm,compiler-rt}/cmake/config-ix.cmakeinto a new centralized, reusableFindDIASDK.cmakemodule.In addition to code deduplication, it also helps to avoid hard-coded references to the DIA SDK location in
LLVMExports.cmake, hence allowing a pre-built LLVM distribution for Windows to be used on another host without requiring the DIA SDK location to be the same.Fixes #86250.
Fixes #100372.
Fixes #111829.
Fixes #152268.
CC @petrhosek (LLVM CMake maintainer).