[Lldb-commits] [PATCH] D18519: Allow building LLDB on Windows with MinGW 64/4.9.2 and later
Zachary Turner via lldb-commits
lldb-commits at lists.llvm.org
Mon Mar 28 12:42:47 PDT 2016
zturner requested changes to this revision.
zturner added a comment.
This revision now requires changes to proceed.
Mostly small comments, but a lot of them. Mostly around style issues such as indentation, putting platform specific `#defines` in the right header file, and some nuances around `_WIN32` vs `__MINGW32__` vs `LLVM_ON_WINDOWS` vs `_MSC_VER`
================
Comment at: CMakeLists.txt:3-5
@@ -2,1 +2,5 @@
+if(MINGW)
+ set(LLDB_DISABLE_PYTHON 1)
+endif()
+
----------------
Is it not possible to get MinGW working with python? By setting this here, you're making it impossible to run the test suite with MinGW. Which means we will have no idea what works and doesn't work.
================
Comment at: cmake/modules/LLDBConfig.cmake:224-234
@@ -223,13 +223,13 @@
# Disable Clang warnings
check_cxx_compiler_flag("-Wno-deprecated-register"
CXX_SUPPORTS_NO_DEPRECATED_REGISTER)
-if (CXX_SUPPORTS_NO_DEPRECATED_REGISTER)
+if (CXX_SUPPORTS_NO_DEPRECATED_REGISTER AND NOT MINGW)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-deprecated-register")
endif ()
check_cxx_compiler_flag("-Wno-vla-extension"
CXX_SUPPORTS_NO_VLA_EXTENSION)
-if (CXX_SUPPORTS_NO_VLA_EXTENSION)
+if (CXX_SUPPORTS_NO_VLA_EXTENSION AND NOT MINGW)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-vla-extension")
endif ()
----------------
These are supposed to be clang warnings, according to the comment at line 223. If you're building with MinGW then you aren't using clang. So maybe it's better to put this whole block of code inside of an if statement that only runs if compiling with clang.
================
Comment at: cmake/modules/LLDBConfig.cmake:249-255
@@ -248,5 +248,9 @@
# Use the Unicode (UTF-16) APIs by default on Win32
if (CMAKE_SYSTEM_NAME MATCHES "Windows")
+ if(MINGW)
+ add_definitions( -D_UNICODE -DUNICODE )
+ else()
add_definitions( /D _UNICODE /D UNICODE )
endif()
+endif()
----------------
The indentation is a little messed up here. It's not aligned, and also we use 2 space instead of 4 space.
================
Comment at: cmake/modules/LLDBConfig.cmake:397-404
@@ -392,4 +396,10 @@
else()
+ if(MINGW)
+ # 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)
+ else()
set(LLDB_CAN_USE_LLDB_SERVER 0)
endif()
+endif()
----------------
Indentation is wrong again here. Also you will need to decide whether you want to port lldb-server to windows or to use the local code path. If you decide to use the local code path, then you'll want to remove this change but figure out why it's using lldb-server to begin with.
================
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>
----------------
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__)`
================
Comment at: source/Host/common/File.cpp:280-282
@@ +279,5 @@
+
+#ifndef _SH_DENYNO
+#define _SH_DENYNO 0x40
+#endif
+
----------------
This should go in `Host/windows/win32.h`, and it would be better to put it inside of an `#if defined(__MINGW32__)` rather than an `#ifndef _SH_DENYNO`. Because otherwise someone reading the code can understand what it does, but not why? If you write `#if defined(__MINGW32__)` then people know what this is for.
================
Comment at: source/Host/common/OptionParser.cpp:9-11
@@ -8,2 +8,5 @@
//===----------------------------------------------------------------------===//
+#ifdef __MINGW32__
+#define _BSD_SOURCE
+#endif
----------------
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.
================
Comment at: source/Host/windows/EditLineWin.cpp:45
@@ -44,3 +44,3 @@
-#if !defined( _WIP_INPUT_METHOD )
+#if !defined( _WIP_INPUT_METHOD ) && !defined(__MINGW32__)
----------------
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?
================
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
----------------
This should be in `Host/windows/win32.h` at the very least, but perhaps even in CMake files.
================
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());
----------------
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?
================
Comment at: source/Plugins/Process/Windows/Common/ProcessWindowsLog.h:85-88
@@ -84,4 +84,5 @@
+#ifndef __MINGW32__
#define WINLOG_IFANY(Flags, ...) WINLOGF_IF(Flags, LogMaskReq::Any, Printf, __VA_ARGS__)
#define WINLOG_IFALL(Flags, ...) WINLOGF_IF(Flags, LogMaskReq::All, Printf, __VA_ARGS__)
#define WINLOGV_IFANY(Flags, ...) WINLOGF_IF(Flags, LogMaskReq::Any, Verbose, __VA_ARGS__)
----------------
Disabling logging for MinGW doesn't seem like a good idea. Why is this necessary?
================
Comment at: source/Target/ProcessLaunchInfo.cpp:369
@@ -368,3 +368,3 @@
int open_flags = O_RDWR | O_NOCTTY;
-#if !defined(_MSC_VER)
+#if !defined(_MSC_VER) && !defined(__MINGW32__)
// We really shouldn't be specifying platform specific flags
----------------
Maybe change this to `#if !defined(_WIN32)` or `#if !defined(LLVM_ON_WINDOWS)`
================
Comment at: source/Utility/PseudoTerminal.cpp:23-25
@@ -22,3 +22,5 @@
#include "lldb/Host/windows/win32.h"
+#ifndef __MINGW32__
typedef uint32_t pid_t;
+#endif
// empty functions
----------------
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"
================
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)
----------------
Does it not compile under MinGW?
================
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[])
----------------
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?
================
Comment at: tools/driver/Driver.cpp:1314
@@ -1313,3 +1313,3 @@
-#ifdef _WIN32
+#if defined(_WIN32) && !defined(__MINGW32__)
// Convert wide arguments to UTF-8
----------------
`#if defined(_MSC_VER)`
================
Comment at: tools/lldb-server/lldb-gdbserver.cpp:9-11
@@ -8,2 +8,5 @@
//===----------------------------------------------------------------------===//
+#ifdef __MINGW32__
+#define _BSD_SOURCE
+#endif
----------------
See earlier, this should be in `Host/windows/win32.h`, or better yet in CMake.
================
Comment at: tools/lldb-server/lldb-platform.cpp:10
@@ -9,2 +9,3 @@
+#ifndef _WIN32
// C Includes
----------------
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.
================
Comment at: tools/lldb-server/lldb-server.cpp:66-68
@@ -65,2 +65,5 @@
case 'g':
+#ifdef __MINGW32__
+ case 'p':
+#endif
initialize();
----------------
And changes to this file should be reverted.
http://reviews.llvm.org/D18519
More information about the lldb-commits
mailing list