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

[Adrian McCarthy via lldb-commits]

amccarth added a subscriber: amccarth.
amccarth added a comment.

I'm not a CMake expert, so I haven't really reviewed those bits.


================
Comment at: cmake/modules/LLDBConfig.cmake:253
@@ -250,2 +252,3 @@
+    else()
 	add_definitions( /D _UNICODE /D UNICODE )
 endif()
----------------
Can you check the indentation here?  It looks wrong in the diff.

================
Comment at: cmake/modules/LLDBConfig.cmake:399
@@ +398,3 @@
+        # It seems that lldb-server is required for Windows/MINGW, I am not sure why
+        # So we allows it to built
+        set(LLDB_CAN_USE_LLDB_SERVER 1)
----------------
typos:  `allow` `build`

================
Comment at: include/lldb/Host/windows/win32.h:96
@@ -95,3 +95,3 @@
 // MSVC 2015 and higher have timespec.  Otherwise we need to define it ourselves.
-#if defined(_MSC_VER) && _MSC_VER >= 1900
+#if (defined(_MSC_VER) && (_MSC_VER >= 1900)) || defined(__MINGW32__)
 #include <time.h>
----------------
zturner wrote:
> This might be cleaner to say `#if defined(_MSC_VER) && _MSC_VER < 1900` then define it ourselves, otherwise `#include <time.h>`.    This way you don't need to `define(__MINGW32__)`
This could be simplified by turning it around:

    #if defined(_MSC_VER) && (_MSC_VER < 1900)
    struct timespec ...
    #else
    #include <time.h>
    #endif

That would keep the comment completely consistent with the code.

================
Comment at: source/Host/windows/FileSystem.cpp:268
@@ +267,3 @@
+#else
+    file = fopen(path, mode);
+#endif
----------------
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.)

================
Comment at: source/Host/windows/ProcessRunLock.cpp:4
@@ +3,3 @@
+#ifdef __MINGW32__
+#define _WIN32_WINNT 0x0600
+#include <synchapi.h>
----------------
This is probably something we need to define in the environment so that it's applied consistently throughout.

================
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());
----------------
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.

================
Comment at: source/Plugins/Process/Windows/Common/ProcessWindowsLog.h:107
@@ -95,1 +106,3 @@
+#define WINWARN_IFALL(Flags, ...) 
+#endif
 
----------------
This will silently break logging on Windows.  Why is this necessary?

================
Comment at: source/Utility/PseudoTerminal.cpp:23
@@ -22,2 +22,3 @@
 #include "lldb/Host/windows/win32.h"
+#ifndef __MINGW32__
 typedef uint32_t pid_t;
----------------
zturner wrote:
> Change to `#if defined(_MSC_VER)`.  In practice they're equivalent but in theory saying "one single compiler" is narrower than saying "every compiler except X"
What we're really worried about here is which language runtime library we're using, because the Microsoft one doesn't define non-standard types like `pid_t`.  So this should probably be:

    #ifdef _MSC_VER



================
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[])
----------------
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.

================
Comment at: tools/driver/Driver.cpp:1314
@@ -1313,3 +1313,3 @@
 
-#ifdef _WIN32
+#if defined(_WIN32) && !defined(__MINGW32__)
         // Convert wide arguments to UTF-8
----------------
zturner wrote:
> `#if defined(_MSC_VER)`
If we get rid of `wmain` above and make this block fetch the wide command line directly, then this code could be the same on Windows, regardless of the compiler, eliminating the need to make this #if more complex.

================
Comment at: tools/lldb-server/lldb-platform.cpp:418
@@ -416,1 +417,2 @@
 }
+#endif // WIN32
----------------
Comment lost the underscore:  `_WIN32`


http://reviews.llvm.org/D18519