With Go 1.11 and HEAD: go version devel +a2a3dd00c9 Thu Sep 13 09:52:57 2018 +0000 darwin/amd64, the following program:

package main

// void f(void* ptr) {}
import "C"

import (
    "compress/gzip"
    "unsafe"
)

type cgoWriter struct{}

func (cgoWriter) Write(p []byte) (int, error) {
    ptr := unsafe.Pointer(&p[0])
    C.f(ptr)
    return 0, nil
}

func main() {
    w := cgoWriter{}
    gzw := gzip.NewWriter(w)
    gzw.Close() // calls cgoWriter.Write
}

Panics:

panic: runtime error: cgo argument has Go pointer to Go pointer

goroutine 1 [running]:
main.cgoWriter.Write.func1(0xc00015e020)
    /Users/crawshaw/junk.go:15 +0x48
main.cgoWriter.Write(0xc00015e020, 0x1, 0xf8, 0xc0000b1e50, 0x4027ff1, 0x40dad60)
    /Users/crawshaw/junk.go:15 +0x35
compress/flate.(*huffmanBitWriter).write(0xc00015e000, 0xc00015e020, 0x1, 0xf8)
    /Users/crawshaw/go/go/src/compress/flate/huffman_bit_writer.go:136 +0x5f
compress/flate.(*huffmanBitWriter).flush(0xc00015e000)
    /Users/crawshaw/go/go/src/compress/flate/huffman_bit_writer.go:128 +0xb7
compress/flate.(*huffmanBitWriter).writeStoredHeader(0xc00015e000, 0x0, 0x1)
    /Users/crawshaw/go/go/src/compress/flate/huffman_bit_writer.go:412 +0x63
compress/flate.(*compressor).close(0xc0000ba000, 0x0, 0xc0000a62b0)
    /Users/crawshaw/go/go/src/compress/flate/deflate.go:647 +0x83
compress/flate.(*Writer).Close(0xc0000ba000, 0x0, 0x0)
    /Users/crawshaw/go/go/src/compress/flate/deflate.go:729 +0x2d
compress/gzip.(*Writer).Close(0xc0000a6210, 0x419bf30, 0xc0000a6210)
    /Users/crawshaw/go/go/src/compress/gzip/gzip.go:242 +0x6e
main.main()
    /Users/crawshaw/junk.go:22 +0x47

I suspect (but cannot yet show that) this is related to the fact that gzip.Writer is passing a []byte to the Write method made out of an array field of the gzip.Writer struct.

Comment From: ianlancetaylor

cgo isn't clever enough to look at where the arguments are coming from. It should work if you write it as

    C.f(unsafe.Pointer(&p[0]))

Comment From: aenkya

Hi @ianlancetaylor . Is this something that does not need to be addressed anymore? If so, can we close the issue. If not, I would love to pick it up as a first issue

Comment From: ianlancetaylor

As far as I know, this still happens. The program above still fails in the same way.

If you want to look at this, that would be great. But I want to caution you that I don't think this will be easy to fix. At the moment I don't really see how to fix it at all.

Comment From: aenkya

Awesome. I'm delighted to investigate it and try some options. Reproducing it first..

Comment From: aenkya

@ianlancetaylor sorry I have been unavailable for a while due to some unforeseen circumstances. Up until now, - I tested out the code and confirmed behaviour - I have gone through the unsafe documentation as well as the gzip documentation. I observed a note during pointer conversion that said

Note that both conversions must appear in the same expression, with only the intervening arithmetic between them:

That would explain why this is able to work just fine as you mentioned earlier when the expressions are combined. What I intend to do within the next 3 days - Go through the cgo documentation and implementation.

I would love to also look at how the type conversion is implemented. Can you point me in any direction of where this is? Thanks

Comment From: ianlancetaylor

The relevant code here is cmd/cgo/gcc.go, around the method rewriteCall.

I don't think there is going to be any simple fix here. I think that fixing this will require a significant change to how the cgo tool works.

Comment From: Zyl9393

I believe I may be seeing a variation of this problem with this message:

cgo argument has Go pointer to unpinned Go pointer

However, I don't know how to confirm anything. The offending unsafe.Pointer is pointing to a float32-field in a struct which may or may not have escaped to heap. Do we have a comprehensive list of which situations trigger the panic and how to react?

Code:

func (ssp *StaticShaderProgram) SetLights(sphereLights []shaderstate.SphereLight) {
    numLights := int32(min(MaxLights, len(sphereLights)))
    gl.Uniform1i(ssp.uniformNumLightsLocation, numLights)
    if len(sphereLights) > 0 {
        gl.BindBufferBase(gl.UNIFORM_BUFFER, ssp.uniformBlockBindingIndexUBLights, ssp.uniformBlockBufferUBLights)
        gl.BufferSubData(gl.UNIFORM_BUFFER, 0, ssp.uniformBlockBufferSizeUBLights, unsafe.Pointer(&sphereLights[0].Position[0])) // panic!
    }
}

No luck using pinner, either:

var p runtime.Pinner
ptr := &sphereLights[0].Position[0]
p.Pin(ptr)
gl.BufferSubData(gl.UNIFORM_BUFFER, 0, ssp.uniformBlockBufferSizeUBLights, unsafe.Pointer(ptr)) // still panics
p.Unpin()

Comment From: ianlancetaylor

Questions are better asked on a forum. See https://go.dev/wiki/Questions. Thanks.

I'm not aware of any comprehensive list of cases where this arises. The basic idea is pretty simple: the cgo tool can't handle complex expressions that take the address of a field in a struct, and so it treats a pointer as a pointer to the entire struct. If that struct has other fields that contain Go pointers, you get the cgo error at run time. The workaround is to simplify the expressions of rearrange the struct so that it doesn't contain Go pointers.

Note that addresses passed to cgo always escape.

Comment From: strager

The relevant code here is cmd/cgo/gcc.go, around the method rewriteCall.

I don't think there is going to be any simple fix here. I think that fixing this will require a significant change to how the cgo tool works.

Idea (motivated by #74117; not sure if it's relevant to this issue):

  • runtime.cgoCheckArg() accepts another parameter, sizeHint, which rewriteCall() sets to the byte size of the arg's pointee.

  • When runtime.cgoCheckArg() processes a struct, it only considers fields that overlap p..(p+sizeHint-1). In other words, it ignores fields before and after the struct field that's actually pointed to.

Tradeoffs:

  • For arrays, we don't know how much would be accessed, so sizeHint might be too low. My algorithm would make the implementation more conservative in its checking (i.e. more false negatives).

If this sounds like a good plan that would solve the problem, I'm happy to take a stab at implementing it @ianlancetaylor. I haven't touched Go before; this would be my first contribution. (Note to future self: https://go.dev/doc/contribute)

Comment From: ianlancetaylor

If we know the byte size of the arg's pointee, then we know the pointer type. And that's all we need to know. The issue arises when cgo has an unsafe.Pointer and doesn't know what it points to. So I think the fix has to center around improving cgo's ability to know the type of the pointer.