[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 ↗

(On Diff #76765)

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

[mgorny]

include/llvm/Config/config.h.cmake

42 ↗

(On Diff #76765)

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 ↗

(On Diff #78071)

No spaces inside the parenthesis.

llvm/trunk/cmake/config-ix.cmake

if( MSVC )
   # 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()
+
+  option(LLVM_ENABLE_DIA_SDK "Use MSVC DIA SDK for debugging if available."
+                             ${HAVE_DIA_SDK})
+
+  if(LLVM_ENABLE_DIA_SDK AND NOT HAVE_DIA_SDK)
+    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()
-  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 )

llvm/trunk/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@)

llvm/trunk/docs/CMake.rst

 
 **LLVM_USE_INTEL_JITEVENTS**:BOOL
   Enable building support for Intel JIT Events API. Defaults to OFF.
 
 **LLVM_ENABLE_ZLIB**:BOOL
   Enable building with zlib to support compression/uncompression in LLVM tools.
   Defaults to ON.
 
+**LLVM_ENABLE_DIA_SDK**:BOOL
+  Enable building with MSVC DIA SDK for PDB debugging support. Available
+  only with MSVC. Defaults to ON.
+
 **LLVM_USE_SANITIZER**:STRING
   Define the sanitizer used to build LLVM binaries and tests. Possible values
   are ``Address``, ``Memory``, ``MemoryWithOrigins``, ``Undefined``, ``Thread``,
   and ``Address;Undefined``. Defaults to empty string.
 
 **LLVM_ENABLE_LTO**:STRING
   Add ``-flto`` or ``-flto=`` flags to the compile and link command
   lines, enabling link-time optimization. Possible values are ``Off``,

llvm/trunk/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
+#cmakedefine01 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}
 

llvm/trunk/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

llvm/trunk/lib/DebugInfo/PDB/PDB.cpp

 //===- PDB.cpp - base header file for creating a PDB reader -----*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/DebugInfo/PDB/PDB.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Config/config.h"
 #include "llvm/DebugInfo/PDB/GenericError.h"
 #include "llvm/DebugInfo/PDB/IPDBSession.h"
 #include "llvm/DebugInfo/PDB/PDB.h"
-#if HAVE_DIA_SDK
+#if LLVM_ENABLE_DIA_SDK
 #include "llvm/DebugInfo/PDB/DIA/DIASession.h"
 #endif
 #include "llvm/DebugInfo/PDB/Raw/RawSession.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ManagedStatic.h"
 
 using namespace llvm;
 using namespace llvm::pdb;
 
 Error llvm::pdb::loadDataForPDB(PDB_ReaderType Type, StringRef Path,
                                 std::unique_ptr<IPDBSession> &Session) {
   // Create the correct concrete instance type based on the value of Type.
   if (Type == PDB_ReaderType::Raw)
     return RawSession::createFromPdb(Path, Session);
 
-#if HAVE_DIA_SDK
+#if LLVM_ENABLE_DIA_SDK
   return DIASession::createFromPdb(Path, Session);
 #else
   return llvm::make_error<GenericError>("DIA is not installed on the system");
 #endif
 }
 
 Error llvm::pdb::loadDataForEXE(PDB_ReaderType Type, StringRef Path,
                                 std::unique_ptr<IPDBSession> &Session) {
   // Create the correct concrete instance type based on the value of Type.
   if (Type == PDB_ReaderType::Raw)
     return RawSession::createFromExe(Path, Session);
 
-#if HAVE_DIA_SDK
+#if LLVM_ENABLE_DIA_SDK
   return DIASession::createFromExe(Path, Session);
 #else
   return llvm::make_error<GenericError>("DIA is not installed on the system");
 #endif
 }

llvm/trunk/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")