[lldb][CMake] Fix windows-x86-debug compilation with python enabled using multi-target generator

[leonid.mashinskiy]

Summary

Visual Studio CMake generator is multi-target and does not define CMAKE_BUILD_TYPE, so Debug build on VS was failing due selection of release python library.
This patch reverts back some of latest changes and fixes building by raw VS using CMake expression generators.

[labath]

+windows folks, then can speak to the substance of this change.

As for the implementation, I get the impression that there is a lot of duplicated code in here. The code for finding the release and debug pythons is nearly identical. Can this be extracted into an utility function?

[amccarth]

I'll look at this in detail soon, but a few caveats:

• lldb tests don't work with the debug Python interpreter, which is why I've been complaining about test failures approximately forever when the bots seem fine.

• Some of these changes you're undoing caused a lot of pain when they were first put in, so let's be really sure if we need to undo this.

• I use the VS project only for code browsing and debugging. For building and tests, I use the ninja project. So I'm not very familiar with the problem you're trying to fix.

[stella.stamenova]

Is there a way to reduce some of the code duplication? The more duplication there is, the harder it is to maintain.

cmake/modules/LLDBConfig.cmake

240 ↗

(On Diff #218050)

Nit: multi-configuration

[amccarth]

cmake/modules/LLDBConfig.cmake

164 ↗

(On Diff #218050)

This first changed line seems useful.

The rest seems of dubious benefit, even if the duplication can be reduced. Are folks really using the VS project to build both Debug and Release? I'd be surprised, since the debug version won't pass half of the tests.

[leonid.mashinskiy]

Since this patch appeared to be arguable from your point of view I will write some comments before making any further changes.

In D66994#1652868, @amccarth wrote:

• I use the VS project only for code browsing and debugging. For building and tests, I use the ninja project. So I'm not very familiar with the problem you're trying to fix.

Since there are lot of tablegenned code just opening project in VS without building it makes lot's of code unresolved for producivity tools providing smart code navigation, highlighting and etc (like Intellisence or Resharper) which helps me understand code faster.
Also I find it useful to be able to compile single cpp file from VS.
And also building from VS was working on 9 branch so I just thought it was broken unintentionally and can be fixed.

In D66994#1652868, @amccarth wrote:

• Some of these changes you're undoing caused a lot of pain when they were first put in, so let's be really sure if we need to undo this.

I am not just undoing latest changes, that's why there are so many copy-pasted code. If CMAKE_BUILD_TYPE is set (which is set when building using ninja or other single-target generator) then logic should flow the same way as without my changes. I'm just supporting third way when CMAKE_BUILD_TYPE is not selected and configuration is deduced on built time and paths to selected libraries are selected using CMake expression generators as it was in 9 branch.

In D66994#1652868, @amccarth wrote:

• lldb tests don't work with the debug Python interpreter, which is why I've been complaining about test failures approximately forever when the bots seem fine.

I found it easier to debug some tests in full debug environment, though I didn't try to run all test suite in debug.
Also we found some tests to be flaky even in release configurations but bot had never showed that.

If you are ok with my explanations I will try to extract some code to reduce copy-pasting, otherwise this patch can be abandoned.

Also I have a question about x86 windows configuration - is it officially supported or it will be abandoned soon? Because now it is completely broken.

[amccarth]

I'm open to this if we can reduce the code duplication.

One of my concerns is that changes in July and August completely broke the build for me for many days. I had to remove all but one version of Python to ensure that it always found the right one.

I, too, would appreciate being able to run tests under the debugger, but I find that's impossible with python_d because it detects and halts virtually every test (like using the Python allocator without holding the GIL and leaked resource warnings).

[leonid.mashinskiy]

Extracted python lookup logic into function

[labath]

Thanks. The new version seems fine to me. I'll leave it for the windows folks to have the final say on this..

[amccarth]

Thanks for factoring out the duplication.

Fingers crossed.

[leonid.mashinskiy]

Thanks for the review!

Can somebody commit this please because I don't have commit access?

[amccarth]

Yes, I can commit it for you soon.

lldb/trunk/cmake/modules/LLDBConfig.cmake

 #    a binary release distribution of python and a custom built distribution.
 # 2) From MSVC 2015 and onwards, it is only possible to use Python 3.5 or later.
 # 3) FindPythonLibs queries the registry to locate Python, and when looking for a
 #    64-bit version of Python, since cmake.exe is a 32-bit executable, it will see
 #    a 32-bit view of the registry.  As such, it is impossible for FindPythonLibs to
 #    locate 64-bit Python libraries.
 # This function is designed to address those limitations.  Currently it only partially
 # addresses them, but it can be improved and extended on an as-needed basis.
+function(find_python_libs_windows_helper LOOKUP_DEBUG OUT_EXE_PATH_VARNAME OUT_LIB_PATH_VARNAME OUT_DLL_PATH_VARNAME OUT_VERSION_VARNAME)
+  if(LOOKUP_DEBUG)
+      set(POSTFIX "_d")
+  else()
+      set(POSTFIX "")
+  endif()
+
+  file(TO_CMAKE_PATH "${PYTHON_HOME}/python${POSTFIX}.exe"                       PYTHON_EXE)
+  file(TO_CMAKE_PATH "${PYTHON_HOME}/libs/${PYTHONLIBS_BASE_NAME}${POSTFIX}.lib" PYTHON_LIB)
+  file(TO_CMAKE_PATH "${PYTHON_HOME}/${PYTHONLIBS_BASE_NAME}${POSTFIX}.dll"      PYTHON_DLL)
+
+  foreach(component PYTHON_EXE;PYTHON_LIB;PYTHON_DLL)
+    if(NOT EXISTS ${${component}})
+      message(WARNING "Unable to find ${component}")
+      unset(${component})
+    endif()
+  endforeach()
+
+  if (NOT PYTHON_EXE OR NOT PYTHON_LIB OR NOT PYTHON_DLL)
+    message(WARNING "Unable to find all Python components.  Python support will be disabled for this build.")
+    set(LLDB_DISABLE_PYTHON 1 PARENT_SCOPE)
+    return()
+  endif()
+
+  # Find the version of the Python interpreter.
+  execute_process(COMMAND "${PYTHON_EXE}" -c
+                  "import sys; sys.stdout.write('.'.join([str(x) for x in sys.version_info[:3]]))"
+                  OUTPUT_VARIABLE PYTHON_VERSION_OUTPUT
+                  RESULT_VARIABLE PYTHON_VERSION_RESULT
+                  ERROR_QUIET)
+
+  if(PYTHON_VERSION_RESULT)
+    message(WARNING "Unable to retrieve Python executable version")
+    set(PYTHON_VERSION_OUTPUT "")
+  endif()
+
+  set(${OUT_EXE_PATH_VARNAME} ${PYTHON_EXE} PARENT_SCOPE)
+  set(${OUT_LIB_PATH_VARNAME} ${PYTHON_LIB} PARENT_SCOPE)
+  set(${OUT_DLL_PATH_VARNAME} ${PYTHON_DLL} PARENT_SCOPE)
+  set(${OUT_VERSION_VARNAME}  ${PYTHON_VERSION_OUTPUT} PARENT_SCOPE)
+endfunction()
+
 function(find_python_libs_windows)
   if ("${PYTHON_HOME}" STREQUAL "")
     message(WARNING "LLDB embedded Python on Windows requires specifying a value for PYTHON_HOME.  Python support disabled.")
     set(LLDB_DISABLE_PYTHON 1 PARENT_SCOPE)
     return()
   endif()
 
   file(TO_CMAKE_PATH "${PYTHON_HOME}/Include" PYTHON_INCLUDE_DIR)
else()
     message(WARNING "Python support will be disabled for this build.")
     set(LLDB_DISABLE_PYTHON 1 PARENT_SCOPE)
     return()
   endif()
 
   file(TO_CMAKE_PATH "${PYTHON_HOME}" PYTHON_HOME)
   # TODO(compnerd) when CMake Policy `CMP0091` is set to NEW, we should use
   # if(CMAKE_MSVC_RUNTIME_LIBRARY MATCHES MultiThreadedDebug)
-  if(CMAKE_BUILD_TYPE STREQUAL Debug)
-    file(TO_CMAKE_PATH "${PYTHON_HOME}/python_d.exe" PYTHON_EXE)
-    file(TO_CMAKE_PATH "${PYTHON_HOME}/libs/${PYTHONLIBS_BASE_NAME}_d.lib" PYTHON_LIB)
-    file(TO_CMAKE_PATH "${PYTHON_HOME}/${PYTHONLIBS_BASE_NAME}_d.dll" PYTHON_DLL)
-  else()
-    file(TO_CMAKE_PATH "${PYTHON_HOME}/python.exe" PYTHON_EXE)
-    file(TO_CMAKE_PATH "${PYTHON_HOME}/libs/${PYTHONLIBS_BASE_NAME}.lib" PYTHON_LIB)
-    file(TO_CMAKE_PATH "${PYTHON_HOME}/${PYTHONLIBS_BASE_NAME}.dll" PYTHON_DLL)
+  if(NOT DEFINED CMAKE_BUILD_TYPE)
+    # Multi-target generator was selected (like Visual Studio or Xcode) where no concrete build type was passed
+    # Lookup for both debug and release python installations
+    find_python_libs_windows_helper(TRUE  PYTHON_DEBUG_EXE   PYTHON_DEBUG_LIB   PYTHON_DEBUG_DLL   PYTHON_DEBUG_VERSION_STRING)
+    find_python_libs_windows_helper(FALSE PYTHON_RELEASE_EXE PYTHON_RELEASE_LIB PYTHON_RELEASE_DLL PYTHON_RELEASE_VERSION_STRING)
+    if(LLDB_DISABLE_PYTHON)
+      set(LLDB_DISABLE_PYTHON 1 PARENT_SCOPE)
+      return()
     endif()
 
-  foreach(component PYTHON_EXE;PYTHON_LIB;PYTHON_DLL)
-    if(NOT EXISTS ${${component}})
-      message(WARNING "unable to find ${component}")
-      unset(${component})
-    endif()
-  endforeach()
+    # We should have been found both debug and release python here
+    # Now check that their versions are equal
+    if(NOT PYTHON_DEBUG_VERSION_STRING STREQUAL PYTHON_RELEASE_VERSION_STRING)
+      message(FATAL_ERROR "Python versions for debug (${PYTHON_DEBUG_VERSION_STRING}) and release (${PYTHON_RELEASE_VERSION_STRING}) are different."
+                          "Python installation is corrupted")
+    endif ()
 
-  if (NOT PYTHON_EXE OR NOT PYTHON_LIB OR NOT PYTHON_DLL)
-    message(WARNING "Unable to find all Python components.  Python support will be disabled for this build.")
+    set(PYTHON_EXECUTABLE $<$<CONFIG:Debug>:${PYTHON_DEBUG_EXE}>$<$<NOT:$<CONFIG:Debug>>:${PYTHON_RELEASE_EXE}>)
+    set(PYTHON_LIBRARY    $<$<CONFIG:Debug>:${PYTHON_DEBUG_LIB}>$<$<NOT:$<CONFIG:Debug>>:${PYTHON_RELEASE_LIB}>)
+    set(PYTHON_DLL        $<$<CONFIG:Debug>:${PYTHON_DEBUG_DLL}>$<$<NOT:$<CONFIG:Debug>>:${PYTHON_RELEASE_DLL}>)
+    set(PYTHON_VERSION_STRING ${PYTHON_RELEASE_VERSION_STRING})
+  else()
+    # Lookup for concrete python installation depending on build type
+    if (CMAKE_BUILD_TYPE STREQUAL Debug)
+      set(LOOKUP_DEBUG_PYTHON TRUE)
+    else()
+      set(LOOKUP_DEBUG_PYTHON FALSE)
+    endif()
+    find_python_libs_windows_helper(${LOOKUP_DEBUG_PYTHON} PYTHON_EXECUTABLE PYTHON_LIBRARY PYTHON_DLL PYTHON_VERSION_STRING)
+    if(LLDB_DISABLE_PYTHON)
       set(LLDB_DISABLE_PYTHON 1 PARENT_SCOPE)
       return()
     endif()
+  endif()
 
-  # Find the version of the Python interpreter.
-  execute_process(COMMAND "${PYTHON_EXE}" -c
-                  "import sys; sys.stdout.write(';'.join([str(x) for x in sys.version_info[:3]]))"
-                  OUTPUT_VARIABLE PYTHON_VERSION_OUTPUT
-                  RESULT_VARIABLE PYTHON_VERSION_RESULT
-                  ERROR_QUIET)
-  if(NOT PYTHON_VERSION_RESULT)
-    string(REPLACE ";" "." PYTHON_VERSION_STRING "${PYTHON_VERSION_OUTPUT}")
-    list(GET PYTHON_VERSION_OUTPUT 0 PYTHON_VERSION_MAJOR)
-    list(GET PYTHON_VERSION_OUTPUT 1 PYTHON_VERSION_MINOR)
-    list(GET PYTHON_VERSION_OUTPUT 2 PYTHON_VERSION_PATCH)
+  if(PYTHON_VERSION_STRING)
+    string(REPLACE "." ";" PYTHON_VERSION_PARTS "${PYTHON_VERSION_STRING}")
+    list(GET PYTHON_VERSION_PARTS 0 PYTHON_VERSION_MAJOR)
+    list(GET PYTHON_VERSION_PARTS 1 PYTHON_VERSION_MINOR)
+    list(GET PYTHON_VERSION_PARTS 2 PYTHON_VERSION_PATCH)
+  else()
+    unset(PYTHON_VERSION_MAJOR)
+    unset(PYTHON_VERSION_MINOR)
+    unset(PYTHON_VERSION_PATCH)
   endif()
 
   # Set the same variables as FindPythonInterp and FindPythonLibs.
-  set(PYTHON_EXECUTABLE ${PYTHON_EXE} PARENT_SCOPE)
-  set(PYTHON_LIBRARY ${PYTHON_LIB} PARENT_SCOPE)
-  set(PYTHON_DLL ${PYTHON_DLL} PARENT_SCOPE)
-  set(PYTHON_INCLUDE_DIR ${PYTHON_INCLUDE_DIR} PARENT_SCOPE)
-  set(PYTHONLIBS_VERSION_STRING "${PYTHONLIBS_VERSION_STRING}" PARENT_SCOPE)
-  set(PYTHON_VERSION_STRING ${PYTHON_VERSION_STRING} PARENT_SCOPE)
-  set(PYTHON_VERSION_MAJOR ${PYTHON_VERSION_MAJOR} PARENT_SCOPE)
-  set(PYTHON_VERSION_MINOR ${PYTHON_VERSION_MINOR} PARENT_SCOPE)
-  set(PYTHON_VERSION_PATCH ${PYTHON_VERSION_PATCH} PARENT_SCOPE)
+  set(PYTHON_EXECUTABLE         "${PYTHON_EXECUTABLE}"          PARENT_SCOPE)
+  set(PYTHON_LIBRARY            "${PYTHON_LIBRARY}"             PARENT_SCOPE)
+  set(PYTHON_DLL                "${PYTHON_DLL}"                 PARENT_SCOPE)
+  set(PYTHON_INCLUDE_DIR        "${PYTHON_INCLUDE_DIR}"         PARENT_SCOPE)
+  set(PYTHONLIBS_VERSION_STRING "${PYTHONLIBS_VERSION_STRING}"  PARENT_SCOPE)
+  set(PYTHON_VERSION_STRING     "${PYTHON_VERSION_STRING}"      PARENT_SCOPE)
+  set(PYTHON_VERSION_MAJOR      "${PYTHON_VERSION_MAJOR}"       PARENT_SCOPE)
+  set(PYTHON_VERSION_MINOR      "${PYTHON_VERSION_MINOR}"       PARENT_SCOPE)
+  set(PYTHON_VERSION_PATCH      "${PYTHON_VERSION_PATCH}"       PARENT_SCOPE)
 
   message(STATUS "LLDB Found PythonExecutable: ${PYTHON_EXECUTABLE} (${PYTHON_VERSION_STRING})")
-  message(STATUS "LLDB Found PythonLibs: ${PYTHON_LIB} (${PYTHONLIBS_VERSION_STRING})")
+  message(STATUS "LLDB Found PythonLibs: ${PYTHON_LIBRARY} (${PYTHONLIBS_VERSION_STRING})")
   message(STATUS "LLDB Found PythonDLL: ${PYTHON_DLL}")
   message(STATUS "LLDB Found PythonIncludeDirs: ${PYTHON_INCLUDE_DIR}")
 endfunction(find_python_libs_windows)
 
 if (NOT LLDB_DISABLE_PYTHON)
   if ("${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")
     find_python_libs_windows()