Page MenuHomePhabricator

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

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

Details

Summary

Make lldb-server work on Windows. Implement some remote capability support in PlatformWindows, launch gdbserver, and launch/attach processes using Windows APIs.

Some changes in this commit include:

  • #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.
  • Better error handling in SocketAddress::GetAddressInfo.
  • Clear the string before calling llvm::convertWideToUTF8 to avoid an unexpected assertion.
  • Make some of the tests work and disable others that don't.

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
78

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

106–109

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
267

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 ↗(On Diff #179968)

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 ↗(On Diff #179968)

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
77

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
77–78

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.

106–109

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
262–266

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

tools/lldb-server/SystemInitializerLLGS.cpp
37–52 ↗(On Diff #179968)

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 ↗(On Diff #179968)

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 ↗(On Diff #179968)

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
77–78

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

106–109

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.

asmith updated this revision to Diff 193232.Apr 1 2019, 9:26 PM
asmith updated this revision to Diff 193236.Apr 1 2019, 11:56 PM
asmith edited the summary of this revision. (Show Details)
asmith retitled this revision from [lldb-server] Add initial support for building lldb-server on Windows to [lldb-server] Add initial support for lldb-server on Windows.Apr 2 2019, 12:01 AM
labath added a comment.Apr 2 2019, 1:01 AM

The patch is pretty big, so I haven't done an in-depth review yet. The fact that you've ran clang-format over entire files (and not just the diffs) does not help.

For a start, I've highlighted the parts that I believe can/should go into a separate patch. Some of them are obvious and can be committed immediately. Others should go through a separate review. We can get back to the core of the patch once these have been dealt with (FWIW, I haven't seen anything obviously wrong from a quick glance).

include/lldb/Core/Debugger.h
338 ↗(On Diff #193232)

This looks like experimental code accidentally left in (?)

include/lldb/Target/Platform.h
599 ↗(On Diff #193232)

To keep the scope of this patch (which is already pretty big) as small as possible, could you submit this as a separate patch? No review needed.

include/lldb/lldb-types.h
42–45 ↗(On Diff #193232)

Please revert or submit as a separate patch.

lldb.xcodeproj/project.pbxproj
10522 ↗(On Diff #193232)

I am surpised that you needed to change the xcode project in any way, as this code should not even build there. Please explain.

packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteRegisterState.py
28 ↗(On Diff #193232)

The fact that you were able to do this told me that this part of the test is actually irrelevant. So, I just went ahead and deleted this part altogether (r357451).

packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py
208–211

That sounds fun. Is that how things are supposed to work, or is that something you're planning to change/fix? (mostly just curious).

packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py
677–678

delete ?

1496

The -1 looks like it needs to be windows-only.

packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
434

I don't think this can go in like this. However, I agree that this double-retry loop is pretty ugly. When I get some time, I'll try to see if I can rewrite this part to use reverse connects. That way we won't get any races at all. For the time being, I'd just revert this.

packages/Python/lldbsuite/test/tools/lldb-server/main.cpp
1

Could you (as a separate patch) convert this file to use c++ threads? That should allow us to cut down on the ifdefs by using standard std::thread and std::mutex.

source/Host/common/Socket.cpp
117–135

Please avoid spurious reformats of entire files. This makes it patches hard to review, particularly when they are as large as this one.

For the changes in this specific file, I think you can just commit them as a separate patch. Or revert the file completely, as the only thing you seem to be doing is adding logging.

source/Host/common/SocketAddress.cpp
267

This comment still stands.

source/Host/windows/HostInfoWindows.cpp
98 ↗(On Diff #193232)

Move this into a separate patch, add a unit test checking that string is cleared when this function is called.

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
294 ↗(On Diff #193232)

Move UUID handling into a separate patch.

source/Plugins/Process/Utility/RegisterContextWindows_x64.cpp
103 ↗(On Diff #193232)

<iostream> is banned in llvm. (and it looks like you're not using it anyway).

source/Plugins/Process/Utility/RegisterContextWindows_x64.h
33 ↗(On Diff #193232)

d_ ?

source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp
65

delete?

99

delete?

source/Plugins/Process/Windows/Common/NativeProcessWindows.h
17

It looks like IDebugDelegate.h is missing from the patch.

139

It looks like m_process will never be null. Convert it to a reference, and delete all the null-checks?

source/Plugins/Process/Windows/Common/ProcessDebugger.h
17

File missing from patch.

source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
565–580

spurious reformats.

tools/lldb-server/SystemInitializerLLGS.cpp
37–52 ↗(On Diff #179968)

I still believe this should go to Socket::Initialize (as a separate patch).

tools/lldb-server/lldb-server.cpp
38–74 ↗(On Diff #193232)

The whole file looks good. You can commit this separately straight away. (Maybe add a comment somewhere that the namespace is there to avoid conflicts on windows. Otherwise someone in the future might get the idea to replace the file-local namespace with an anonymous one.)

asmith marked 5 inline comments as done.Apr 2 2019, 10:13 AM

Committed a few NFC changes

asmith marked 6 inline comments as done.Apr 8 2019, 11:38 PM
asmith added inline comments.
tools/lldb-server/SystemInitializerLLGS.cpp
37–52 ↗(On Diff #179968)
asmith updated this revision to Diff 194380.Apr 9 2019, 12:34 PM
asmith marked 4 inline comments as done.Apr 9 2019, 12:47 PM
asmith marked 4 inline comments as done.Apr 9 2019, 12:52 PM
asmith added inline comments.
source/Plugins/Process/Windows/Common/NativeProcessWindows.h
17

IDebugDelegate.h is located in source/Plugins/Process/Windows/Common

source/Plugins/Process/Windows/Common/ProcessDebugger.h
17

That's found in source/Plugins/Process/Windows/Common

asmith marked 4 inline comments as done.Apr 9 2019, 12:55 PM
asmith added inline comments.
source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
294 ↗(On Diff #193232)

The UUID change is being discussed here https://reviews.llvm.org/D56229

asmith marked an inline comment as done.Apr 9 2019, 12:56 PM
asmith marked 2 inline comments as done.Apr 9 2019, 10:40 PM
asmith added inline comments.
packages/Python/lldbsuite/test/tools/lldb-server/main.cpp
1
labath marked an inline comment as done.Apr 10 2019, 12:09 AM
labath added inline comments.
source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
294 ↗(On Diff #193232)

Ah, I completely forgot about that patch. Thanks for reminding me. This is actually very good timing because I was just diving into the topic of UUID matching in lldb on windows (in order to match up minidumps with respective exe/pdb files). I'll elaborate more there...

asmith updated this revision to Diff 194454.Apr 10 2019, 12:39 AM
asmith marked an inline comment as done.
asmith edited the summary of this revision. (Show Details)