proposal: Remove 'C' types from the exposed APIs

[hajimehoshi]

Now gl packages has APIs that expose C types:

func BufferStorageExternalEXT(target uint32, offset int, size int, clientBuffer C.GLeglClientBufferEXT, flags uint32) {}
func CreateSyncFromCLeventARB(context *C.struct__cl_context, event *C.struct__cl_event, flags uint32) uintptr {}
func DebugMessageCallbackAMD(callback C.GLDEBUGPROCAMD, userParam unsafe.Pointer) {}
func EGLImageTargetRenderbufferStorageOES(target uint32, image C.GLeglImageOES) {}
func EGLImageTargetTexStorageEXT(target uint32, image C.GLeglImageOES, attrib_list *int32) {}
func EGLImageTargetTexture2DOES(target uint32, image C.GLeglImageOES) {}
func EGLImageTargetTextureStorageEXT(texture uint32, image C.GLeglImageOES, attrib_list *int32) {}
func GetVkProcAddrNV(name *uint8) C.GLVULKANPROCNV {}
func NamedBufferStorageExternalEXT(buffer uint32, offset int, size int, clientBuffer C.GLeglClientBufferEXT, flags uint32) {}

The problem is that this violates the guideline of Cgo: https://golang.org/cmd/cgo/ says that "a Go package should not expose C types in its exported API". Furthermore, #109 (remove Cgo dependencies on Windows) is now impossible due to these APIs.

I propose to replace such C types with unsafe.Pointer:

• C.GLeglClientBufferEXT is equivalent to void*: https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_external_buffer.txt
• *C.struct__cl_context is a pointer
• *C.struct__cl_event is a pointer
• C.GLDEBUGPROCAMD is a function pointer: https://www.khronos.org/registry/OpenGL/extensions/AMD/AMD_debug_output.txt
• C.GLeglImageOES is equiavalent to void*: https://www.khronos.org/registry/OpenGL/extensions/OES/OES_EGL_image_external.txt
• C.GLVULKANPROCNV is a function pointer: (could not find a source but some implementation says it is a pointer).

This proposal breaks the backward compability since this changes the exposed APIs, but I think the risk is low. GitHub code search says there is no actual usage of these functions:

• BufferStorageExternalEXT
• CreateSyncFromCLeventARB
• DebugMessageCallbackAMD
• EGLImageTargetRenderbufferStorageOES
• EGLImageTargetTexStorageEXT
• EGLImageTargetTexture2DOES
• EGLImageTargetTextureStorageEXT
• GetVkProcAddrNV
• NamedBufferStorageExternalEXT

It's possible to do further investigation with sourcegraph.com, but this is harder since this needs to specified GL versions.

What do you think? Thanks!

CC @dmitshur

[dmitshur]

In general, removing C types from the gl public API SGTM. I suspect it was an oversight rather than intentional decision that they ended up there (please correct me if wrong).

replace such C types with unsafe.Pointer

A counter-proposal to consider is to replace such C types with uintptr.

I think we should evaluate both and make an informed decision of which one to pick.

Whether a package should expose C void * as unsafe.Pointer or uintptr is a tricky question. I've been doing some research on that recently myself. The most helpful resource I found so far is this issue with comments by @slimsag that provided good rationale. Specifically these comments:

• Expose Window.data as *C.GLFWwindow glfw#172 (comment) (last paragraph)
• Expose Window.data as *C.GLFWwindow glfw#172 (comment)

/cc @slimsag @dominikh for their Cgo/unsafe.Pointer/uintptr expertise.

[hajimehoshi]

Thank you! From those comments, my understanding is that unsafe.Pointer forces to import unsafe package to the package users, which is not good.

Then +1 to uintptr. I'd like to know the other experts' opinions.

[dmitshur]

my understanding is that unsafe.Pointer forces to import unsafe package to the package users

I'm not sure if that's quite right. My understanding is that if a package returns unsafe.Pointer in its exported API, it makes it possible for other packages to write code that uses unsafe.Pointer without importing unsafe themselves, which is what's bad:

package main

import "your/pkg"
// note that "unsafe" isn't imported here

func main() {
    foo := pkg.DoSomething()
    // foo is unsafe.Pointer... yet this package didn't import "unsafe"
    // so it doesn't appear to be using unsafe
}

