[WIP] Windows support #276

lukehoban · 2015-10-16T23:53:14Z

Initial work on porting Delve to support Windows.

There's a lot more to do before this is ready to be merged, but most of the core infrastructure for Windows support is in place, and some basic Delve scenarios are working in simple scenarios (launching, setting breakpoints, continuing, breaking on breakpoints, inspecting registers, seeing sources, inspecting locals, exiting, etc.).

Fixes #198.

ignatov · 2015-10-27T08:01:57Z

Great!

lukehoban · 2015-11-10T03:39:24Z

A quick update on progress:

All debugger features are generally working (except attach).
Unit tests are clean on Windows.
Unit tests remain clean on Mac/Linux (modulo known flakiness).
Appveyor CI support added to guard against regressions. (https://ci.appveyor.com/project/lukehoban/delve/history)

A few of the known remaining work items:

The TestNextNetHTTP test is disabled and needs to be investigated.
Attach support is not yet enabled on Windows.
Thread information is not displayed nicely for most non-active threads because the PC on those threads is in ntdll or other dynamically loaded code. To address this, we would need to load symbols on Windows and merge that information with the DWARF symbols. This should be possible, but worth discussing before implementing.
There are a handful of TODOs to investigate and improve on workarounds in the existing implementation.
Effort was made to have minimal impact on the Mac/Linux codebase so far. In the future, it likely will make sense to define a more formal platform abstraction layer that is platform independent.

However, at this point it is worth starting to get more eyes on this code and discussing if/when this should be merged into master.

/cc @derekparker

aarzilli · 2015-11-10T15:42:52Z

proc/threads_windows.go

+	if err != nil {
+		return err
+	}
+	_, err = t.dbp.trapWait(0)


I think this is a bug, calling trapWait inside singleStep means delve will get stuck on breakpoints set on 1 byte instructions. See #262 and the commit I added today to #259.

That's true. @aarzilli is there a test for that? If not we should add one, because for now let's simply not use trapWait here, but shortly after this and some other PRs land I'd like to look into ensuring that trapWait is safe to call in that situation. Either way I'd like to prevent that as a regression in the future.

#259 has TestIssue262 which does catch this on linux and OS X, however it's highly dependent on what the compiler does.

resume loops in continueOnce moved to a OS specific resume function, this makes the problem easier to deal with and seems to be more appropriate to a windows port given what transpired from discussion of Pull Request go-delve#276

derekparker · 2015-12-03T01:56:38Z

@lukehoban sorry for the delay, have gotten really busy lately. I will be taking another look tomorrow. How is the work going to finish implementing features and get all tests to pass?

derekparker · 2015-12-13T22:15:07Z

@lukehoban do you plan on 100% parity with this PR or to get an initial implementation? Because I'm fine with merging a solid implementation that doesn't have 100% parity, as long as we have issues to track the other features that must be implemented.

aarzilli · 2015-12-14T12:53:29Z

My note about the trapWait call can be disregarded given the Continue refactoring we did on the other branch.

resume loops in continueOnce moved to a OS specific resume function, this makes the problem easier to deal with and seems to be more appropriate to a windows port given what transpired from discussion of Pull Request go-delve#276

lukehoban · 2015-12-17T23:17:54Z

@derekparker Apologies for the delay getting back to you.

I do think this is close to ready to merge in with a discrete set of known issues we can track in the issue tracker.

There is only one truly "incorrect" thing I am aware of in the current implementation, which is the issue mentioned in https://github.com/derekparker/delve/pull/276#discussion_r45100529.

Other than that, the remaining points from https://github.com/derekparker/delve/pull/276#issuecomment-155274585 are places where there is not yet 100% parity, but what does work works correctly. I'm happy to work on these as issues after merging this PR.

Certainly, merging this in with the CI turned on will help to make sure the Windows support doesn't regress going forward.

On the singleStep issue, I'll try to find some time to look at that in the next few days, and to bring this branch up to date with master. Once I have an update on that, we can decide whether to merge this in.

dprime · 2015-12-18T17:18:00Z

Hey,

I'm just trying to build this to test it out on my windows setup. Having a few problems getting things running. Should I be building on windows or cross compiling from linux to windows? When attempting to build on windows, it moans about lacking a gcc install and when on linux, it complains about ../../service/api/conversions.go:9:2: C source files not allowed when not using cgo or SWIG: threads_windows.c
Any thoughts?

lukehoban · 2015-12-18T17:39:32Z

Yeah - Delve uses cgo heavily, and cgo on Windows requires a GCC toolchain. I've been compiling on Windows with the MinGW64 tools.

The appveyor.yml has one particular toolchain configuration that works and is being used in the CI toolchain: x86_64-4.9.2-release-win32-seh-rt_v4-rev3.

Locally I've used a slightly different MinGW version: x86_64-5.2.0-posix-seh-rt_v4-rev0.

derekparker · 2016-01-20T19:58:09Z

proc/proc_windows.go

+
+// Attach to an existing process with the given PID.
+func Attach(pid int) (*Process, error) {
+	fmt.Println("Attach")


Let's remove this and just return the error.

ignatov · 2016-01-20T20:34:26Z

@lukehoban Could you ping me directly when the PR will be merged, please.

derekparker · 2016-01-20T20:35:54Z

proc/proc_windows.go

+	// in this case, one for each thread, so we should only handle the BP hit
+	// on the thread that the debugger was evented on.
+
+	err := trapthread.SetCurrentBreakpoint()


nit: rewrite as return trapthread.SetCurrentBreakpoint()

derekparker · 2016-01-20T20:44:41Z

@lukehoban https://github.com/derekparker/delve/pull/351 has landed, fixing the single step behavior.

derekparker · 2016-01-20T20:49:06Z

proc/threads_windows.go

+		return false
+	}
+	switch fn.Name {
+	case "runtime.kevent", "runtime.mach_semaphore_wait", "runtime.usleep":


We can definitely remove "runtime.mach_semaphore_wait" from this list.

derekparker · 2016-01-20T20:57:31Z

@lukehoban also, let's just go ahead and add the Appveyor badge to the README in the PR, since CI is already setup there it seems. That way it'll just be there when this lands.

Fixes go-delve#198.

lukehoban · 2016-01-21T03:08:33Z

I've addressed all the code review feedback in the latest commits.

#351 has landed, fixing the single step behavior.

Great. I've updated the windows branch to align with this (actually just removing some code).

let's just go ahead and add the Appveyor badge to the README in the PR, since CI is already setup there it seems.

I think you'll need to set up an AppVeyor project to track derekparker/delve. The one I have at https://ci.appveyor.com/project/lukehoban/delve is pointing at lukehoban/delve.

derekparker · 2016-01-24T04:05:57Z

Just merged this branch, will now start creating issues to track outstanding work that needs to be done, and I will get AppVeyor setup as well. Thank you @lukehoban for all your work on this!

lukehoban · 2016-01-24T04:50:49Z

⚡

kolkov · 2016-01-24T05:07:50Z

This is very cool!
@ignatov as you asked!

ahmetb · 2016-01-24T11:02:06Z

congrats!

techtonik · 2016-10-26T05:41:41Z

@derekparker looks like this article https://blog.gopheracademy.com/advent-2015/debugging-with-delve/ should be updated with new info about Windows support. )

resume loops in continueOnce moved to a OS specific resume function, this makes the problem easier to deal with and seems to be more appropriate to a windows port given what transpired from discussion of Pull Request go-delve#276

lukehoban mentioned this pull request Oct 16, 2015

Windows support #198

Closed

ignatov mentioned this pull request Oct 28, 2015

Support delve on Windows go-lang-plugin-org/go-lang-idea-plugin#1922

Closed

aarzilli reviewed Nov 10, 2015
View reviewed changes

lukehoban mentioned this pull request Dec 7, 2015

Debugging support on Windows microsoft/vscode-go#149

Closed

derekparker reviewed Jan 20, 2016
View reviewed changes

lukehoban force-pushed the windows branch from 0e447db to 3d2af49 Compare January 21, 2016 03:02

Add support for Windows.

bddb712

Fixes go-delve#198.

lukehoban force-pushed the windows branch from 3d2af49 to bddb712 Compare January 21, 2016 03:07

derekparker merged commit bddb712 into go-delve:master Jan 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Windows support #276

[WIP] Windows support #276

lukehoban commented Oct 16, 2015

ignatov commented Oct 27, 2015

lukehoban commented Nov 10, 2015

aarzilli Nov 10, 2015

derekparker Nov 16, 2015

aarzilli Nov 16, 2015

derekparker commented Dec 3, 2015

derekparker commented Dec 13, 2015

aarzilli commented Dec 14, 2015

lukehoban commented Dec 17, 2015

dprime commented Dec 18, 2015

lukehoban commented Dec 18, 2015

derekparker Jan 20, 2016

ignatov commented Jan 20, 2016

derekparker Jan 20, 2016

derekparker commented Jan 20, 2016

derekparker Jan 20, 2016

derekparker commented Jan 20, 2016

lukehoban commented Jan 21, 2016

derekparker commented Jan 24, 2016

lukehoban commented Jan 24, 2016

kolkov commented Jan 24, 2016

ahmetb commented Jan 24, 2016

techtonik commented Oct 26, 2016

[WIP] Windows support #276

[WIP] Windows support #276

Conversation

lukehoban commented Oct 16, 2015

ignatov commented Oct 27, 2015

lukehoban commented Nov 10, 2015

aarzilli Nov 10, 2015

Choose a reason for hiding this comment

derekparker Nov 16, 2015

Choose a reason for hiding this comment

aarzilli Nov 16, 2015

Choose a reason for hiding this comment

derekparker commented Dec 3, 2015

derekparker commented Dec 13, 2015

aarzilli commented Dec 14, 2015

lukehoban commented Dec 17, 2015

dprime commented Dec 18, 2015

lukehoban commented Dec 18, 2015

derekparker Jan 20, 2016

Choose a reason for hiding this comment

ignatov commented Jan 20, 2016

derekparker Jan 20, 2016

Choose a reason for hiding this comment

derekparker commented Jan 20, 2016

derekparker Jan 20, 2016

Choose a reason for hiding this comment

derekparker commented Jan 20, 2016

lukehoban commented Jan 21, 2016

derekparker commented Jan 24, 2016

lukehoban commented Jan 24, 2016

kolkov commented Jan 24, 2016

ahmetb commented Jan 24, 2016

techtonik commented Oct 26, 2016