[cmake] Add LLVM_ENABLE_DIA_SDK option, and expose it in LLVMConfig

[mgorny]

Summary

Add an explicit LLVM_ENABLE_DIA_SDK option to control building support
for DIA SDK-based debugging. Control its value to match whether DIA SDK
support was found and expose it in LLVMConfig (alike LLVM_ENABLE_ZLIB).

Its value is needed for LLDB to determine whether to run tests requiring
DIA support. Currently it is obtained from llvm/Config/config.h;
however, this file is not available for standalone builds. Following
this change, LLDB will be modified to use the value from LLVMConfig.

[zturner]

include/llvm/Config/config.h.cmake

42

What's the difference between #cmakedefine01 and the syntax you've used here?

[mgorny]

include/llvm/Config/config.h.cmake

42

1. #cmakedefine01 converts any CMake bool to 0/1, mine relies on the 0/1 being set in CMake (it was already done like this).
2. #cmakedefine01 does not support different #define and CMake variable name (or at least doesn't mention it in the docs). So if I were to continue using #cmakedefine01, I'd either have to define additional HAVE_DIA_SDK var in CMake, or rename the #define.

[beanz]

I think you're conflating two different types of variables, and I don't understand why.

LLVM_ENABLE_* variables are intended to be used for people to opt in or out of functionality. HAVE_* variables are for compiler checks against the system capabilities. Generally system capabilities shouldn't be captured in LLVM installed bits, which is what you're doing here.

I think the correct solution would be for you to perform the HAVE_DIA_SDK check in LLDB as well as LLVM. If the code is sufficiently complex, the LLVM version of the code could be abstracted into a module and re-used.

[mgorny]

In D26255#588591, @beanz wrote:

I think you're conflating two different types of variables, and I don't understand why.

LLVM_ENABLE_* variables are intended to be used for people to opt in or out of functionality. HAVE_* variables are for compiler checks against the system capabilities. Generally system capabilities shouldn't be captured in LLVM installed bits, which is what you're doing here.

I was considering this issue, and I've decided having user control over DIA SDK a worthwhile future effort. While this patch doesn't add it, it prepares the stuff to use it when it's added.

I think the correct solution would be for you to perform the HAVE_DIA_SDK check in LLDB as well as LLVM. If the code is sufficiently complex, the LLVM version of the code could be abstracted into a module and re-used.

DIA support enables some PDB debug format support in LLVM, that is unavailable otherwise. LLDB doesn't use DIA at all but the PDB tests require DIA support enabled during LLVM build. In this regard, it's similar to zlib support, upon which I based my patch.

[mgorny]

@beanz, how about this one? I've added an explicit LLVM_ENABLE_DIA_SDK option to let people control this explicitly.

[beanz]

If LLVM_ENABLE_DIA_SDK is On and you don't have a the SDK available that should be a fatal error. Treating it this way will also allow you to clean up some of the logic you're modifying in config-ix.cmake. Also gate the option under WIN32 so it only shows up if you're targeting windows.

With those changes I think this looks reasonable.

[mgorny]

In D26255#596037, @beanz wrote:

If LLVM_ENABLE_DIA_SDK is On and you don't have a the SDK available that should be a fatal error. Treating it this way will also allow you to clean up some of the logic you're modifying in config-ix.cmake.

I was thinking about that but it seems that other options (like LLVM_ENABLE_ZLIB) also are non-fatal like this. While I don't mind changing this, I'm not sure how to do it properly so that it doesn't suddenly fail for a lot of users.

One option would be to detect DIA SDK first, and base the default off it. However, I don't see doing that cleanly without moving the option to config-ix. Another would be to make the option 'trinary', and put some special default like 'AUTO' — i.e. AUTO meaning use if present, ON meaning fail if non-present.

Also gate the option under WIN32 so it only shows up if you're targeting windows.

WIN32 or MSVC? The code is currently using the latter.

[beanz]

Moving the option into config-ix is fine, and basing the default off whether or not the SDK is present seems like a good way to handle this.

I'll defer to you whether WIN32 or MSVC is correct. WIN32 is for if you are targeting Windows, and MSVC is if your compiler is MSVC-compatible. WIN32 seems right to me, but I don't know.

[zturner]

Whether or not to use WIN32 or MSC_VER is always a tricky question. I would probably use MSC_VER in this case. Because WIN32 can be defined if you're using GCC / MinGW for example, and DIA won't won't work under those scenarios.

[beanz]

Seems like MSVC is the right way to go. That will be defined if your compiler is MSVC or clang-cl.

[mgorny]

Ok, I think that'd be the best version so far.

The option is now exposed on MSVC only, and defaults to autodetection result. If it is forced ON and DIA SDK is not available, it errors out verbosely and explains the potential problem (based on comment above the check). On non-MSVC platforms, LLVM_ENABLE_DIA_SDK is set to 0 for interoperability.

I've also went ahead and changed the #define name. The LLDB patch is already approved, so I can commit both simultaneously.

[mgorny]

Oh, since I don't have MSVC, could someone test this on MSVC?

[mgorny]

Ping.

