Open davidmanzanares opened 4 years ago
Are you using the Go race detector? It caused similar errors for me when I knew for sure that there was nothing wrong.
I'm pretty sure this is the issue:
github.com/vulkan-go/vulkan/cgo_helpers.go:171
: var cpApplicationName_allocs *cgoAllocMap
refb0af7378.pApplicationName, cpApplicationName_allocs = unpackPCharString(x.PApplicationName)
allocsb0af7378.Borrow(cpApplicationName_allocs)
github.com/vulkan-go/vulkan/cgo_helpers.go:342
:// unpackPCharString represents the data from Go string as *C.char and avoids copying.
func unpackPCharString(str string) (*C.char, *cgoAllocMap) {
h := (*stringHeader)(unsafe.Pointer(&str))
return (*C.char)(h.Data), cgoAllocsUnknown
}
When c-for-go project started in 2015, it was relatively safe practice to pass memory behind string
into C land without copying. Nowadays it's considered not safe, moreover, this particular pointer to Go memory is stored inside C struct memory. Strings are immutable by definition, but in practice who knows how Go memory is organized.
I'll need to fix vulkan-go bindings to so unpackPCharString
would copy strings. And add \x00
too.
Perhaps a rewrite for the bindings would be in order? The bindings have worked fine for me but there are small issues like this and also some performance and memory safety questions I'm worried about, particularly with regards to wrapped structs.
@jclc as been discussed in https://github.com/xlab/c-for-go/issues/86 performance and memory safety questions with wrapped structs is a phantom problem — I've never seen any reasonable numbers, and for my use cases there is no issues at all..
There are some tricky parts in binding generation, like callbacks, especially with custom memory allocators for Vulkan. I think this is a standalone feature that needs to be improved. But there is no point in debates about performance vs safety. If you need both, then one should use Rust.
Regarding this particular case with zero-alloc string — initially this has beed implemented as a way to avoid allocation pressure. Which turns out to be not so safe and not so useful in 2020. So I'll add additional mem copy for (unnoticeable) sacrifice of performance.
Thank you!
No, I haven't used the race detector.
I'll need to fix vulkan-go bindings to so unpackPCharString would copy strings. And add \x00 too.
That would be amazing, I noticed the \x00
too, it would be a nice plus.
Btw, thank you very much for doing this project, despite GC, runtime, and all of that, I've had great experiences with Go and OpenGL in the past, it's really nice to be able to use vulkan now.
I tried the naive approach of simply replacing unpackPCharString
and packPCharString
with the cgo equivalents C.CString
and C.GoString
respectively but this did not seem to fix the issue.
Hi, I tried to enable the debug reporting callback on the go vulkan demo (vulkancube) by setting VulkanDebug() to return true:
It crashes with:
I'm pretty worried about this issue because, when I restored the original code to disable the reporting functionality, but enabled Go's CGO checker to the maximum with
GODEBUG=cgocheck=2
,vk.CreateInstance
crashed hard due to the same problem:Is this a false positive in the go tooling? Is this a real issue with these bindings? If so, I'd gladly try to help