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

proposal: cmd/cgo: get rid of CFType, JNI, EGL unsafe.Pointer => uintptr special cases #41337

Open
eliasnaur opened this issue Sep 11, 2020 · 8 comments

Comments

@eliasnaur
Copy link
Contributor

@eliasnaur eliasnaur commented Sep 11, 2020

Cgo used to represent opaque C types such as typedef void *EGLDisplay; as unsafe.Pointer just like it treats any other C pointer type. However, some C types are sometimes used for non-address values, for example handles into an internal datastructure. When the Go garbage collector sees a non-pointer value, it may crash with a fatal error: invalid pointer found on stack. The latest example are the JNI jmethodID and jfieldID types (#40954).

Before CL 250940, each such special type would need a special case in cgo to treat them as uintptrs, not unsafe.Pointer. See the cmd/cgo documentation and supporting code,

func (c *typeConv) badCFType(dt *dwarf.TypedefType) bool {

func (c *typeConv) badJNI(dt *dwarf.TypedefType) bool {

func (c *typeConv) badEGLType(dt *dwarf.TypedefType) bool {

for the handling of Apple's CFType hierarchy, Java's JNI types, EGL types.

CL 250940 introduces a general fix, and no further types need special casing.

This proposal is about getting rid of the remaining uintptr special cases in Go 1.16. More specifically I propose that when go.mod contains go 1.16 as the source version, the special cases no longer apply. Moreover, I propose that a future Go version (no earlier than 1.18) removes the special cases altogether.

The downside is that users referring to one or more of the special cased types would potentially incur a one-time cost of converting their code, most likely the use of zero values (0 to nil). I expect few users to be directly affected by this proposal.

Note that it's possible to maintain compatibility with both Go 1.16 and previous versions of Go by using a variable for zero values:

// works regardless of C.EGLDisplay being an unsafe.Pointer or uintptr.
var zeroEGLDisplay C.EGLDisplay
...
var someEGLDisplay C.EGLDisplay = ...
if someEGLDisplay == zeroEGLDisplay {
   ...
}

The upside is that a significant body of special cases including supporting code, documentation and tests no longer applies in Go 1.16 and will disappear altogether in the future. A nice side-effect is that code written for Go versions before the special cases were introduced would work without changes in Go 1.16.

@gopherbot gopherbot added this to the Proposal milestone Sep 11, 2020
@gopherbot gopherbot added the Proposal label Sep 11, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 11, 2020

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Sep 11, 2020
@randall77
Copy link
Contributor

@randall77 randall77 commented Sep 14, 2020

I'd rather leave the special cases as is. Yes, it is a wart that we'll have to live with ~forever. But it isn't a big wart - you'll just have to use 0 instead of nil in a few places.
But leaving the special cases in means no one has to rewrite their code. Adding a go1.16 to the go.mod (presumably, for other unrelated reasons) won't trigger any required code changes. And we won't have a one-way gate from 1.15 to 1.16 that when people walk through, they can't walk back.

@eliasnaur
Copy link
Contributor Author

@eliasnaur eliasnaur commented Sep 15, 2020

And we won't have a one-way gate from 1.15 to 1.16 that when people walk through, they can't walk back.

Can you elaborate? As explained in my proposal, you can make your code compatible with both Go 1.16 and previous versions by using implicitly zeroed variables of the special cased types.

@randall77
Copy link
Contributor

@randall77 randall77 commented Sep 15, 2020

True, if people adopt that pattern then their code will work in both 1.15 and 1.16.
There is no way to enforce that adoption, however. You'd have to test on both 1.15 and 1.16 to make sure you got it. I imagine some package authors will not do that. They'll have a = 0 or = nil assignment somewhere, and someone importing their package and using a different Go version will end up hitting a compile error.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 15, 2020

We could in principle use the language version in the go.mod file to control how we handle these types. Then you might get a compilation error when updating to a new language version, but at least people importing your module wouldn't see a problem.

@randall77
Copy link
Contributor

@randall77 randall77 commented Sep 15, 2020

I guess if you put a 1.16 in your go.mod file, then anyone importing your package must be building with >= 1.16, so that works.
Then the trick Elias uses to avoid 0 or nil isn't really necessary. Just use nil everywhere.
But then you've forced all of the users of your package to upgrade to 1.16 (or keep using an old version of your package).

Or you don't use go.mod, but then you have to ensure you've used Elias's trick correctly, or use buildtags, ... Possible, but also not ideal.

So while I understand the motivation, the benefit seems minor and there's a real cost.

@rsc
Copy link
Contributor

@rsc rsc commented Sep 16, 2020

I guess if you put a 1.16 in your go.mod file, then anyone importing your package must be building with >= 1.16, so that works.

This is not strictly speaking true. Go 1.15 will still try to compile the code, it will just include "by the way the module says it wants Go 1.16" along with any compile error. Maybe that's enough.

We could also wait on this until GOPATH is gone (so everyone has go.mod).

@rsc rsc moved this from Incoming to Active in Proposals Sep 16, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Sep 23, 2020

It sounds like in the long term it would be good to clean up, but we need to wait for GOPATH to be gone so that all Go code is versioned at least at the top module (and then we'll define the implicit Go version for module dependencies without go.mod, but we can't do that yet). Putting on hold for now.

@rsc rsc moved this from Active to Hold in Proposals Sep 23, 2020
@rsc rsc added the Proposal-Hold label Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.