Page MenuHomePhabricator

[lldb-server] Add initial support for building lldb-server on Windows
Needs ReviewPublic

Authored by asmith on Jan 2 2019, 4:34 PM.

Details

Summary

This is intended as a first step to make lldb-server work on Windows. Follow-up changes to implement remote capabilities in PlatformWindows, launch gdbserver, launch/attach processes using Windows APIs etc will come in separate revisions.

The changes in this commit include the following:

  • #ifdef what's not supported on Windows, for example signals
  • Add a dummy 'waitpid' to the Windows PosixApi along with some definitions that are needed for compilation.
  • Setup WSAsocket connection in SystemInitializerLLGS::Initialize.
  • Add a namespace to static function 'terminate()' in lldb-server.cpp because its ambiguous with a Windows API.
  • Better error handling in SocketAddress::GetAddressInfo.
  • Clear the string before calling llvm::convertWideToUTF8 to avoid an unexpected assertion.

Diff Detail

Event Timeline

asmith created this revision.Jan 2 2019, 4:34 PM
labath added a subscriber: labath.
labath added inline comments.
include/lldb/Host/windows/PosixApi.h
79

I don't see WUNTRACED being used anywhere within lldb. Why did you need this?

108–111

What are your plans for this?

I assume you needed this for the fork-server in lldb-platform.cpp. I think the best way to address that would be to change the code from spawning a separate process for each connection to having a new thread for each connection. That should be much more portable and will avoid the need to mock posix apis on windows. I can help you with that if you run into any problems there.

source/Host/common/SocketAddress.cpp
268

Writing to stdout like this is very rude. If there isn't a suitable way to return this info, then I suggest writing this to the log instead.

tools/lldb-server/SystemInitializerLLGS.cpp
37–52

I think a better option here would be to move this code to something like Socket::Initialize. Then we could call this from SystemInitializerCommon::Initialize, and it would be available to everyone. This would allow us to remove the copy of this code in PlatformWindows, and replace the WSAStartup calls in various unittests with Socket::Initialize()

tools/lldb-server/lldb-server.cpp
40

In other places we use llgs (for LLdb Gdb Server), so I suggest sticking to that.

excited to see this starting as well!

krytarowski added inline comments.
include/lldb/Host/windows/PosixApi.h
78

I think that these symbols should not be leaked here in the first place.

zturner added inline comments.Jan 7 2019, 10:04 AM
include/lldb/Host/windows/PosixApi.h
78–79

In general we should avoid mocking posix APIs for Windows. If something is currently implemented in terms of a Posix API that doesn't exist on Windows, we should provide an implementation of that function with an entirely new name and platform-agnostic set of parameters whose interface need not resemble the posix interface in any way..

For example, I see this is used in lldb-platform.cpp when we call waitpid(). Instead, we could add something like void WaitForAllChildProcessesToExit(); and then just implement that function on posix and Windows completely separately. This is more flexible and portable than trying to fit Windows semantics into a posix api, which doesn't always work.

108–111

Are you ok with the behavioral change? What if one connection crashes?

Assuming we wanted to implement this behavior on Windows, the way to do it would be to create all child processes in a single "job" (aka process group), as that is the best way to get notifications for when all childs have terminated.

source/Host/common/SocketAddress.cpp
263–267

Perhaps we could define something like lldb::eErrorTypeHostNative to avoid this ifdef.

tools/lldb-server/SystemInitializerLLGS.cpp
37–52

Also, why do we need the g_ref_count? Can't we just print an error message and call exit if the version is not what we expect?

58–61

I don't think Initialize and Terminate need to be re-entrant. So we can probably delete the ref count code here

tools/lldb-server/lldb-server.cpp
40

The static from before indicates that the function should not have external linkage, so I think we should keep that. Whether we put it in a namespace or not is orthogonal, but for global functions in cpp functions, we generally prefer to mark them all static.

labath added inline comments.Jan 8 2019, 2:09 AM
include/lldb/Host/windows/PosixApi.h
78–79

For the record, the intention of that code in lldb-platform is reapAllChildProcessesThatHaveAlreadyExited()

108–111

Yes, I would be fine with that (in fact I wanted to do that at some point anyway). The reason the fork server was added was to enable parallel running of remote tests (before that we only ever handled one concurrent connection). Also the platform mode of llgs doesn't do anything fancy. It's mostly just about copying files around and launching gdb-remote processes (so the real debugging still happens in a separate process), so the crashing surface is pretty limited.

BTW, the only reason we call waitpid there is that on unix, you *have* to do that or you will die of zombie infestation. So, if on windows you can create fire-and-forget child processes, then this whole code is unnecessary there.