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

Tue Mar 29 03:26:06 PDT 2016

eran.ifrah added a comment.




================
Comment at: CMakeLists.txt:3
@@ -2,1 +2,3 @@
 
+if(MINGW_DEBUG)
+    # force debugging info into lldb sources
----------------
labath wrote:
> Why do we need this? Why is `-DCMAKE_BUILD_TYPE=Debug` not sufficient?
No. As I mentioned in my previous comment the object file generated are too big for as.exe to handle
and it aborts. This results from Clang files (not LLDB). As a workaround and I added this command line directive to be able to produce debug builds of LLDB so I can actually debug them and provide some more insights if needed

================
Comment at: cmake/modules/LLDBConfig.cmake:224
@@ -223,2 +223,3 @@
 # Disable Clang warnings
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
 check_cxx_compiler_flag("-Wno-deprecated-register"
----------------
labath wrote:
> Why is this necessary? The whole purpose `check_cxx_compiler_flag` is to check whether a compiler supports some flag. Hardcoding the check ourselves makes that kinda pointless. What error were you getting here?
Tons of warnings about unrecognized command line directive (or something similar) - can't remember the exact warning message - but its something like this. If really need to know the warning message, I can re-compile without this change

================
Comment at: source/Host/common/OptionParser.cpp:9-11
@@ -8,2 +8,5 @@
 //===----------------------------------------------------------------------===//
+#ifdef __MINGW32__
+#define _BSD_SOURCE
+#endif
 
----------------
zturner wrote:
> Not sure what `_BSD_SOURCE` is, can you explain what this does?  In any case, it should go in `Host/windows/win32.h` along with other similar preprocessor defines.
This is needed to workaround bug with MinGW + getopt, without this macro defined (before any file in the source) optreset is undeclared.
I moved it to Win32.h and added a big comment that explains why this is needed

================
Comment at: source/Host/windows/EditLineWin.cpp:45
@@ -44,3 +44,3 @@
 
-#if !defined( _WIP_INPUT_METHOD )
+#if !defined( _WIP_INPUT_METHOD ) && !defined(__MINGW32__)
 
----------------
zturner wrote:
> This is weird but I honestly don't even know what `_WIP_INPUT_METHOD` is.  My guess is it's not even relevant anymore.
> 
> Anyway, does MinGW already have an implementation of Editline?
I am not really sure what Editline is (need to google that...) - this macro simply makes the code compile ;)

================
Comment at: source/Host/windows/FileSystem.cpp:268
@@ +267,3 @@
+#else
+    file = fopen(path, mode);
+#endif
----------------
amccarth wrote:
> This code just went through the trouble of converting the path from UTF-8 to a wide string.  If you can't use `_wfopen_s`, it seems you should skip more of this function.  (I'm also surprised you didn't have to make a similar change in more places.)
I moved the `__MINGW32__` a bit higher to exclude more parts of the code (indeed, the conversion to UTF8 can be avoided)

================
Comment at: source/Host/windows/ProcessRunLock.cpp:3-5
@@ -2,13 +2,5 @@
 #include "lldb/Host/windows/windows.h"
-
-namespace
-{
-#if defined(__MINGW32__)
-// Taken from WinNT.h
-typedef struct _RTL_SRWLOCK {
-    PVOID Ptr;
-} RTL_SRWLOCK, *PRTL_SRWLOCK;
-
-// Taken from WinBase.h
-typedef RTL_SRWLOCK SRWLOCK, *PSRWLOCK;
+#ifdef __MINGW32__
+#define _WIN32_WINNT 0x0600
+#include <synchapi.h>
 #endif
----------------
zturner wrote:
> This should be in `Host/windows/win32.h` at the very least, but perhaps even in CMake files.
`_WIN32_WINNT` is now defined in `LLDBConfig.cmake` 

================
Comment at: source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp:30
@@ -29,3 +29,3 @@
     if (!process_sp) return false;
-#ifdef _WIN32
+#if defined(WIN32) && !defined(__MINGW32__)
     HANDLE process_handle = ::OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, FALSE, process_sp->GetID());
----------------
amccarth wrote:
> zturner wrote:
> > Three things here.
> > 
> >   # `_WIN32` got changed to `WIN32`.  Was this intentional or accidental?
> >   # The given conditional appears to be equivalent to `#if defined(_MSC_VER)` which is simpler to understand.
> >   # Why disable this for MinGW builds?  Other native Win32 APIs are available, is this one not?
> What the goal of skipping this here?  As far as I can see, this is just calling Win32 APIs, so I'd expect those to be available regardless of which compiler you're using.
The function `MiniDumpWriteDump` simply does not exist in my MinGW build (using TDM-GCC-64/4.9.2)
I fixed the condition to `#if defined(_MSC_VER)` which is indeed simpler to understand

================
Comment at: source/Plugins/Process/Windows/Common/ProcessWindowsLog.h:107
@@ -95,1 +106,3 @@
+#define WINWARN_IFALL(Flags, ...) 
+#endif
 
----------------
amccarth wrote:
> This will silently break logging on Windows.  Why is this necessary?
I don't like this either, but it causes alot of build errors with `std::atomic` it was simpler to avoid them and get the build done. This can be addressed later, but I suspect it has a lower priority

================
Comment at: tools/CMakeLists.txt:7-9
@@ -6,5 +6,5 @@
   add_subdirectory(driver)
-if (NOT __ANDROID_NDK__)
+if (NOT __ANDROID_NDK__ AND NOT MINGW)
   add_subdirectory(lldb-mi)
 endif()
 if (LLDB_CAN_USE_LLDB_SERVER)
----------------
zturner wrote:
> Does it not compile under MinGW?
Unfortunately, no. Many errors...
Again, as my goal was to get `liblldb.dll` compiled (I don't even need `lldb.exe`) I simply skipped this.
I can work this later on getting this tool to compile.

I added a TODO comment in the `CMakeLists.txt` file to ensure it won't be forgotten

================
Comment at: tools/driver/Driver.cpp:1302
@@ -1301,3 +1301,3 @@
 int
-#ifdef WIN32
+#if defined(WIN32) && !defined(__MINGW32__)
 wmain(int argc, wchar_t const *wargv[])
----------------
amccarth wrote:
> zturner wrote:
> > Should change this to `#if defined(_MSC_VER)`.  Also if you don't need the `wchar_t` version of `main()`, then is the CMake change that adds `-D_UNICODE -DUNICODE` also necessary?  Or maybe that can be removed?
> We probably should get rid of wmain and just have main everywhere.  Then, in the WinAPI-specific code below, we could use `GetCommandLineW` to get the command line and convert them into UTF-8.
Changed to `_MSC_VER` condition

================
Comment at: tools/lldb-server/lldb-gdbserver.cpp:9-11
@@ -8,2 +8,5 @@
 //===----------------------------------------------------------------------===//
+#ifdef __MINGW32__
+#define _BSD_SOURCE
+#endif
 
----------------
zturner wrote:
> See earlier, this should be in `Host/windows/win32.h`, or better yet in CMake.
I moved it to CMake

================
Comment at: tools/lldb-server/lldb-platform.cpp:10
@@ -9,2 +9,3 @@
 
+#ifndef _WIN32
 // C Includes
----------------
zturner wrote:
> Based on your other patch, you figured out how to get your MinGW build to not require lldb-server.  So this change should be removed.
Indeed, I reverted all changes to this file


http://reviews.llvm.org/D18519



