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