Page MenuHomePhabricator

Fix TestProcesslaunch regression caused by D7372
ClosedPublic

Authored by labath on Feb 5 2015, 8:26 AM.

Details

Summary

After closing all the leaked file descriptors to the inferior tty, the following problem occured:

  • when stdin, stdout and stderr are redirected, there are no slave descriptors open (which is good)
  • lldb has a reader thread, which attempts to read from the master end of the tty
  • this thread receives an EOF
  • in response, it closes it's master end
  • as this is the last open file descriptor for the master end, this deletes the tty and sends SIGHUP to the inferior (this is bad)

I fix this problem by making sure the master end remains open for the duration of the inferior
process by storing a copy of the file descriptor in ProcessMonitor. I create a copy to avoid
ownership issues with the reading thread.

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 19409.Feb 5 2015, 8:26 AM
labath retitled this revision from to Fix TestProcesslaunch regression caused by D7372.
labath updated this object.
labath edited the test plan for this revision. (Show Details)
labath added a reviewer: ovyalov.
labath added subscribers: emaste, Unknown Object (MLST).
emaste added a comment.Feb 5 2015, 8:44 AM

Can you apply the ProcessMonitor.{cpp,h} changes to FreeBSD as well?

labath updated this revision to Diff 19415.Feb 5 2015, 9:17 AM

Applying the changes to FreeBSD, as requested. Note that I currently have no means of testing
this. @emaste, could you verify?

labath removed a subscriber: emaste.
clayborg accepted this revision.Feb 5 2015, 10:00 AM
clayborg added a reviewer: clayborg.
clayborg added a subscriber: clayborg.

I am hitting accept as I can't verify any of this as we don't use Process monitor on MacOSX. I am good with this as long as all others are.

This revision is now accepted and ready to land.Feb 5 2015, 10:00 AM
emaste accepted this revision.Feb 5 2015, 11:01 AM
emaste edited edge metadata.

Applying the changes to FreeBSD, as requested. Note that I currently have no means of testing
this. @emaste, could you verify?

Thanks! I applied it locally and it works for me.

ovyalov accepted this revision.Feb 5 2015, 11:24 AM
ovyalov edited edge metadata.

lgtm

This revision was automatically updated to reflect the committed changes.