Closed hajimehoshi closed 5 years ago
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:
/cc @slimsag @dominikh for their Cgo/unsafe.Pointer
/uintptr
expertise.
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.
my understanding is that
unsafe.Pointer
forces to importunsafe
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.
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:
*(*float)(pkg.DoSomething())
and not realize they've done an unsafe operation)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.
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.
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
?
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
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
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 https://github.com/go-gl/glfw/issues/230 for it.
(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 :-)
Now gl packages has APIs that expose C types:
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 tovoid*
: 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 pointerC.GLDEBUGPROCAMD
is a function pointer: https://www.khronos.org/registry/OpenGL/extensions/AMD/AMD_debug_output.txtC.GLeglImageOES
is equiavalent tovoid*
: https://www.khronos.org/registry/OpenGL/extensions/OES/OES_EGL_image_external.txtC.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:
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