runtime: prematurely marking CGO pointers as unreachable? #23579
Labels
FrozenDueToAge
NeedsInvestigation
Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
release-blocker
Milestone
Comments
brknstrngz
changed the title
|
You're right: you should not need to add explicit |
|
I don't have a minimal program wrapping C directly that reproduces this, as I am calling into C through two CGO wrapping layers (one is 3rd party and fairly involved), but this program should do the trick. It needs to link against this commit, currently last on the flier/gohs master branch to exhibit the symptom. The commit in flier/gohs that works around the problem is here. Hope this helps. |
|
Thanks. I compiled that program with tip. After running for a few minutes, I got this crash. Is this the expected failure mode? |
|
Hm, no, that is new (and odd). I don't have tip at had at the moment, I'll get it tomorrow, test again and report back. The original crash dump with 1.9.3 is here. |
|
Got a different crash: |
|
Yes, that's the one we've been seeing without the KeepAlive trick. I have managed to get 1.10rc1 installed on my laptop and can confirm it still occurs. |
|
The escape analysis reported by the compiler looks correct. cgo converts the function in question to this: func hsDeserializeDatabaseAt(data []byte, db hsDatabase) error {
if ret := func(_cgo0 *_Ctype_char, _cgo1 _Ctype_size_t, _cgo2 *_Ctype_struct_hs_database) _Ctype_hs_error_t {; _cgoCheckPointer(_cgo2); return (_Cfunc_hs_deserialize_database_at)(_cgo0, _cgo1, _cgo2);}((*_Ctype_char)(unsafe.Pointer(&data[0])), _Ctype_size_t(len(data)), db); ret != (_Ciconst_HS_SUCCESS) {
return HsError(ret)
}
return nil
}Compiling that with |
|
The function //go:cgo_unsafe_args
func _Cfunc_hs_deserialize_database_at(p0 *_Ctype_char, p1 _Ctype_size_t, p2 *_Ctype_struct_hs_database) (r1 _Ctype_hs_error_t) {
_cgo_runtime_cgocall(_cgo_676e1442a9c2_Cfunc_hs_deserialize_database_at, uintptr(unsafe.Pointer(&p0)))
if _Cgo_always_false {
_Cgo_use(p0)
_Cgo_use(p1)
_Cgo_use(p2)
}
return
}For that the compiler reports |
|
I guess what matters here is the liveness, but that also looks OK: |
ianlancetaylor
added
the
NeedsInvestigation
|
@ianlancetaylor You may check hsScanVector which cause the crash and it is using two layers pointer array. |
|
@flier Thanks for the pointer. That code is not safe and breaks the rule of is not permitted. See https://golang.org/pkg/unsafe/#Pointer , which lists all the valid ways that an Also the call to I recommend fixing the code and seeing whether the problem still occurs. |
|
I have to use those |
|
The runtime error Unfortunately I don't know the problem domain well enough to know what to suggest. Perhaps you could provide an API to aggregate the bytes in C memory, rather than taking a |
|
I think the problem is the Go slice of slices using different memory layout to the C array of array, the trick is dirty but works and efficient, thanks anyway |
|
The trick doesn't work. In the absence of other information, I'm going to assume that it is causing the program crashes reported here. Closing this issue. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (
go version)?1.9.3
Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env)?amd64, freebsd (clang 4.0) and darwin (clang-900.0.39.2)
What did you do?
While investigating a crash in the hyperscan Go wrapper it seemed that pointers passed to CGO were marked as unreachable and thus reaped by the GC just before the actual CGO call. We worked around this by explicitly marking them as reachable until after the CGO call.
What did you expect to see?
The CGO wrapper not requiring explicit runtime.KeepAlive() calls.
What did you see instead?
The CGO wrapper requiring explicit runtime.KeepAlive() calls.
If this works as intended, please ignore and close the ticket. Thanks.
The text was updated successfully, but these errors were encountered: