-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
cmd/cgo: type confusion between CGO and CoreFoundation types on Go1.10 [1.10 backport] #25036
Comments
Change https://golang.org/cl/122818 mentions this issue: |
Change https://golang.org/cl/124218 mentions this issue: |
Change https://golang.org/cl/124217 mentions this issue: |
Change https://golang.org/cl/124215 mentions this issue: |
Change https://golang.org/cl/124216 mentions this issue: |
Change https://golang.org/cl/128095 mentions this issue: |
Change https://golang.org/cl/128096 mentions this issue: |
Closed by merging 48016b8 to release-branch.go1.10. |
Closed by merging 424f4ea to release-branch.go1.10. |
… for bad C pointer types We need to determine whether arguments to and return values from C functions are "bad" typedef'd pointer types which need to be uintptr on the Go side. The type of those arguments are not specified explicitly. As a result, we never look through the C declarations for the GetTypeID functions associated with that type, and never realize that they are bad. However, in another function in the same package there might be an explicit reference. Then we end up with the declaration being uintptr in one file and *struct{...} in another file. Badness ensues. Fix this by doing a 2-pass algorithm. In the first pass, we run as normal, but record all the argument and result types we see. In the second pass, we include those argument types also when reading the C types. Update #25036 Change-Id: I8d727e73a2fbc88cb9d9899f8719ae405f59f753 Reviewed-on: https://go-review.googlesource.com/122575 Run-TryBot: Keith Randall <khr@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> (cherry picked from commit 20803e0f52809fa6088285c1c87246642df2b62d) Reviewed-on: https://go-review.googlesource.com/122818
Two fixes: 1) Typedefs of the bad typedefs should also not be rewritten to the underlying type. They shouldn't just be uintptr, though, they should retain the C naming structure. For example, in C: typedef const __CFString * CFStringRef; typedef CFStringRef SecKeyAlgorithm; we want the Go: type _Ctype_CFStringRef uintptr type _Ctype_SecKeyAlgorithm = _Ctype_CFStringRef 2) We need more types than just function arguments/return values. At least we need types of global variables, so when we see a reference to: extern const SecKeyAlgorithm kSecKeyAlgorithmECDSASignatureDigestX962SHA1; we know that we need to investigate the type SecKeyAlgorithm. Might as well just find every typedef and check the badness of all of them. This requires looping until a fixed point of known types is reached. Usually it takes just 2 iterations, sometimes 3. Update #25036 Change-Id: I32ca7e48eb4d4133c6242e91d1879636f5224ea9 Reviewed-on: https://go-review.googlesource.com/123177 Run-TryBot: Keith Randall <khr@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-on: https://go-review.googlesource.com/124215
…and earlier The test uses functions from C that were introduced in OSX 1.12. Include stubs for those functions when compiling for 1.11 and earlier. This test really a compile-time test, it doesn't matter much what the executed code actually does. Use a nasty #define hack to work around the fact that cgo doesn't support static global variables. Update #25036 Change-Id: Icf6f7bc9b6b36cacc81d5d0e033a2ebaff7e0298 Reviewed-on: https://go-review.googlesource.com/123715 Run-TryBot: Keith Randall <khr@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-on: https://go-review.googlesource.com/124216
The test in CL 123715 doesn't work on iOS, it needs to use a different version scheme to determine whether SecKeyAlgorithm and friends exist. Restrict the old version test to OSX only. The same problem occurs on iOS: the functions tested don't exist before iOS 10. But we don't have builders below iOS 10, so it isn't a big issue. If we ever get older builders, or someone wants to run all.bash on an old iOS, they'll need to figure out the right incantation. Update #25036 Change-Id: Ia3ace86b00486dc172ed00c0c6d668a95565bff7 Reviewed-on: https://go-review.googlesource.com/123959 Run-TryBot: Keith Randall <khr@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-on: https://go-review.googlesource.com/124217 Reviewed-by: Ian Lance Taylor <iant@golang.org>
TARGET_OS_OSX is the right macro, but it also was only introduced in 1.12. For 1.11 and earlier a reasonable substitution is TARGET_OS_IPHONE == 0. Update #25036 Change-Id: I5f43c463d14fada9ed1d83cc684c7ea05d94c5f3 Reviewed-on: https://go-review.googlesource.com/124075 Run-TryBot: Keith Randall <khr@golang.org> Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-on: https://go-review.googlesource.com/124218
Ensure that we call FinishType on all the types added to the ptrs map. We only add a key to ptrKeys once. Once we FinishType for that key, we'll never look at that key again. But we can add a new type under that key later, and we'll never finish it. Make sure we add the key to the ptrKeys list every time we make the list of types for that key non-empty. This makes sure we FinishType each pointer type exactly once. Update #25036 Change-Id: Iad86150d516fcfac167591daf5a26c38bec7d143 Reviewed-on: https://go-review.googlesource.com/126275 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-on: https://go-review.googlesource.com/128095
…h __builtin types Expanding __builtin types (__builtin_va_list, particularly) leads to problems because they are expanded by the compiler itself - the expansions are not generated by anything in a .h file. The types a __builtin type expand to are thus very confusing to cgo. See CL 126275. Fixes #25036. Change-Id: I66eb6a4f27f652f1b934ba702f580f6daa62a566 Reviewed-on: https://go-review.googlesource.com/127156 Run-TryBot: Keith Randall <khr@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-on: https://go-review.googlesource.com/128096
@FiloSottile requested issue #24161 to be considered for backport to the next 1.10 minor release.
The text was updated successfully, but these errors were encountered: