Page MenuHomePhabricator

Allow building LLDB on Windows with MinGW 64/4.9.2 and later
Needs ReviewPublic

Authored by eran.ifrah on Mar 28 2016, 11:31 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

As the title

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
eran.ifrah added a subscriber: lldb-commits.
zturner requested changes to this revision.Mar 28 2016, 12:42 PM
zturner edited edge metadata.

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

CMakeLists.txt
3–5

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.

cmake/modules/LLDBConfig.cmake
224–234

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.

249–255

The indentation is a little messed up here. It's not aligned, and also we use 2 space instead of 4 space.

397–404

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.

include/lldb/Host/windows/win32.h
96

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__)

source/Host/common/File.cpp
280–282

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.

source/Host/common/OptionParser.cpp
9–11

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.

source/Host/windows/EditLineWin.cpp
45

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?

source/Host/windows/ProcessRunLock.cpp
3–5

This should be in Host/windows/win32.h at the very least, but perhaps even in CMake files.

source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp
30

Three things here.

  1. _WIN32 got changed to WIN32. Was this intentional or accidental?
  2. The given conditional appears to be equivalent to #if defined(_MSC_VER) which is simpler to understand.
  3. Why disable this for MinGW builds? Other native Win32 APIs are available, is this one not?
source/Plugins/Process/Windows/Common/ProcessWindowsLog.h
85–88

Disabling logging for MinGW doesn't seem like a good idea. Why is this necessary?

source/Target/ProcessLaunchInfo.cpp
369

Maybe change this to #if !defined(_WIN32) or #if !defined(LLVM_ON_WINDOWS)

source/Utility/PseudoTerminal.cpp
23–25

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"

tools/CMakeLists.txt
7–9

Does it not compile under MinGW?

tools/driver/Driver.cpp
1302

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?

1314

#if defined(_MSC_VER)

tools/lldb-server/lldb-gdbserver.cpp
9–11

See earlier, this should be in Host/windows/win32.h, or better yet in CMake.

tools/lldb-server/lldb-platform.cpp
10

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.

tools/lldb-server/lldb-server.cpp
66–68

And changes to this file should be reverted.

This revision now requires changes to proceed.Mar 28 2016, 12:42 PM

Everywhere I said LLVM_ON_WINDOWS earlier, it's actually called LLVM_ON_WIN32.

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

cmake/modules/LLDBConfig.cmake
253

Can you check the indentation here? It looks wrong in the diff.

399

typos: allow build

include/lldb/Host/windows/win32.h
96

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.

source/Host/windows/FileSystem.cpp
268

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.)

source/Host/windows/ProcessRunLock.cpp
4

This is probably something we need to define in the environment so that it's applied consistently throughout.

source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp
30

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.

source/Plugins/Process/Windows/Common/ProcessWindowsLog.h
107

This will silently break logging on Windows. Why is this necessary?

source/Utility/PseudoTerminal.cpp
23

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
tools/driver/Driver.cpp
1302

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.

1314

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.

tools/lldb-server/lldb-platform.cpp
418

Comment lost the underscore: _WIN32

eran.ifrah edited edge metadata.
eran.ifrah marked 21 inline comments as done.

An updated patch that fixes all your comments and in includes this one as well: http://reviews.llvm.org/D18520
I also added new command line argument option to CMake : -DMINGW_DEBUG=1 when passed from the command line, it adds -g -O0 to the LLDB sources (only LLDB)
Using the standard way -DCMAKE_BUILD_TYPE=Debug creates file too large for MinGW to handle

It's night here, so I will have a detailed look tomorrow. But in the
meantime, some of the issues don't appear to be addressed. For example the
issue about why logging is disabled, the disabled code in
ProcessWinMiniDump.cpp, and a few other things. If you view the diff in
Phabricator, you can respond inline to each of those with some kind of
explanation if possible.

labath added a subscriber: labath.Mar 29 2016, 3:04 AM

