Page MenuHomePhabricator

ProcessLaunchInfo: Remove Target reference
ClosedPublic

Authored by labath on Jan 2 2019, 1:43 AM.

Details

Summary

The target was being used in FinalizeFileActions to provide default
values for stdin/out/err. Also, most of the logic of this function was
very specific to how the lldb's Target class wants to launch processes,
so I, move it to Target::FinalizeFileActions, inverting the dependency.
The only piece of logic that was useful elsewhere (lldb-server) was the
part which sets up a pty and relevant file actions. I've kept this part
as ProcessLaunchInfo::SetUpPtyRedirection.

This makes ProcessLaunchInfo independent of any high-level lldb constructs.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Jan 2 2019, 1:43 AM
clayborg accepted this revision.Jan 2 2019, 12:56 PM
This revision is now accepted and ready to land.Jan 2 2019, 12:56 PM
This revision was automatically updated to reflect the committed changes.
Hui added a subscriber: Hui.Jan 24 2019, 10:37 AM
Hui added inline comments.
lldb/trunk/source/Target/ProcessLaunchInfo.cpp
220

Look like the windows implementation of OpenFirstAvailableMaster is missing.

There was no error before these changes.

$ ./lldb-server.exe g *:2000 --log-channels="lldb all:gdb-remote all" "d:\\m.exe"
1548354703.504552364 lldb-server launch
1548354703.504905224 argv[0] = 'g'
1548354703.505040407 argv[1] = '--log-channels=lldb all:gdb-remote all'
1548354703.505141735 argv[2] = '*:2000'
1548354703.505235672 argv[3] = 'd:\m.exe'
1548354703.505473614 000000DBF458F040 Broadcaster::Broadcaster("gdb-remote.server")
1548354703.505620718 000000DBF458F040 Communication::Communication (name = gdb-remote.server)
1548354703.507417917 000000DBF458F658 Broadcaster::Broadcaster("process.stdio")
1548354703.507594585 000000DBF458F658 Communication::Communication (name = process.stdio)
1548354703.510816097 processlaunchinfo.cpp:SetUpPtyRedirection Generating a pty to use for stdin/out/err
error: failed to launch 'd:\m.exe': PTY::OpenFirstAvailableMaster failed

labath marked an inline comment as done.Jan 27 2019, 11:35 PM
labath added inline comments.
lldb/trunk/source/Target/ProcessLaunchInfo.cpp
220

Yes, there was no error before, though that doesn't mean that the code was correct. The reason you're seeing the error now is because I changed GDBRemoteCommunicationServerLLGS::LaunchProcess to check the error code, whereas previously it was ignoring it.

Overall, I think you'll have to figure out stdio forwarding sooner or later, but if it's enough for you right now to replicate the !lldb_server path and open a new terminal for the debugged app, then the easiest way to achieve that would be to move the pty setup code into the NativeProcess class. So, NativeProcessLinux/NetBSD would call SetUpPtyRedirection to do the usual stdout forwarding, and the windows process would do whatever it takes to create a new terminal window (and return -1 for GetTerminalFileDescriptor).