forked from golang/go
-
Notifications
You must be signed in to change notification settings - Fork 0
cmd/cgo: improve error messages #108
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
Open
austinderek
wants to merge
42
commits into
master
Choose a base branch
from
cgo-error-messages
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5bf50a0
to
6d5b137
Compare
74f2fb0
to
e723643
Compare
7056c71
to
5bf50a0
Compare
fd91e81
to
99cf4d6
Compare
The lab confirmed the that entropy source doesn't have to be inside the module boundary, although changing the entropy source of a module does require recertification. Move the v1.0.0 entropy source out of crypto/internal/fips140, to a versioned path that lets us keep multiple versions (which would be used by different modules) if we wish to. Change-Id: I6a6a69647e9dfca1c375650a0869bdc001d65173 Reviewed-on: https://go-review.googlesource.com/c/go/+/710057 Reviewed-by: Daniel McCarney <[email protected]> Auto-Submit: Filippo Valsorda <[email protected]> Reviewed-by: David Chase <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]>
I've didn't caught theses while reviewing CL 701615. Change-Id: I721978c173a255eb6d7c3e43dea2b903a9fd016d Reviewed-on: https://go-review.googlesource.com/c/go/+/712740 LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Jorropo <[email protected]> Reviewed-by: Keith Randall <[email protected]> Reviewed-by: Keith Randall <[email protected]> Reviewed-by: David Chase <[email protected]>
I don't know why we were eliminating horses, this not cool. Change-Id: I0d4b5a1b2f584e071de0a85ef88f9baf9183e12e Reviewed-on: https://go-review.googlesource.com/c/go/+/712820 Reviewed-by: David Chase <[email protected]> Auto-Submit: Jorropo <[email protected]> Reviewed-by: Keith Randall <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Keith Randall <[email protected]>
Avoid needing an unsafe section to store LR and adjust SP for large constants by using the stdux (MOVDU) instruction. This is also a few instructions shorter as the large constant adjustment is only created once. Change-Id: I6ff7a24181cdadb1846a33129fc148dcf59b76d5 Reviewed-on: https://go-review.googlesource.com/c/go/+/710197 Reviewed-by: Keith Randall <[email protected]> Reviewed-by: Keith Randall <[email protected]> Reviewed-by: David Chase <[email protected]> Auto-Submit: Keith Randall <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
694182d
to
fd91e81
Compare
Found by github.com/mdempsky/unconvert Change-Id: I88ce10390a49ba768a4deaa0df9057c93c1164de GitHub-Last-Rev: 3b0f7e8 GitHub-Pull-Request: golang#75974 Reviewed-on: https://go-review.googlesource.com/c/go/+/712940 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Keith Randall <[email protected]> Auto-Submit: Keith Randall <[email protected]> Reviewed-by: Keith Randall <[email protected]> Reviewed-by: David Chase <[email protected]>
The test is flaking on windows, and I haven't been able to figure out why. For now, to unblock folks, just allow the value that occasionally appears: 'no errors' to avoid having a broken test. This seems like it's probably a race though so we should fix it as soon as we can. For golang#73976 Change-Id: I6a6a696431d784d048ed798b828a759e752b6393 Reviewed-on: https://go-review.googlesource.com/c/go/+/713220 LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Michael Matloob <[email protected]> Reviewed-by: Alan Donovan <[email protected]> Reviewed-by: Michael Matloob <[email protected]>
TestNISTECAllocations is flaky (~1% failure rate) on my local Windows machine since CL 710058, which touched TestEntropyRace. These tests are unrelated, but some allocations might be incorrectly accounted to TestNISTECAllocations, affecting the end result due to the low number of iterations done in that test. Change-Id: I01323c2a45b12665e86d940467f4f91c2e66696b Reviewed-on: https://go-review.googlesource.com/c/go/+/712620 Reviewed-by: David Chase <[email protected]> Reviewed-by: Roland Shoemaker <[email protected]> Auto-Submit: Quim Muntal <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
Change-Id: Ic48a756b71a62be1c6c4cfe781c02b89010e2338 GitHub-Last-Rev: 8c0d89b GitHub-Pull-Request: golang#75985 Reviewed-on: https://go-review.googlesource.com/c/go/+/713041 Reviewed-by: Keith Randall <[email protected]> Reviewed-by: Keith Randall <[email protected]> Reviewed-by: Jorropo <[email protected]> Reviewed-by: David Chase <[email protected]> Auto-Submit: Keith Randall <[email protected]> Auto-Submit: Jorropo <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
…rsions The Windows implementation of RemoveAll supports deleting read-only files only on file systems that supports POSIX semantics and on newer Windows versions (Windows 10 RS5 and latter). For all the other cases, the read-only bit was not clearer before deleting read-only files, so they fail to delete. Note that this case was supported prior to CL 75922, which landed on Go 1.25. Fixes golang#75922 Change-Id: Id6e6477f42e1952d08318ca3e4ab7c1648969f66 Reviewed-on: https://go-review.googlesource.com/c/go/+/713480 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: David Chase <[email protected]> Reviewed-by: Damien Neil <[email protected]> Auto-Submit: Damien Neil <[email protected]>
b31dc77
to
fd91e81
Compare
fe7c70a
to
5e9a432
Compare
Linux introduced new syscalls to fix the year 2038 issue. To still be able to use the old ones, the Kconfig option COMPAT_32BIT_TIME would be necessary. Use the new 64-bit syscall for timer_settime by default. Add a fallback to use the 32-bit syscall when the 64-bit version returns _ENOSYS. Fixes golang#75133 Change-Id: Iccb8831b67f665067ee526e93c3ff2f4f5392edf GitHub-Last-Rev: 6c3d62d GitHub-Pull-Request: golang#75957 Reviewed-on: https://go-review.googlesource.com/c/go/+/712642 Reviewed-by: Jorropo <[email protected]> Reviewed-by: David Chase <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Jorropo <[email protected]>
This commit improves the error messages after panics due to the sharing of an unpinned Go pointer (or a pointer to an unpinned Go pointer) between Go and C. This occurs when it is: 1. returned from Go to C (through cgoCheckResult) 2. passed as argument to a C function (through cgoCheckPointer). An unpinned Go pointer refers to a memory location that might be moved or freed by the garbage collector. Therefore: - change the signature of cgoCheckArg (it does the real work behind cgoCheckResult and cgoCheckPointer) - change the signature of cgoCheckUnknownPointer (called by cgoCheckArg for checking unexpected pointers) - introduce cgoFormatErr (it formats an error message with the caller name) - update the cgo pointer tests - remove unused code in TestPointerChecks. 1. cgoCheckResult When an unpinned Go pointer (or a pointer to an unpinned Go pointer) is returned from Go to C, > package main > > import ( > "C" > ) > > //export foo > func foo() map[int]int { > return map[int]int{0: 1,} > } This error shows up at runtime: > panic: runtime error: cgo result is unpinned Go pointer or points to unpinned Go pointer > > goroutine 17 [running, locked to thread]: > panic({0x78ac36001d40?, 0xc000194000?}) > /usr/lib/go/src/runtime/panic.go:802 +0x168 > runtime.cgoCheckArg(0x78ac36002f40, 0xc000190000, 0xd8?, 0x0, {0x78ac35fdda62, 0x42}) > /usr/lib/go/src/runtime/cgocall.go:628 +0x4c5 > runtime.cgoCheckResult({0x78ac36002f40, 0xc000190000}) > /usr/lib/go/src/runtime/cgocall.go:795 +0x4b > _cgoexp_1c29bd2c0b96_foo(0x7fffc16d87b0) > _cgo_gotypes.go:46 +0x6f > runtime.cgocallbackg1(0x78ac35fd4d20, 0x7fffc16d87b0, 0x0) > /usr/lib/go/src/runtime/cgocall.go:446 +0x289 > runtime.cgocallbackg(0x78ac35fd4d20, 0x7fffc16d87b0, 0x0) > /usr/lib/go/src/runtime/cgocall.go:350 +0x132 > runtime.cgocallbackg(0x78ac35fd4d20, 0x7fffc16d87b0, 0x0) > <autogenerated>:1 +0x2b > runtime.cgocallback(0x0, 0x0, 0x0) > /usr/lib/go/src/runtime/asm_amd64.s:1082 +0xcd > runtime.goexit({}) > /usr/lib/go/src/runtime/asm_amd64.s:1693 +0x1 _cgoexp_1c29bd2c0b96_foo is the faulty caller; it is not obvious to find it in the stack trace. Moreover the error does say which kind of pointer caused the panic; for instance, a Go map. Retrieve caller name and pointer kind; use them in the error message: > panic: runtime error: result of cgo function foo is unpinned Go map or points to unpinned Go map > > goroutine 17 [running, locked to thread]: > panic({0x76f2fd69b3c0?, 0x1fba8c79c000?}) > /mnt/go/src/runtime/panic.go:877 +0x16f > runtime.cgoCheckArg(0x76f2fd69c5c0, 0x1fba8c790000, 0x0?, 0x0, 0x1) > /mnt/go/src/runtime/cgocall.go:631 +0x499 > runtime.cgoCheckResult({0x76f2fd69c5c0, 0x1fba8c790000}) > /mnt/go/src/runtime/cgocall.go:798 +0x45 > _cgoexp_1c29bd2c0b96_foo(0x7ffd0803b8c0) > _cgo_gotypes.go:46 +0x6f > runtime.cgocallbackg1(0x76f2fd667680, 0x7ffd0803b8c0, 0x0) > /mnt/go/src/runtime/cgocall.go:446 +0x289 > runtime.cgocallbackg(0x76f2fd667680, 0x7ffd0803b8c0, 0x0) > /mnt/go/src/runtime/cgocall.go:350 +0x132 > runtime.cgocallbackg(0x76f2fd667680, 0x7ffd0803b8c0, 0x0) > <autogenerated>:1 +0x2b > runtime.cgocallback(0x0, 0x0, 0x0) > /mnt/go/src/runtime/asm_amd64.s:1101 +0xcd > runtime.goexit({}) > /mnt/go/src/runtime/asm_amd64.s:1712 +0x1 2. cgoCheckPointer When an unpinned Go pointer (or a pointer to an unpinned Go pointer) is passed to a C function, > package main > > /* > #include <stdio.h> > void foo(void *bar) {} > */ > import "C" > import "unsafe" > > func main() { > m := map[int]int{0: 1,} > C.foo(unsafe.Pointer(&m)) > } This error shows up at runtime: > panic: runtime error: cgo argument has Go pointer to unpinned Go pointer > > goroutine 1 [running]: > main.main.func1(...) > /mnt/chexit/cgomap.go:12 > main.main() > /mnt/chexit/cgomap.go:12 +0x91 > exit status 2 Retrieve callee name and pointer kind; use them in the error message. > panic: runtime error: cgo argument of function main.main.func1 has Go pointer to unpinned Go map > > goroutine 1 [running]: > main.main.func1(...) > /mnt/chexit/cgomap.go:12 > main.main() > /mnt/chexit/cgomap.go:12 +0x9b > exit status 2 Link: https://pkg.go.dev/cmd/cgo#hdr-Passing_pointers Suggested-by: Ian Lance Taylor <[email protected]> Fixes golang#75856
4620db7
to
fd91e81
Compare
5e9a432
to
89d562b
Compare
39fd61d
to
fd91e81
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello maintainers,
The PR aims to improve error messages in
cgoCheckPointer
andcgoCheckResult
.The issue details the problem in
cgoCheckResult
. Let me know if you want me to split the PR forcgoCheckPointer
.Here is a reproducer for
cgoCheckPointer
:And the error:
Fixes golang#75856
cc @ianlancetaylor, @randall77, @seankhliao
🔄 This is a mirror of upstream PR golang#75894