Some more "why do this?" questions from me...

I was looking into using mingw a couple weeks ago, but gave up, so I'm interested to see your progress here. However, I'd like to avoid adding hacks into the cmake files if it is not completely necessary, as it increases the future maintenance burden.

CMakeLists.txt
3

Why do we need this? Why is -DCMAKE_BUILD_TYPE=Debug not sufficient?

cmake/modules/LLDBConfig.cmake
224

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?

tools/argdumper/CMakeLists.txt
5

Why is this necessary? This seems like it's trying to work around some bug present elsewhere..

tools/driver/CMakeLists.txt
20

Same question.

CMakeLists.txt
3

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

cmake/modules/LLDBConfig.cmake
224

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

source/Host/common/OptionParser.cpp
9–11

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

source/Host/windows/EditLineWin.cpp
45

I am not really sure what Editline is (need to google that...) - this macro simply makes the code compile ;)

source/Host/windows/FileSystem.cpp
268

I moved the __MINGW32__ a bit higher to exclude more parts of the code (indeed, the conversion to UTF8 can be avoided)

source/Host/windows/ProcessRunLock.cpp
3–5

_WIN32_WINNT is now defined in LLDBConfig.cmake

source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp
30

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

source/Plugins/Process/Windows/Common/ProcessWindowsLog.h
107

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

tools/CMakeLists.txt
7–9

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

tools/driver/Driver.cpp
1302

Changed to _MSC_VER condition

tools/lldb-server/lldb-gdbserver.cpp
9–11

I moved it to CMake

tools/lldb-server/lldb-platform.cpp
10

Indeed, I reverted all changes to this file

labath added inline comments.Mar 29 2016, 7:16 AM
CMakeLists.txt
3

Sorry, I came late to the discussion and I did not catch that.

This sounds like the same problem clang is having with MSVC on windows. For MSVC, it is possible to fix this by specifying the /bigobj flag to the compiler (see clang/lib/ASTMatchers/Dynamic/CMakeLists.txt). A quick search https://sourceforge.net/p/mingw-w64/bugs/341/ seems to indicate that the same solution could be applied to mingw as well. Could see if you could make that work instead?

cmake/modules/LLDBConfig.cmake
224

Ok, so it seems the problem is that this check will not work because most compilers will not treat an unknown -W flag as a fatal error. The trick seems to be to pass -Werror as well to force a non-zero exit (see llvm/cmake/modules/HandleLLVMOptions.cmake). Perhaps you could do the same here?

eran.ifrah updated this revision to Diff 51918.Mar 29 2016, 8:20 AM
eran.ifrah edited edge metadata.

Fixed: WindowsMiniDump should now also compile and work on MinGW by adding Dbghelp library
Fixed: DebuggerThread: don't pass std::atomic<DWORD> to the LOG macros as it yields g++ error on MinGW, instead use a temp variable
Fixed: Re-enable LOG macros on MinGW

A few minor points left. Most are just CMake indentation issues, so should be easy to fix. A few code changes though.

CMakeLists.txt
3–5

Can you try the /bigobj solution proposed by Pavel? which specific object file fails to link, is it an LLDB obj or a clang obj? Because as Pavel mentions clang has had this problem before, so maybe you just need to specify /bigobj for MinGW in the clang library and not in LLDB?

cmake/modules/LLDBConfig.cmake
225–235

All of this needs to be indented 2 spaces. CMake code is just like regular C++ code where everything inside of a conditional has to be indented. For CMake we use 2 spaces.

225–236

Still indentation problems here.

249–255

Still indentation problems here.