[mgorny]

@beanz, ping.

[compnerd]

cmake/config-ix.cmake

443

No spaces inside the parenthesis.

cmake/config-ix.cmake

if( MSVC )
   # 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)
+    set(LLVM_ENABLE_DIA_SDK 1)
   else()
-    set(HAVE_DIA_SDK 0)
+    set(LLVM_ENABLE_DIA_SDK 0)
   endif()
 else()
-  set(HAVE_DIA_SDK 0)
+  set(LLVM_ENABLE_DIA_SDK 0)
 endif( MSVC )
 
 # FIXME: Signal handler return type, currently hardcoded to 'void'
 set(RETSIGTYPE void)
 
 if( LLVM_ENABLE_THREADS )
   # Check if threading primitives aren't supported on this platform
   if( NOT HAVE_PTHREAD_H AND NOT WIN32 )
     set(LLVM_ENABLE_THREADS 0)
   endif()
 endif()

cmake/modules/LLVMConfig.cmake.in

 set(LLVM_ENABLE_RTTI @LLVM_ENABLE_RTTI@)
 
 set(LLVM_ENABLE_TERMINFO @LLVM_ENABLE_TERMINFO@)
 
 set(LLVM_ENABLE_THREADS @LLVM_ENABLE_THREADS@)
 
 set(LLVM_ENABLE_ZLIB @LLVM_ENABLE_ZLIB@)
 
+set(LLVM_ENABLE_DIA_SDK @LLVM_ENABLE_DIA_SDK@)
+
 set(LLVM_NATIVE_ARCH @LLVM_NATIVE_ARCH@)
 
 set(LLVM_ENABLE_PIC @LLVM_ENABLE_PIC@)
 
 set(LLVM_BUILD_32_BITS @LLVM_BUILD_32_BITS@)
 
 set(LLVM_ENABLE_PLUGINS @LLVM_ENABLE_PLUGINS@)
 set(LLVM_EXPORT_SYMBOLS_FOR_PLUGINS @LLVM_EXPORT_SYMBOLS_FOR_PLUGINS@)

include/llvm/Config/config.h.cmake

/* Define to 1 if you have the declaration of `FE_INEXACT', and to 0 if you
    don't. */
 #cmakedefine01 HAVE_DECL_FE_INEXACT
 
 /* Define to 1 if you have the declaration of `strerror_s', and to 0 if you
    don't. */
 #cmakedefine01 HAVE_DECL_STRERROR_S
 
 /* Define to 1 if you have the DIA SDK installed, and to 0 if you don't. */
-#cmakedefine01 HAVE_DIA_SDK
+#define HAVE_DIA_SDK ${LLVM_ENABLE_DIA_SDK}
 
 /* Define to 1 if you have the <dirent.h> header file, and it defines `DIR'.
    */
 #cmakedefine HAVE_DIRENT_H ${HAVE_DIRENT_H}
 
 /* Define to 1 if you have the <dlfcn.h> header file. */
 #cmakedefine HAVE_DLFCN_H ${HAVE_DLFCN_H}
 

lib/DebugInfo/PDB/CMakeLists.txt

 macro(add_pdb_impl_folder group)
   list(APPEND PDB_IMPL_SOURCES ${ARGN})
   source_group(${group} FILES ${ARGN})
 endmacro()
 
-if(HAVE_DIA_SDK)
+if(LLVM_ENABLE_DIA_SDK)
   include_directories(${MSVC_DIA_SDK_DIR}/include)
   set(LIBPDB_LINK_FOLDERS "${MSVC_DIA_SDK_DIR}\\lib")
   if (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)
 
   add_pdb_impl_folder(DIA

test/lit.site.cfg.in

 config.host_arch = "@HOST_ARCH@"
 config.host_cc = "@HOST_CC@"
 config.host_cxx = "@HOST_CXX@"
 config.host_ldflags = "@HOST_LDFLAGS@"
 config.llvm_use_intel_jitevents = "@LLVM_USE_INTEL_JITEVENTS@"
 config.llvm_use_sanitizer = "@LLVM_USE_SANITIZER@"
 config.have_zlib = "@HAVE_LIBZ@"
 config.have_libxar = "@HAVE_LIBXAR@"
-config.have_dia_sdk = @HAVE_DIA_SDK@
+config.have_dia_sdk = @LLVM_ENABLE_DIA_SDK@
 config.enable_ffi = "@LLVM_ENABLE_FFI@"
 config.test_examples = "@ENABLE_EXAMPLES@"
 
 # Support substitution of the tools_dir with user parameters. This is
 # used when we can't determine the tool dir at configuration time.
 try:
     config.llvm_tools_dir = config.llvm_tools_dir % lit_config.params
     config.llvm_shlib_dir = config.llvm_shlib_dir % lit_config.params
 except KeyError:
     e = sys.exc_info()[1]
     key, = e.args
     lit_config.fatal("unable to find %r parameter, use '--param=%s=VALUE'" % (key,key))
 
 # Let the main config do the real work.
 lit_config.load_config(config, "@LLVM_SOURCE_DIR@/test/lit.cfg")