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

Jump to bottom

cmd/cgo: treat Windows HANDLE as non-pointer type #42018

Closed
eliasnaur opened this issue Oct 16, 2020 · 12 comments
Closed

cmd/cgo: treat Windows HANDLE as non-pointer type #42018

eliasnaur opened this issue Oct 16, 2020 · 12 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone
Backlog

Comments

@eliasnaur
Copy link
Contributor

eliasnaur commented Oct 16, 2020 •
edited

The Windows HANDLE type is translated to unsafe.Pointer by cgo. Unfortunately, HANDLE can contain valid non-pointer values, and should be treated accordingly. Incomplete pointer types such as the JNI types are covered by a rule so that they're not treated as pointers. However, that rule does not match HANDLE, which is typedef'ed to void *.

From golang-dev:

package main

import (
    "fmt"
    "os"
    "path/filepath"
    "runtime"

    "golang.org/x/sys/windows"
)

/*
#include <windows.h>
*/
import "C"

func main() {
    name := filepath.Join(os.TempDir(), "handletest.tmp")
    name16, err := windows.UTF16PtrFromString(name)
    if err != nil {
        panic(err)
    }
    fmt.Println("Using filepath", name)

    handle := C.CreateFileW((*C.wchar_t)(name16), C.GENERIC_READ, C.FILE_SHARE_READ|C.FILE_SHARE_WRITE, nil, C.CREATE_ALWAYS, 0, nil)
    if handle == C.HANDLE(C.INVALID_HANDLE_VALUE) {
        panic("failed to open")
    }

    fmt.Println("handle addr", handle)

    runtime.GC()

    rtn := C.CloseHandle(handle)
    if rtn == C.FALSE {
        panic("failed to close handle")
    }
}

go build && handletest.exe
Using filepath C:\Users\Alex\AppData\Local\Temp\handletest.tmp
handle addr 0x154
runtime: bad pointer in frame main.main at 0xc000075f10: 0x154
fatal error: invalid pointer found on stack

Duplicate of #8921.

CC @randall77
@toothrot toothrot added this to the Backlog milestone Oct 16, 2020
@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 16, 2020
@ianlancetaylor ianlancetaylor changed the title runtime/cgo: treat Windows HANDLE as non-pointer type cmd/cgo: treat Windows HANDLE as non-pointer type Oct 17, 2020
@zx2c4
Copy link
Contributor

zx2c4 commented Nov 28, 2020

Cc @alexbrainman

@eliasnaur
Copy link
Contributor Author

Gentle ping. Expanding use of Cgo in Gio on Windows is blocked by this issue that is subtle and has no workaround.

@zx2c4
Copy link
Contributor

zx2c4 commented Aug 28, 2021

Im curious: why not use the faster x/sys/windows syscall trampolines instead of cgo?

@alexbrainman
Copy link
Member

I agree this is a duplicate of #8921. So you have to take it up with @rsc, because he made working as intended decision in #8921.

I also agree with @zx2c4 that you don't need to use Cgo to call Windows APIs.

Alex

@zx2c4
Copy link
Contributor

zx2c4 commented Aug 28, 2021

I agree this is a duplicate of #8921. So you have to take it up with @rsc, because he made working as intended decision in #8921.

We could treat HANDLE as uintptr as a special case, though, I guess?

@alexbrainman
Copy link
Member

We could treat HANDLE as uintptr as a special case, though, I guess?

We could. But Russ specifically says at #8921 (comment)

There is no plausible way to start enumerating the 'non-pointer' pointer types.

I read this as he does not want any exceptions in Cgo.

Alex

@eliasnaur
Copy link
Contributor Author

Im curious: why not use the faster x/sys/windows syscall trampolines instead of cgo?

There's no support for window system functions.

We could treat HANDLE as uintptr as a special case, though, I guess?

We could. But Russ specifically says at #8921 (comment)

There is no plausible way to start enumerating the 'non-pointer' pointer types.

I read this as he does not want any exceptions in Cgo.

His comment is from 2014, 3 years before adding special cases for CoreFoundation and Android JNI types. We don't even have to change the mapped type from unsafe.Pointer to uintptr anymore.

@alexbrainman
Copy link
Member

There's no support for window system functions.

I suspect it will be easy enough to add whatever functions you need. I would spend extra effort to avoid Cgo. I am even happy to try and implement them myself, if you want help. Just tell us which functions you need.

His comment is from 2014, 3 years before adding special cases for CoreFoundation and Android JNI types.

Yes. It has been long time. But still you have to take it up with Russ or Keith.

We don't even have to change the mapped type from unsafe.Pointer to uintptr anymore.

I am not familiar with compiler code. If you know what to change, just send CL to Russ or Keith or any compiler developer.

Alex

@eliasnaur
Copy link
Contributor Author

There's no support for window system functions.

I suspect it will be easy enough to add whatever functions you need. I would spend extra effort to avoid Cgo. I am even happy to try and implement them myself, if you want help. Just tell us which functions you need.

Thank you. I already have x/sys/windows wrappers for what I need for now. My primary motivation for pushing this issue is #38917. However, without a reliable HANDLE (and HWND) you can't make reliable Cgo Windows GUI programs.

We don't even have to change the mapped type from unsafe.Pointer to uintptr anymore.

I am not familiar with compiler code. If you know what to change, just send CL to Russ or Keith or any compiler developer.

I don't know what to change. My comment was to point out that while the older special cases (CoreFoundation, JNI) were breaking changing (Cgo generated type changed from unsafe.Pointer to uintptr), new special cases preserve the type but treat their values as non-pointers.

@alexbrainman
Copy link
Member

I already have x/sys/windows wrappers for what I need for now.

OK.

My primary motivation for pushing this issue is #38917. However, without a reliable HANDLE (and HWND) you can't make reliable Cgo Windows GUI programs.

Thanks for explaining.

I don't know what to change.

I definitely cannot help you with that.

Alex

@ianlancetaylor
Copy link
Contributor

As far as I know the approach here would be to change cmd/cgo/gcc.go so that when it sees the type HANDLE with GOOS == "windows" it treats it as a pointer to an empty struct type for which NotInHeap is true.

@eliasnaur eliasnaur mentioned this issue Sep 15, 2021
Closed
@gopherbot
Copy link

Change https://golang.org/cl/350070 mentions this issue: cmd/cgo: add go:notinheap annotation to Windows handle types

@golang golang locked and limited conversation to collaborators Sep 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@toothrot @zx2c4 @eliasnaur @ianlancetaylor @gopherbot @alexbrainman