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/fix: incomplete "go tool fix -r cftype" suggestions #23091

Closed
kevinburke opened this issue Dec 11, 2017 · 10 comments
Closed

cmd/fix: incomplete "go tool fix -r cftype" suggestions #23091

kevinburke opened this issue Dec 11, 2017 · 10 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.10

Comments

@kevinburke
Copy link
Contributor

Go 1.10 introduces some changes to cgo, but I ran "go tool fix -r cftype" and still got compilation failures. Is the team still interested in improving the heuristics for "go tool fix"? I'm just an end user of a lot of these libraries, I haven't done that much with cgo and I'm not sure how to fix them.

Running "go tool fix" on this package makes some changes but there are still a number of compilation failures: keybase/go-keychain#19

Also the "remaining change" listed here: golang/text@572a2b1

Feel free to close as "this library needs to do some work, go tool fix is relatively complete."
@ianlancetaylor
Copy link
Contributor

CC @randall77

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Dec 11, 2017
@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 11, 2017
@ianlancetaylor ianlancetaylor changed the title cmd/cgo: incomplete "go tool fix -r cftype" suggestions cmd/fix: incomplete "go tool fix -r cftype" suggestions Dec 11, 2017
@odeke-em
Copy link
Member

odeke-em commented Jan 9, 2018

Am not sure if this will be for Go1.10. Perhaps let's punt to Go1.11?

@ianlancetaylor
Copy link
Contributor

ping @randall77

@randall77
Copy link
Contributor

(I responded to this yesterday! Somehow it didn't stick.)

There seem to be 3 different issues here.

  1. Incomplete typechecking. go tool fix has a limited typechecker. It doesn't have the type information for the C functions like C.CFDataCreate. So it doesn't know that the argument or return types are. For example:
cfData := C.CFDataCreate(nil, p, C.CFIndex(len(b)))

It doesn't know to rewrite the nil to 0 or the subsequent comparison of cfData == nil to cfData == 0.
I think the problem here is that the typechecker doesn't handle global variables, only functions. The C.* functions are actually variables. I'll see if I can upgrade the typechecker for these.

  1. Converting pointers to CF*Ref types. Example:
		C.CFDictionaryGetKeysAndValues(cfDict, (*unsafe.Pointer)(&keys[0]), (*unsafe.Pointer)(&values[0]))

It used to be that &CFTypeRef was convertible to *unsafe.Pointer as above. Now it will need an additional unsafe.Pointer cast, like this: (*unsafe.Pointer)(unsafe.Pointer(&keys[0])). We might be able to detect this automatically.

  1. There's a whole set of types the rewrite doesn't apply to, and it probably should. C.SecAccessRef, C.SecTrustedApplicationRef, C.SecKeychainRef, C.SetKeychainItemRef. I've never heard of them before, but they look convertible to C.CFTypeRef so they should be covered. I'll add the C.Sec*Ref pattern also. Maybe also C.*Ref? It will cover future discoveries like this, but also risks including types we might not want to.

I definitely want to handle 1 and 3. 2 is more questionable, as it involves a more complicated rewrite.

@randall77
Copy link
Contributor

Actually, number 1 is because cgo isn't run before typechecking, so we don't have the types of the C. functions at all.

@randall77
Copy link
Contributor

So it looks like there are a lot of types that are upcastable to a CFTypeRef. Grepping though header files, I find 214 of them. They have no particular prefix, just the Ref suffix.
For number 3, I think we need to go with C.*Ref as the matching type.

@gopherbot
Copy link

Change https://golang.org/cl/87615 mentions this issue: cmd/cgo: rewrite all *Ref types on Darwin to uintptr

@gopherbot
Copy link

Change https://golang.org/cl/87616 mentions this issue: cmd/fix: extend typechecker to use cgo types

gopherbot pushed a commit that referenced this issue Jan 17, 2018
@randall77
cmd/cgo: rewrite CFTypeRef and subytes on Darwin to uintptr
d162a29 
Cgo currently maps CFTypeRef and its subtypes to unsafe.Pointer
or a pointer to a named empty struct.

However, Darwin sometimes encodes some of CFTypeRef's subtypes as a
few int fields packed in a pointer wrapper. This hackery confuses the
Go runtime as the pointers can look like they point to things that
shouldn't be pointed at.

Switch CFTypeRef and its subtypes to map to uintptr.

Detecting the affected set of types is tricky, there are over 200 of
them, and the set isn't static across Darwin versions. Fortunately,
downcasting from CFTypeRef to a subtype requires calling CFGetTypeID,
getting a CFTypeID token, and comparing that with a known id from a
*GetTypeID() call. So we can find all the type names by detecting all
the *GetTypeID() prototypes and rewriting the corresponding *Ref types
to uintptr. This strategy covers all the cases I've checked and is
unlikely to have a false positive.

Update #23091.

Change-Id: I487eb4105c9b4785ba564de9c38d472c8c9a76ac
Reviewed-on: https://go-review.googlesource.com/87615
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Jan 17, 2018
@randall77
cmd/fix: extend typechecker to use cgo types
2dc025e 
If a file uses cgo, incorporate the types generated by running cgo.

Update #23091

Change-Id: I10958fa7fd6027c2c96a9fd8a9658de35439719f
Reviewed-on: https://go-review.googlesource.com/87616
Reviewed-by: Robert Griesemer <gri@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/88075 mentions this issue: cmd/fix: don't depend on *GetTypeID functions being present

gopherbot pushed a commit that referenced this issue Jan 17, 2018
@randall77
cmd/fix: don't depend on *GetTypeID functions being present
bd89333 
cgo uses the presence of these functions to determine whether
a given type is in the CFTypeRef hierarchy and thus should be
a uintptr instead of a pointer. But if the *GetTypeID functions
aren't used by the user code, then they won't be present in the
cgo output, and thus cmd/fix won't see them.

Use the simpler rule that anything ending in *Ref should be
rewritten. This could over-rewrite, but I don't see a simpler
solution. Unlike cgo, it is easy to edit the output to fix any
issues. And fix is a much rarer operation than cgo.

This is a revert of portions of CL 87616.

Update #23091

Change-Id: I74ecd9fb25490a3d279b372e107248452bb62185
Reviewed-on: https://go-review.googlesource.com/88075
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/88175 mentions this issue: cmd/fix: add intermediate cast for *C.CFTypeRef <-> *unsafe.Pointer

@golang golang locked and limited conversation to collaborators Jan 19, 2019
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.
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
5 participants
@kevinburke @ianlancetaylor @odeke-em @randall77 @gopherbot