So, it's the fact that importing unsafe is not forced that we may want to avoid. @slimsag should be able to confirm if that's right.

What I don't know is how bad that is compared to the other alternatives we might have at our disposal. At the end of the day, Cgo code isn't going to be as great as one might want, so we have to find and use the least bad solution.

[slimsag]

The reason Go packages in general should not return an unsafe.Pointer is because it enables users of that package to perform dangerous behavior without directly importing unsafe on their own. Requiring users to directly import unsafe is good for two reasons:

1. It makes the user immediately aware that they are using something unsafe, and need to take caution. (otherwise they could just write e.g. *(*float)(pkg.DoSomething()) and not realize they've done an unsafe operation)
2. It makes it easier to locate which packages in a Go program have potentially unsafe consequences.

Now, the above two points are true for Go packages in general, but with OpenGL wrappers the fact of the matter is that they are often not safe APIs in general. It is very easy to pass incorrect arguments to OpenGL APIs and get e.g. an invalid pointer, stack overflow, etc., etc. Even browsers have a very hard time sanitizing OpenGL code for the purpose of WebGL, and have to e.g. create lists of driver implementations that are bad. So the benefits of the two above points are lesser for OpenGL Go wrappers.

I think uintptr makes the most sense, just because that is what is proper in general / it is the least bad solution.

One last note: C.Foobar types are type-safe. When you get a uintptr or unsafe.Pointer.. you need to know what C.Foobar type you should cast it to. We should keep these documented in the public API.

[hajimehoshi]

Thanks!

I think uintptr makes the most sense, just because that is what is proper in general / it is the least bad solution.

What do you think about the current a lot of unsafe.Pointer usages in the current gl API? I don't think we should replace them immediately though.

One last note: C.Foobar types are type-safe. When you get a uintptr or unsafe.Pointer.. you need to know what C.Foobar type you should cast it to. We should keep these documented in the public API.

+1.

[dmitshur]

I think uintptr makes the most sense, just because that is what is proper in general / it is the least bad solution.

One issue I'm currently facing with pointers to C types returned as uintptr (instead of unsafe.Pointer), e.g., GetCocoaWindow() uintptr, is that they force me to convert them back to unsafe.Pointer myself, which vet then reports lines such as:

unsafe.Pointer(window.GetCocoaWindow()) // need to convert to unsafe.Pointer to pass back to Cgo in my code

With:

./main.go:65: possible misuse of unsafe.Pointer

@slimsag Any suggestions on how to work around that other than to use unsafe.Pointer instead of uintptr?

[slimsag]

Apologies for the trouble / misleading statements here. The reason vet reports this error is because:

https://godoc.org/github.com/golang/go/src/cmd/vet#hdr-Misuse_of_unsafe_Pointers

If I understand correctly, it is because that uintptr memory could be allocated by Go, and vet has no way to check that so it is indeed a "possible misuse of unsafe.Pointer". It's just only a misuse of unsafe.Pointer if that memory is actually allocated by Go and not C.

Fundamentally, I think the problem here is: Canonical Go packages should not expose unsafe.Pointer, they should expose a safe Go API. But what we're doing here is actually rather low level (these GL packages are intended to be used by a package that does expose a safe Go API), and hence we're not subject to that same general contract. And I believe we've came to the same conclusion in past discussions here as well.

I take back what I said about uintptr being the right thing here. I think unsafe.Pointer being exposed by the API is the right way to go

[slimsag]

Looks like GitHub is having issues and didn't submit my last comment.... so I unfortunately lost it. TL;DR was that we should actually use unsafe.Pointer not uintptr here. go-gl is low-level and not really subject to the same restrictions of more canonical Go packages where e.g. only exposing a safe Go API is the target goal

[dmitshur]

Sounds good; I'm in agreement.

Following the same logic, I think we should change (or consider changing; perhaps for 3.3) GetCocoaWindow and similar methods in glfw to return unsafe.Pointer rather than uintptr. Opened issue go-gl/glfw#230 for it.

[hajimehoshi]

(Looks like GitHub shows today's comments now :-)

TBH I didn't have a strong opinion on unsafe.Pointer or uintptr so I was fine with either. Now I'm convinced that unsafe.Pointer is fine here since OpenGL APIs are special since they need to expose low-level things. I'm also in agreement :-)