[Lldb-commits] [PATCH] D18519: Allow building LLDB on Windows with MinGW 64/4.9.2 and later

[Zachary Turner via lldb-commits]

zturner added a comment.

A few minor points left.  Most are just CMake indentation issues, so should be easy to fix.  A few code changes though.


================
Comment at: CMakeLists.txt:3-5
@@ -2,1 +2,5 @@
 
+if(MINGW_DEBUG)
+    # force debugging info into lldb sources
+    message("-- Building LLDB in Debug mode")
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -g -O0")
----------------
Can you try the /bigobj solution proposed by Pavel?  which specific object file fails to link, is it an LLDB obj or a clang obj?  Because as Pavel mentions clang has had this problem before, so maybe you just need to specify /bigobj for MinGW in the clang library and not in LLDB?

================
Comment at: cmake/modules/LLDBConfig.cmake:225-235
@@ -224,3 +224,4 @@
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
 check_cxx_compiler_flag("-Wno-deprecated-register"
                         CXX_SUPPORTS_NO_DEPRECATED_REGISTER)
 if (CXX_SUPPORTS_NO_DEPRECATED_REGISTER)

@@ -232,3 +233,4 @@
 if (CXX_SUPPORTS_NO_VLA_EXTENSION)
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-vla-extension")
 endif ()
+endif()
----------------
All of this needs to be indented 2 spaces.  CMake code is just like regular C++ code where everything inside of a conditional has to be indented.  For CMake we use 2 spaces.

================
Comment at: cmake/modules/LLDBConfig.cmake:225-236
@@ -224,3 +224,4 @@
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
 check_cxx_compiler_flag("-Wno-deprecated-register"
                         CXX_SUPPORTS_NO_DEPRECATED_REGISTER)
 if (CXX_SUPPORTS_NO_DEPRECATED_REGISTER)

@@ -232,4 +233,5 @@
 if (CXX_SUPPORTS_NO_VLA_EXTENSION)
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-vla-extension")
 endif ()
+endif()
 
----------------
Still indentation problems here.

================
Comment at: cmake/modules/LLDBConfig.cmake:249-255
@@ -247,5 +249,8 @@
 
 # Use the Unicode (UTF-16) APIs by default on Win32
 if (CMAKE_SYSTEM_NAME MATCHES "Windows")
+  if(MINGW)
+    add_definitions( -D_UNICODE -DUNICODE -D_WIN32_WINNT=0x0600 -D_BSD_SOURCE)
+  else()
 	add_definitions( /D _UNICODE /D UNICODE )
 endif()
----------------
Still indentation problems here.

================
Comment at: source/API/CMakeLists.txt:74-85
@@ -73,6 +73,14 @@
 # want every single library taking a dependency on the script interpreters.
+if(MINGW)
 target_link_libraries(liblldb PRIVATE
   lldbPluginScriptInterpreterNone
   lldbPluginScriptInterpreterPython
+    Dbghelp # Needed for MiniDumpWriteDump
   )
+else()
+  target_link_libraries(liblldb PRIVATE
+    lldbPluginScriptInterpreterNone
+    lldbPluginScriptInterpreterPython
+    )
+endif()
 
----------------
How about this instead:

    target_link_libraries(liblldb PRIVATE
      lldbPluginScriptInterpreterNone
      lldbPluginScriptInterpreterPython)

    if(MINGW)
      target_link_libraries(liblldb PRIVATE Dbghelp)
    endif()


================
Comment at: source/Plugins/Process/Windows/Live/DebuggerThread.cpp:380-383
@@ -379,5 +379,6 @@
         {
+            DWORD dwPidToDetach = m_pid_to_detach;
             WINLOG_IFANY(WINDOWS_LOG_EVENT | WINDOWS_LOG_EXCEPTION | WINDOWS_LOG_PROCESS,
                             "Breakpoint exception is cue to detach from process 0x%x",
-                            m_pid_to_detach);
+                            dwPidToDetach);
             ::DebugActiveProcessStop(m_pid_to_detach);
----------------
Can this line be reverted?  I'm not sure what the point of saving it into a temp variable is.

================
Comment at: tools/argdumper/CMakeLists.txt:6-8
@@ +5,5 @@
+if(MINGW)
+    # link directly against the dll file
+    add_dependencies(lldb-argdumper liblldb)
+    target_link_libraries(lldb-argdumper -L"${CMAKE_BINARY_DIR}/bin" -llldb)
+else()
----------------
2 space indent for CMake, not 4.  Also the `target_link_libraries in the `else()` branch should be indented.  As Pavel says, I'm not sure why this is needed, but I can't really argue against it because I don't know much about MinGW


http://reviews.llvm.org/D18519