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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
