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

syscall: passing structs by value is problematic on Windows #44020

Open
zx2c4 opened this issue Jan 30, 2021 · 5 comments
Open

syscall: passing structs by value is problematic on Windows #44020

zx2c4 opened this issue Jan 30, 2021 · 5 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone

Comments

@zx2c4
Copy link
Contributor

zx2c4 commented Jan 30, 2021

On Windows, sometimes GUIDs are passed by value. This amounts to different things on different architectures.

  • On amd64 it's passed by reference anyway.
  • On 386 it's passed on the stack, split up word-by-word.
  • On arm and arm64 it's passed in registers, split up word-by-word, but only if there are enough registers left to do that; otherwise it's passed by reference.

So, here's a function that does not work with mkwinsyscall easily:

//sys  setInterfaceDnsSettings(guid windows.GUID, settings *dnsInterfaceSettings) (ret error) = iphlpapi.SetInterfaceDnsSettings?

Currently mkwinsyscall tries to convert guid to uintptr with a cast, which fails, and so the code doesn't compile. Instead I'm doing something pretty gross like this:

//sys   setInterfaceDnsSettingsByPtr(guid *windows.GUID, settings *dnsInterfaceSettings) (ret error) = iphlpapi.SetInterfaceDnsSettings?
//sys   setInterfaceDnsSettingsByQwords(guid1 uintptr, guid2 uintptr, settings *dnsInterfaceSettings) (ret error) = iphlpapi.SetInterfaceDnsSettings?
//sys   setInterfaceDnsSettingsByDwords(guid1 uintptr, guid2 uintptr, guid3 uintptr, guid4 uintptr, settings *dnsInterfaceSettings) (ret error) = iphlpapi.SetInterfaceDnsSettings?

func setInterfaceDnsSettings(guid windows.GUID, settings *dnsInterfaceSettings) error {
        words := (*[4]uintptr)(unsafe.Pointer(&guid))
        switch runtime.GOARCH {
        case "amd64":
                return setInterfaceDnsSettingsByPtr(&guid, settings)
        case "arm64":
                return setInterfaceDnsSettingsByQwords(words[0], words[1], settings)
        case "arm", "386":
                return setInterfaceDnsSettingsByDwords(words[0], words[1], words[2], words[3], settings)
        default:
                panic("unknown calling convention")
        }
}

That works, I guess. But it's awfully manual. For structs that are larger than a GUID, it's even more annoying. So I was thinking of trying to generalize this somehow.

Where do you suppose that code belongs? In syscall.Syscall? In mkwinsyscall? What should it look like?

CC @alexbrainman @bradfitz @ianlancetaylor @rozmansi

@ianlancetaylor
Copy link
Contributor

For functions defined in the syscall or golang.org/x/sys/windows packages we can just make the generated code do the right thing, but I don't see any such functions. Are there any?

So I guess the question is: how should we handle functions loaded using DLL.FindProc or similar that take GUID argument values. Is that right?

It does seem tough to handle that case. I don't see any way to do it. I don't even see how to generalize it, since GUID values take different numbers of arguments for different GOARCH values.

How many such functions are there?

@zx2c4
Copy link
Contributor Author

zx2c4 commented Jan 31, 2021

For functions defined in the syscall or golang.org/x/sys/windows packages we can just make the generated code do the right thing, but I don't see any such functions. Are there any?

Not at the moment in x/sys/windows. There are a few scattered around win32 APIs, though. OLE stuff, for example, passes GUIDs by value rather than by reference. I had to make this change a few months ago: lxn/win@c5100a6#diff-bc58b257cf27988dcfdc566b7e06d2b860735a55a826eccd037b89f5bb5bcda5 There might also be other functions that take structs other than GUIDs, though none I can name off the top of my head.

So yea one approach would be making mkwinsyscall split up generic structs according to the splitting rules of the ABI, and then generating helper functions per-arch and using runtime.GOARCH to choose. That would work, though could also be kind of hard. Another approach would be just doing this for matches on windows.GUID or syscall.GUID types, which split things up the same way every time. That's a little easier, but less robust. And not totally easy too -- we'd need logic in there that examines how many arguments are prior to the split argument to see if it actually should be passed by reference because there aren't enough registers (per arm abi).

I wonder if there's a more clever approach that could be done in syscall.Syscall, though, where arbitrary types can be passed rather than just uintptr. But casting through interface{} sounds kind of bad. Would this need special compiler support? Or more flexible trampolines done in asm that handle this better? Or Go2 generics?

How many such functions are there?

I'm not sure, and this most certainly is not a pressing need of any kind, at least at the moment.

But I thought I should at least open the discussion as something to keep in mind as things evolve -- winmd, generics, regabi, maybe other exciting things -- and if anybody has new ideas about it.

@zx2c4
Copy link
Contributor Author

zx2c4 commented Feb 2, 2021

An example of this apparently lives in syscall/, brought to my attention by @rsc's recent CL: https://go-review.googlesource.com/c/go/+/288815

zx2c4 added a commit to zx2c4-forks/win that referenced this issue Feb 3, 2021
Updates: golang/go#36439
Updates: golang/go#44020

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
@networkimprov
Copy link

@gopherbot add OS-Windows

@alexbrainman
Copy link
Member

Perhaps I don't understand the problem. But you should talk to @aclements about it.

I reviewed his CL recently that fixed problems passing Go structs / different data types from C to Go during callbacks. Austin needed correct C to Go data translation for future work they do for passing parameters in registers. There is even runtime.TestStdcallAndCDeclCallbacks that uses gcc to build correspondent C dlls to test this interaction. You can add your own data types to see, if they work (for whatever GOARCH). Windows callback code is part of runtime package. But I am sure Go Team can adopt similar code in the compiler.

I hope it helps.

Alex

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 8, 2021
@dmitshur dmitshur added this to the Backlog milestone Feb 8, 2021
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
cbwest3-ntnx pushed a commit to cbwest3-ntnx/win that referenced this issue Jan 13, 2023
Updates: golang/go#36439
Updates: golang/go#44020

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
cbwest3-ntnx pushed a commit to cbwest3-ntnx/win that referenced this issue Jan 13, 2023
Updates: golang/go#36439
Updates: golang/go#44020

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
cbwest3-ntnx pushed a commit to cbwest3-ntnx/win that referenced this issue Jan 17, 2023
Updates: golang/go#36439
Updates: golang/go#44020

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
cbwest3 pushed a commit to cbwest3/win that referenced this issue Feb 1, 2023
Updates: golang/go#36439
Updates: golang/go#44020

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
Status: Triage Backlog
Development

No branches or pull requests

6 participants