source/API/CMakeLists.txt
74–85 ↗(On Diff #51918)

How about this instead:

target_link_libraries(liblldb PRIVATE
  lldbPluginScriptInterpreterNone
  lldbPluginScriptInterpreterPython)

if(MINGW)
  target_link_libraries(liblldb PRIVATE Dbghelp)
endif()
source/Plugins/Process/Windows/Live/DebuggerThread.cpp
380–383 ↗(On Diff #51918)

Can this line be reverted? I'm not sure what the point of saving it into a temp variable is.

tools/argdumper/CMakeLists.txt
6–8

2 space indent for CMake, not 4. Also the target_link_libraries in the else()` branch should be indented. As Pavel says, I'm not sure why this is needed, but I can't really argue against it because I don't know much about MinGW

eran.ifrah updated this revision to Diff 52463.Apr 2 2016, 8:54 AM
eran.ifrah marked 3 inline comments as done.
  • I have updated the patch with various indention fixes
  • added the -mbig-obj for clang's CMakeLists.txt file (which makes the hack I did earlier in LLDB obsolete)
  • answered some of your comments

Please see updated patch

Spoke too soon, the hack into CMakeLists.txt is still needed
Passing -mbig-obj crashes ld.exe (although it crashes in a much later stage, at about 77% in the build)

CMakeLists.txt
3–5

I fixed the top level CMakeLists.txt (LLVM one)

cmake/modules/LLDBConfig.cmake
225–236

Are you calling the missing space before the opening paren an indentation problem? otherwise, I think I am missing something here
its 2 spaces indented

source/API/CMakeLists.txt
74–85 ↗(On Diff #51918)

This was my first try, however, this yields a warning in cmake 3.0 and later: target_link_libraries can only be used once per target

source/Plugins/Process/Windows/Live/DebuggerThread.cpp
380–383 ↗(On Diff #51918)

No, this change was done on purpose. Removing the fix I added, will trigger this error from the compiler:

use of deleted function
tools/argdumper/CMakeLists.txt
6–8

This is needed because otherwise, you will get multiple definitions for many symbols - this is the side effect when linking with a .dll.a in MinGW systems when you don't really need to do this, since you can link directly to .dll file

tools/driver/CMakeLists.txt
20

Same answer

labath added a comment.Apr 4 2016, 2:24 AM

Spoke too soon, the hack into CMakeLists.txt is still needed
Passing -mbig-obj crashes ld.exe (although it crashes in a much later stage, at about 77% in the build)

Clang only puts /bigobj on a couple of files, which are known to produce extremely large object files. Would that help you get past the linker error by any chance?

source/API/CMakeLists.txt
74–85 ↗(On Diff #52463)

I think you should add this library to system_libs in LLDBConfig.cmake. Then it should get linked in automatically.

source/Plugins/Process/Windows/Live/DebuggerThread.cpp
380–383 ↗(On Diff #52463)

zturner: The point here is that m_pid_to_detach is an std::atomic, and you shouldn't be passing those through ... (I am not sure how it even worked in the first place).

An alternative would be putting m_pid_to_detach.load() here...

tools/argdumper/CMakeLists.txt
11–13

It's a hack, but I think I know why you need it... there are issues with multiply defined symbols in lldb on windows, so I guess mingw is just more sensitive to them...

eran.ifrah updated this revision to Diff 52715.Apr 5 2016, 10:56 AM
eran.ifrah marked an inline comment as done.

Moved dbghelp to system_lib

CMakeLists.txt
3–5

Adding -mbig-obj for some files might get me through this, but my workaround also does the job so I am going to stick with it
Re-building clang takes too long and I can't afford to waste more time on this

source/API/CMakeLists.txt
74–85 ↗(On Diff #52463)

Thanks, I was not aware of this variable. I moved Dbghelp to system_libs under Windows+MinGW

zturner resigned from this revision.Jun 2 2016, 8:37 PM
zturner removed a reviewer: zturner.
zturner added inline comments.
cmake/modules/LLDBConfig.cmake
225–235

All of this needs to be indented 2 spaces. CMake code is just like regular C++ code where everything inside of a conditional has to be indented. For CMake we use 2 spaces.