Page MenuHomePhabricator

Enable lldb-server on Windows

Authored by asmith on May 8 2019, 8:41 AM.



This commit contains three small changes to enable lldb-server on Windows.

  • Add lldb-server for Windows to the build
  • Disable pty redirection on Windows for the initial lldb-server bring up
  • Add a support to get the parent pid for a process on Windows
  • Ifdef some signals which aren't supported on Windows

Thanks to Hui Huang for the help with this patch!

Diff Detail

Event Timeline

asmith created this revision.May 8 2019, 8:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2019, 8:41 AM
asmith updated this revision to Diff 198675.May 8 2019, 8:49 AM
asmith retitled this revision from Disable pty redirection on Windows and add a method to get the parent pid to Enable lldb-server on Windows.
asmith edited the summary of this revision. (Show Details)
amccarth added inline comments.May 8 2019, 11:33 AM

What's the scope of "temporarily"? Is there some specific feature or change that will cause this workaround to be removed?

Hui added a subscriber: Hui.May 8 2019, 12:28 PM
Hui added inline comments.

Currently the absence of pty support is treated as a fatal error that will block the overall usage of lldb-server.exe for windows. In my opinion, the redirection of std -i/o/e are mainly intended to generate I* and O* packets to notify the lldb about the llgs's inferior (shown on lldb console). Without such support, some of the lldb-server functionalities, like launch/attach process and most of the remote packets etc. still can be tested or say be experimented (see the python tests under tools/lldb-server).

Temporarily relax the mentioned codes and wait for the pty support on windows. No concrete idea how to add that support now.

amccarth marked an inline comment as done.May 8 2019, 12:46 PM
amccarth added inline comments.

Got it. I misunderstood the original comment and thought the "temporarily" applied to the code change itself.

compnerd added inline comments.
108 ↗(On Diff #198675)

This should be out-of-lined. Furthermore, is there any place where the use requires process group handling? Otherwise, can't we just replace this with:

pid_t waitpid(pid_t pid, int *status, int options) {
  assert(pid > 0 && "only support waiting for a particular child");
  assert(options ^ ~WNOHANG == 0 && "only WNOHANG is supported');
  HANDLE hProcess;
  DWORD dwResult;
  hProcess = OpenProcess(SYNCHRONIZE, FALSE, pid);
  if (hProcess == NULL) return pid_t(-1); // TODO(compnerd) better error handling
  dwResult = WaitForSingleObject(hProcess, options & WNOHANG ? 0 : INFINITE);
  if (dwResult == WAIT_OBJECT_0 || dwResult == WAIT_TIMEOUT) return pid;
  return pid_t(-1);

PTY support is available as of 2019H1.

labath added inline comments.May 9 2019, 4:02 AM
106–109 ↗(On Diff #198675)

As discussed in the review where this was forked from, we shouldn't be mocking posix apis. Instead we should provide platform-independent abstractions that each platform can implement in it's own way. Waitpid is right now used in only one place. Implementing it here (even just as a stub) just encourages others to add more uses of it.

Given that (I think) you don't care about the "server" mode of lldb-platform at the moment, and the fact that the piece of code calling this is already broken because the use of fork, I think that for now we should just #ifdef-out the block of code calling fork and waitpid (it's already structured in a way that's easy to #ifdef. This would enable you to even provide a friendly error message explaining that the requested feature is not available.

1188–1198 ↗(On Diff #198675)

ProcessInstanceInfo already has the ability to say whether some fields are present/valid or not. So I think you should just replace this #ifdef with something like:

if (proc_info.UserIDIsValid())
  response.Printf(";real-uid:%x", proc_info.GetUserID()));

It looks like this function will be unused after you disable the signal line below, so you might as well ifdef-out the entire function instead of just its body.

compnerd added inline comments.May 9 2019, 10:09 AM
106–109 ↗(On Diff #198675)

I definitely am in favour of removal of the mocked POSIX API, to the point where I'm willing to entertain reduced functionality for that. If we can completely preprocess away the usage instead of adding this, I am absolutely okay with that.

rnk removed a reviewer: rnk.May 13 2019, 1:38 PM
asmith updated this revision to Diff 199555.May 14 2019, 9:51 PM
asmith edited the summary of this revision. (Show Details)
JDevlieghere added inline comments.

Should we just remove LLDB_CAN_USE_LLDB_SERVER instead?

labath accepted this revision.May 15 2019, 2:07 AM
labath added inline comments.

Sounds like a good idea.

This revision is now accepted and ready to land.May 15 2019, 2:07 AM
asmith updated this revision to Diff 214979.Aug 13 2019, 4:46 PM
asmith edited the summary of this revision. (Show Details)
asmith closed this revision.Aug 13 2019, 4:49 PM