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

proc: refactorings to implement follow-exec mode on Windows #3441

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

aarzilli
Copy link
Member

@aarzilli aarzilli commented Jul 14, 2023

Miscellaneous non-functional changes to prepare for adding support for
follow-exec mode on Windows:

  • removed (*nativeProcess).wait function from Windows backend (unused).

  • move close of ptraceDoneChan from release to handlePtraceFuncs, this
    makes postExit callable by a function executed by execPtraceFunc.

  • change addTarget to detach before creating the target object if we
    don't actually want to attach to the child process, also moved the
    detach call to (*processGroup).add instead of having one in addTarget
    and one in the code that calls (*processGroup).add.

  • changed Detach to be a method of TargetGroup/ProcessGroup, the
    Windows backend will need access to the process group to call
    WaitForDebugEvent.

  • moved resume method to processGroup. First all threads stopped at a
    breakpoint need to be stepped, then all other threads can be resumed. This is true also for linux even though it didn't cause the current
    tests to fail.

pkg/proc/native/proc_linux.go Outdated Show resolved Hide resolved
// This means the debugger will no longer receive events from the process
// we were previously debugging.
// If kill is true then the process will be killed when we detach.
func (t *Target) detach(kill bool) error {
Copy link
Member

Choose a reason for hiding this comment

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

Seems to me we could simply keep this, call it in the process group detach, and then we wouldn't need detachWithoutGroup.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I understand. The process group doesn't have access to the target object and (*processGroup).Detach is in a different package and can't call private methods of proc.Target.

pkg/proc/gdbserial/gdbserver.go Outdated Show resolved Hide resolved
Miscellaneous non-functional changes to prepare for adding support for
follow-exec mode on Windows:

- removed (*nativeProcess).wait function from Windows backend (unused).
- move close of ptraceDoneChan from release to handlePtraceFuncs, this
  makes postExit callable by a function executed by execPtraceFunc.
- change addTarget to detach before creating the target object if we
  don't actually want to attach to the child process, also moved the
  detach call to (*processGroup).add instead of having one in addTarget
  and one in the code that calls (*processGroup).add.
- changed Detach to be a method of TargetGroup/ProcessGroup, the
  Windows backend will need access to the process group to call
  WaitForDebugEvent.
- moved resume method to processGroup. First all threads stopped at a
  breakpoint need to be stepped, then all other threads can be resumed.
  This is true also for linux even though it didn't cause the current
  tests to fail.
// Detach from the process being debugged, optionally killing it.
func (dbp *nativeProcess) Detach(kill bool) (err error) {
func (procgrp *processGroup) Detach(pid int, kill bool) (err error) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand. The process group doesn't have access to the target object and (*processGroup).Detach is in a different package and can't call private methods of proc.Target.

Quoting the reply from the other review comment. I should have put the comment against this change instead. Why not leave the (nativeProcess) Detach) method and add the (processGroup) one which simply calls nativeProcess.Detach, so the code paths which now use detachWithoutGroup can remain unchanged without having to jump through this hoop to detach.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have to move the kill method to processGroup because on Windows it needs it to call waitForDebugEvent:

func (procgrp *processGroup) kill(dbp *nativeProcess) error {
	if dbp.exited {
		return nil
	}

	p, err := os.FindProcess(dbp.pid)
	if err != nil {
		return err
	}
	defer p.Release()

	// TODO: Should not have to ignore failures here,
	// but some tests appear to Kill twice causing
	// this to fail on second attempt.
	_ = syscall.TerminateProcess(dbp.os.hProcess, 1)

	dbp.execPtraceFunc(func() {
		procgrp.waitForDebugEvent(waitBlocking | waitDontHandleExceptions)
	})

	p.Wait()

	dbp.postExit()
	return nil
}

Copy link
Member

@derekparker derekparker left a comment

Choose a reason for hiding this comment

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

lgtm

@derekparker derekparker merged commit 7f094c8 into go-delve:master Aug 28, 2023
1 check passed
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.

2 participants