Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix termination on MinGW/Cygwin by calling Windows API to terminate. #636

Merged
merged 4 commits into from
Jul 13, 2017

Conversation

pieandcakes
Copy link
Collaborator

MinGW and Cygwin do not allow Async break to occur without user
intervention. The problem is with -exec-abort (stop debugging) while in
run mode requires us to do an async break so that we can abort. Since we
can't do that without additional user intervention, we will terminate
the child process instead.

MinGW and Cygwin do not allow Async break to occur without user
intervention. The problem is with -exec-abort (stop debugging) while in
run mode requires us to do an async break so that we can abort. Since we
can't do that without additional user intervention, we will terminate
the child process instead.
// to terminate debugging.
if ((this.IsCygwin || this.IsMinGW) && _debuggeePids.Count > 0)
{
int debuggeePid = _debuggeePids.First().Value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_debuggeePids [](start = 42, length = 13)

Is there a reason you only pick the first?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gregg-miskelly I took the code from CmdBreakInternal() (line 697) that breaks on the first thread. Would it be better to iterate through the list and terminate each individual item?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what I would have thought, but @paulmaybee would be more familiar with the child process code. Maybe there is a reason that isn't needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't hurt to terminate all pids. Normally having more than one is a transient state. However there is a non-trivial amount of work done by gdb in between reporting that a new process is available and detaching so that vs can reattach separate miengine instance. If something goes wrong/hangs then killing all children is most likely the right way to recover.

finally
{
if (handle != IntPtr.Zero)
Marshal.FreeCoTaskMem(handle);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FreeCoTaskMem [](start = 40, length = 13)

You need to P/Invoke to CloseHandle

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

// the normal path of sending an internal async break so we can exit doesn't work.
// Therefore, we will call TerminateProcess on the debuggee with the exit code of 0
// to terminate debugging.
if ((this.IsCygwin || this.IsMinGW) && _debuggeePids.Count > 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

) [](start = 82, length = 1)

Exclude gdbserver cases?

Copy link
Member

@gregg-miskelly gregg-miskelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

public static extern IntPtr OpenProcess(int dwDesiredAccess, [MarshalAs(UnmanagedType.Bool)] bool bInheritHandle, int dwProcessId);
[DllImport("kernel32.dll", SetLastError = true)]
[return: MarshalAs(UnmanagedType.Bool)]
static extern bool TerminateProcess(IntPtr hProcess, uint uExitCode);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: private?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Contributor

@paulmaybee paulmaybee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend terminating all pids. Otherwise LGTM.

@pieandcakes pieandcakes merged commit 90ef84f into master Jul 13, 2017
@pieandcakes pieandcakes deleted the users/piel/terminate branch November 22, 2017 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants