x/tools/gopls: improve support for authoring cgo packages

[heschi]

Forked from #31561. This issue is not for users of cgo packages; that issue is #35720.

Beta instructions: https://github.com/golang/go/issues/35721#issuecomment-631669061 contains instructions on how to build a version of Go and gopls that should work well for authoring cgo packages.

Files that import "C" are not really valid Go. They depend heavily on extra information that is produced by the go tool when it builds the package. They can be parsed, but not type checked, though there is some rudimentary support for working around the issues by setting the go/types.Config.FakeImportC flag.

Many people have suggested that gopls use the package's GoFiles rather than its CompiledGoFiles. The GoFiles are the original (invalid) files. Using those may give a slightly better experience for people editing the package, but it will completely break anything that depends on the type information, since the package will no longer type check. We would have to evaluate the effects very carefully before making that change in the current architecture.

We could consider type checking cgo packages twice, once with the real code and once with the user's code. That would be a significant architectural change.

Alternatively, we could run the cgo tool as the .go files change to produce the real code. This is a much more robust approach, but the cost of running cgo processing may be too high, especially on large packages.

No matter what, this is a large project that will need a lot of testing and thought. It's unlikely we'll make huge improvements here very soon.

[OneOfOne]

A suggestion (not sure how viable it is) would be to use clangd if it's available and proxy the C code to it.

[heschi]

That's probably overkill, but does point to an approach I didn't mention: tightly integrate cgo processing into gopls so that we can mitigate the performance problems of running the actual cgo tool. As you imply, the expensive part of cgo is learning about the C code. If we could separate that part from the code generation part, we could cache the C information and run just code generation when .go files changed. That might be the ideal solution, but would require changes to the cgo tool's implementation.

[mdempsky]

Possibly related to #16623?

(It's been a while since I've worked on any go/types-based tools though. I seem to recall something changed where it became easier to process cgo-using packages, if still not perfect, and working on #16623 dropped in priority for me... maybe column position information?)

[heschi]

Definitely related. If the net result of that change was that GoFiles was [x.go, y.go] and CompiledGoFiles was [x.go,y.go,$GOCACHE/.../_cgo_gotypes.go] per https://github.com/golang/go/issues/16623#issuecomment-263477287 then I think we would be in much better shape.

[mdempsky]

@heschik To confirm, by "much better shape" do you mean you think there would still be further work to do to address this issue? (Seems like this would address #35720 too, actually.)

Looking at CLs 33677 and 33678 again, I think they're probably in reasonable shape to polish up for the next release cycle. I think the main issues are:

1. Is go/types.Config.UsesCgo the right API for enabling that setting within go/types?

2. CL 33678 needs to set go/types.Config.UsesCgo instead of FakeImportC. (CL 33677 PS1 originally just extended the semantics of FakeImportC; PS4 instead added UsesCgo, but it looks like I never pushed a matching revision of CL 33678.)

3. It probably also needs to be ported to go/packages too? How does gopls actually invoke go/types?

[heschi]

Further work: Yes -- we'd still have to regenerate the generated file sometimes. But that's a much more manageable concern.

35720: I think we want a fix for that before 1.15 is released. Otherwise I'd say yeah.

If you have a set of patches that work against master, even if the API is rough, I'm happy to work to get go/packages working on top of it if that's necessary. But as long as go list -json returns the right stuff in CompiledGoFiles there shouldn't be anything much to do for either go/packages or gopls -- both just follow go list's lead.

[mdempsky]

@heschik FYI, I just rebased CL 33677 and fixed merge conflicts. I verified that tests still pass, but haven't done anything more extensive than that.

I don't see CompiledGoFiles when I run go list -json net though. Is cmd/go supposed to be running cmd/cgo?

[mdempsky]

Nevermind. Read the docs and realized it needs to be invoked as go list -json -compiled net.

[mdempsky]

I think cmd/go will need a new flag to indicate that you want the unprocessed versions of CgoFiles rather than the processed versions. Old tools might not be able to handle the processed versions, and we can't just break those. I'm not sure what the best way to control this is.

However, for the short term, here's what I think you can do within go/packages:

1. CompiledGoFiles consists of (in this order): GoFiles, cgo(CgoFiles), SFiles, CFiles, CXXFiles.

2. The cgo(CgoFiles) list consists of 1 entry per CgoFiles, plus 2 more fixed entres. _cgo_types.go is the first element (and is what provides the C pseudo-package declarations); the last one is _cgo_dynimport.go (which just provides compiler directives that influence linker behavior, and isn't relevant to go/types).

3. You want to pass to go/types: GoFiles, CgoFiles, and the first file from cgo(CgoFiles). I.e., rather than taking CompiledGoFiles and filtering out the non-go files, take GoFiles; and if CgoFiles is non-empty, take CgoFiles and CompiledGoFiles[len(GoFiles)] too.

4. You also need to set go/types.Config.UsesCgo = true.

[mdempsky]

https://go-review.googlesource.com/c/tools/+/208264 is an ugly hack to do the above. It passes go test, but I haven't tried using it with gopls.

[mdempsky]

Updated CL 33677 for parity with CL 43970.

With CL 33677 and CL 208264 in place, I can run:

$ cd $(go env GOROOT)/misc/cgo/test
$ GOPACKAGESDRIVER=off gopackages -json -mode=types .

and confirm that it's processing the original .go sources, and it's type-checking okay.

Edit: GOPACKAGESDRIVER=off is necessary because I simply hacked the golist driver implementation. The proper solution here will probably require extending the go/packages driver interface.

[mdempsky]

@heschik I think CLs 33677 and 208264 should be in good shape if you want to play with them in the context of gopls.

[heschi]

I'm just about done with #35720, and then I'm excited to give the patches a try. Should be Monday.

[heschi]

So, this is a bit of a good news/bad news situation.

The good news is that @mdempsky's patches work beautifully, and features like autocomplete start working in cgo files with no changes to gopls whatsoever. As I said before, there's still some work to re-trigger cgo compilation when appropriate, and the import "C" line currently shows an error, but these should be pretty tractable problems.

The bad news is that these are changes to the compiler and standard library. As such, they won't be ready until Go 1.15, scheduled for about 8 months from now. Hopefully we can land them early in the cycle so that people can try them out. If there's demand, I can write up some instructions on how to set up an environment with local patches even now.

[mdempsky]

The bad news is that these are changes to the compiler and standard library.

It's only a standard library change, so it should be possible to backport the changes via vendoring if gopls users want to use it before 1.15.

[heschi]

I don't think that's a good option in module mode, unfortunately.

[mdempsky]

@heschik Fair enough. I'm still very much a modules noob, so I defer to your expertise on how best to address the issue. I mostly wanted to clarify that compiler changes aren't involved here.

I'd be curious what happens for "jump to definition" on a C.foo symbol. I'm guessing it jumps to the _cgo_gotypes.go file?

We could maybe get cgo to emit //line directives to make it point into the C preamble and/or header files if that would be more useful.

[sorenvonsarvort]

So, this is a bit of a good news/bad news situation.

The good news is that @mdempsky's patches work beautifully, and features like autocomplete start working in cgo files with no changes to gopls whatsoever. As I said before, there's still some work to re-trigger cgo compilation when appropriate, and the import "C" line currently shows an error, but these should be pretty tractable problems.

The bad news is that these are changes to the compiler and standard library. As such, they won't be ready until Go 1.15, scheduled for about 8 months from now. Hopefully we can land them early in the cycle so that people can try them out. If there's demand, I can write up some instructions on how to set up an environment with local patches even now.

Wow, that would be so useful. There are many people who use cgo because of various reasons, I think all of us will appreciate the information so much.

[heschi]

This is an experimental patch set. Please be ready for some glitches.

Okay, here you go. Note that the patched Go installation must be first on your path when you run your editor, otherwise it won't work.

While trying it out, please use this mental model: The "C" package is populated on demand based on your cgo comments and the identifiers used by your Go code. It will generally only be created once when gopls starts up. If you modify a cgo comment, or start using a new identifier, you may have problems with things not being found, or not being of the right type. In that case you'll need to restart the language server. In VS Code, that means pressing ctrl-shift-P and choosing "Go: Restart Language Server".

Other than that, features should all just work. For now, let's keep feedback on this issue. I may ask you to file new ones if it gets messy.

$ mkdir ~/gopls-cgo
$ cd ~/gopls-cgo
$ git clone https://go.googlesource.com/go
Cloning into 'go'...
remote: Counting objects: 22, done59 MiB ...Counting objects: 1   
remote: Total 403135 (delta 306410), reused 403135 (delta 306410)
Receiving objects: 100% (403135/403135), 260.03 MiB | 39.93 MiB/s, done.
Resolving deltas: 100% (306410/306410), done.
$ cd go/src
$ git checkout -b cgo
$ git fetch "https://go.googlesource.com/go" refs/changes/77/33677/7 && git cherry-pick FETCH_HEAD
From https://go.googlesource.com/go
 * branch                  refs/changes/77/33677/7 -> FETCH_HEAD
Auto-merging src/go/types/resolver.go
Auto-merging src/go/types/check.go
[...]
$ export GOROOT_BOOTSTRAP=$(go env GOROOT)
[...]
$ cd ~/gopls-cgo 
$ export PATH=$HOME/gopls-cgo/go/bin:$PATH
$ git clone https://go.googlesource.com/tools                                                     
Cloning into 'tools'...
remote: Counting objects: 16, done0 MiB ...Counting objects: 1   
remote: Total 36491 (delta 24075), reused 36491 (delta 24075)
Receiving objects: 100% (36491/36491), 18.77 MiB | 39.80 MiB/s, done.
Resolving deltas: 100% (24075/24075), done.
$ cd tools/gopls
$ git checkout -b cgo
$ git fetch "https://go.googlesource.com/tools" refs/changes/64/208264/1 && git cherry-pick FETCH_HEAD
From https://go.googlesource.com/tools
 * branch              refs/changes/64/208264/1 -> FETCH_HEAD
Auto-merging go/packages/packages.go
Auto-merging go/packages/golist.go
[...]
$ go install

[sorenvonsarvort]

GOROOT_BOOTSTRAP=$(dirname $(which go))/.. ./make.bash

In Arch Linux You should write GOROOT_BOOTSTRAP=/usr/lib/go, but $ which go returns /usr/bin/go.

Thank You, it works like magic!

[heschi]

@mdempsky: any plans to get this landed? I would like to get more testers using it ASAP.

[gopherbot]

Change https://golang.org/cl/33677 mentions this issue: go/types: add UsesCgo config to support _cgo_gotypes.go

[gopherbot]

Change https://golang.org/cl/231459 mentions this issue: go/types: add UsesCgo config to support _cgo_gotypes.go

[gopherbot]

Change https://golang.org/cl/229779 mentions this issue: internal/lsp: use TypecheckCgo when possible

[gopherbot]

Change https://golang.org/cl/234103 mentions this issue: internal/lsp: add Regenerate Cgo code lens

[heschi]

Here's the current status: if you build gopls at master with tip Go, you should have a pretty good cgo authoring experience. The one bug I know of is autocomplete, which will offer symbols like _cgo_foo instead of C.foo.

One bump, though, is that it won't regenerate the cgo bindings unless you tell it to. To that end, there should be a "regenerate cgo" popup on top of the import "C" line in a file that uses cgo. You'll need to use it when you reference a new identifier from C, or change the magic comment in a way that affects the symbols Go can see.

To get set up, follow these instructions:

$ go get golang.org/dl/gotip
$ gotip download
[...]
Success. You may now run 'gotip'!
$ cd /
$ GO111MODULE=on gotip get golang.org/x/tools/gopls@master
$ gotip version
... devel +f7f9c8f ...
$ gotip version $(which gopls)
... devel +f7f9c8f ...

You don't need to use tip Go to work on your own project, just to build gopls.

Please leave a reaction on this comment if you try it out, and comment with any bugs you find.

[beakbeak]

Thanks for this! I encountered the following errors from gopls that do not occur when running go build:

// enum EnumType { EnumValue };
import "C"

func ReturnEnum() C.enum_EnumType { // enum_EnumType not declared by package C
    return C.EnumValue
}

// int ReturnInt(int x) { return x; }
import "C"
import "unsafe"

func ReturnFunctionAsPointer() unsafe.Pointer {
    return unsafe.Pointer(C.ReturnInt)
}

func CallFunction() C.int {
    return C.ReturnInt(1) // invalid operation: cannot call non-function C.ReturnInt (variable of type unsafe.Pointer)
}

// typedef struct Struct { int Member; } StructType;
import "C"

func (*C.StructType) method() int {
    return 0
}

func CallMethod(foo *C.StructType) {
    foo.method() // foo.method undefined (type *_Ctype_struct_Struct has no field or method method)
}

[heschi]

@beakbeak Thanks! @mdempsky, see https://golang.org/cl/234878 for tests. I checked that at least one of them works fine with the real compiler. Given that much of the other C stuff works, I don't think this is on my end of things.

[mdempsky]

@beakbeak Thanks for the report. I'll look into those.

As a side note, I think func (*C.StructType) method() shouldn't be accepted, and I think it's a cmd/cgo issue that it's currently accepted. I'd recommend not relying on that if you can avoid it.

[ianlancetaylor]

@beakbeak Do you know of real code that defines a Go method on a type defined in C?

[beakbeak]

@ianlancetaylor No, sorry. I encountered it in an old personal project of mine. I was a bit surprised to see that it was allowed.

[mdempsky]

@beakbeak Thanks. It certainly isn't intended to work. It's just an unfortunate consequence of how cmd/cgo is implemented, and an oversight in hiding those implementation details from users.

[ianlancetaylor]

Not sure why this is marked for 1.15. Moving to backlog.

[heschi]

Because it depended on go/types changes, but those have landed, at least enough for us, for now.

[stamblerre]

@heschik: Will we be able to close this when Go 1.15 is released, or will there still be further work to do here?

[heschi]

39072 is still a problem, but the fix is mostly or entirely in the stdlib, so I think it'd be reasonable to close this one if you want.

[stamblerre]

Oh ok, well let's keep this open for tracking as long as there are still open issues. Should we milestone #39072 for 1.16?

[heschi]

Done.

[Zincr0]

Here's the current status: if you build gopls at master with tip Go, you should have a pretty good cgo authoring experience. The one bug I know of is autocomplete, which will offer symbols like _cgo_foo instead of C.foo.

One bump, though, is that it won't regenerate the cgo bindings unless you tell it to. To that end, there should be a "regenerate cgo" popup on top of the import "C" line in a file that uses cgo. You'll need to use it when you reference a new identifier from C, or change the magic comment in a way that affects the symbols Go can see.

To get set up, follow these instructions:

$ go get golang.org/dl/gotip
$ gotip download
[...]
Success. You may now run 'gotip'!
$ cd /
$ GO111MODULE=on gotip get golang.org/x/tools/gopls@master
$ gotip version
... devel +f7f9c8f ...
$ gotip version $(which gopls)
... devel +f7f9c8f ...

You don't need to use tip Go to work on your own project, just to build gopls.

Please leave a reaction on this comment if you try it out, and comment with any bugs you find.

i'm seeing the "error" in the import "C" line and the "regenerate cgo definitions" in vscode but nothing happens when i click it, no errors, no nothing at all. Is there a way to force it? a console command or something?

[heschi]

@Zincr0 It would be good to add a progress notification, but right now there's no feedback. The only thing that should happen is that any changes you made in the cgo-using code will be reflected, resulting in errors going away or appearing as appropriate. Are you seeing changes not take effect? Can you be specific about what you're seeing vs. what you expect to see?

[Zincr0]

I'm importing a static library and getting an error in the 'import "C"' line. Clicking regenerate cgo definitions doesn't make the linter error go away, i'm guessing that maybe all of this is for for embedded C code only, and not for static libraries?

[heschi]

The only errors regenerating cgo would fix are compiler errors related to the C "package". Lint errors are a separate issue.

Whatever you're seeing, I'm not sure it's related to this issue. Please file a new issue, particularly including the exact error.

[Zincr0]

Created #40595

[linlinlinlin]

Will this issue also related on not declared by package C issue?

[heschi]

No. If your code compiles at the command line, but you're getting an error from gopls, please file a new issue.

[ultimateshadsform]

I don't get any autocompletion at all.

//go:build cgo

package main

/*
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <linux/fb.h>
#include <sys/mman.h>
#include <sys/ioctl.h>
#include <fcntl.h>
#include <unistd.h>

int OpenFrameBuffer(char * name) {
    return open(name, O_RDWR);
}

static  int GetFixedScreenInformation(int fd,struct fb_fix_screeninfo *finfo)
{
    return ioctl(fd, FBIOGET_FSCREENINFO, finfo);
}

static  int GetVarScreenInformation(int fd,struct fb_var_screeninfo *vinfo) {
    return ioctl(fd, FBIOGET_VSCREENINFO, vinfo);
}
*/
import "C"

import (
    "errors"
    "fmt"
    "image"
    "math"
    "os/exec"
    "time"
    "unsafe"
)

// Framebuffer -
type Framebuffer struct {
    Fd           int
    BitsPerPixel int
    Xres         int
    Yres         int
    Data         []byte
    Xoffset      int
    Yoffset      int
    LineLength   int
    Screensize   int
}

// NewFramebuffer -
func NewFramebuffer() *Framebuffer {
    return &Framebuffer{}
}

func _HideCursor() {
    fmt.Print("\033[?25l")
    exec.Command("kill")
}

func _ShowCursor() {
    fmt.Printf("\033[?25h")
}

// Init -
func (f *Framebuffer) Init(dev string) {
    if len(dev) == 0 {
        dev = "/dev/fb0"
    }
    _HideCursor()
    dev_file := C.CString(dev)
    fd, err := C.OpenFrameBuffer(dev_file)
    C.free(unsafe.Pointer(dev_file))

    if err != nil {
        panic(errors.New("open the framebuffer failed"))
    }

    var finfo C.struct_fb_fix_screeninfo
    if _, err := C.GetFixedScreenInformation(fd, &finfo); err != nil {
        fmt.Println(err)
    }

    var vinfo C.struct_fb_var_screeninfo
    if _, err := C.GetVarScreenInformation(fd, &vinfo); err != nil {
        fmt.Println(err)
    }

    f.Xres = int(vinfo.xres)
    f.Yres = int(vinfo.yres)
    f.BitsPerPixel = int(vinfo.bits_per_pixel)
    f.Xoffset = int(vinfo.xoffset)
    f.Yoffset = int(vinfo.yoffset)
    f.LineLength = int(finfo.line_length)

    f.Screensize = int(finfo.smem_len)

    addr := uintptr(C.mmap(nil, C.size_t(f.Screensize), C.PROT_READ|C.PROT_WRITE, C.MAP_SHARED, fd, 0))

    var sl = struct {
        addr uintptr
        len  int
        cap  int
    }{addr, f.Screensize, f.Screensize}

    f.Data = *(*[]byte)(unsafe.Pointer(&sl))

}

// Release open file
func (f *Framebuffer) Release() {
    C.munmap(unsafe.Pointer(&f.Data[0]), C.size_t(f.Screensize))
    C.close(C.int(f.Fd))
    _ShowCursor()
}

func (f *Framebuffer) SetPixel(x int, y int, r uint32, g uint32, b uint32, a uint32) {
    if x < 0 || x > f.Xres {
        panic(errors.New("x is too big or is negative"))
    }

    if y < 0 || y > f.Yres {
        panic(errors.New("y is too big or is negative"))
    }

    location := (x+f.Xoffset)*(f.BitsPerPixel/8) + (y+f.Yoffset)*f.LineLength

    f.Data[location+3] = byte(a & 0xff)
    f.Data[location+2] = byte(r & 0xff)
    f.Data[location+1] = byte(g & 0xff)
    f.Data[location] = byte(b & 0xff)
}

func (f *Framebuffer) DrawImage(xoffset int, yoffset int, image image.Image) {

    b := image.Bounds()

    for y := 0; y < b.Max.Y; y++ {
        for x := 0; x < b.Max.X; x++ {
            r, g, b, a := image.At(x, y).RGBA()
            f.SetPixel(x+xoffset, y+yoffset, r&0xff, g&0xff, b&0xff, a&0xff)
        }
    }
}

func (f *Framebuffer) DrawData(xoffset int, yoffset int, data []byte, w int, h int) {

    if w > f.Xres {
        panic(errors.New("the width of data must NOT be bigger the Xres of the framebuffer"))
    }

    for y := 0; y < h; y++ {
        if (y+1)*w > len(data) {
            panic(errors.New("the length of image data is too small or w(h) argument is wrong"))
        }

        line_start := (xoffset+f.Xoffset)*(f.BitsPerPixel/8) + (y+yoffset+f.Yoffset)*f.LineLength
        line_end := (xoffset+f.Xoffset)*(f.BitsPerPixel/8) + (y+1+yoffset+f.Yoffset)*f.LineLength - 1

        if line_start+w > line_end {
            panic(errors.New("the lines is too long beyond the framebuffer"))
        }

        data_line := data[y*w*4 : (y+1)*w*4]
        copy(f.Data[line_start:line_start+w*4], data_line)
    }

}

func (f *Framebuffer) Fill(r, g, b, a uint32) {
    for y := 0; y < f.Yres; y++ {
        for x := 0; x < f.Xres; x++ {
            f.SetPixel(x, y, r, g, b, a)
        }
    }
}

func main() {
    fb := NewFramebuffer()
    fb.Init("/dev/fb0")

    // Rotate the rainbow
    rotation := 0.0
    for {
        for y := 0; y < fb.Yres; y++ {
            for x := 0; x < fb.Xres; x++ {
                // Calculate rotated coordinates
                xr := float64(x) + rotation*float64(y)
                yr := float64(y) - rotation*float64(x)

                // Clamp rotated coordinates to framebuffer dimensions
                xr = math.Min(math.Max(xr, 0), float64(fb.Xres-1))
                yr = math.Min(math.Max(yr, 0), float64(fb.Yres-1))

                // Set rotated pixel color
                fb.SetPixel(int(xr), int(yr), uint32(x*255/fb.Xres), uint32(y*255/fb.Yres), uint32((x+y)*255/(fb.Xres+fb.Yres)), 255)
            }
        }

        // Increment rotation angle
        rotation += 0.01

        // Sleep for a smooth animation
        time.Sleep(10 * time.Millisecond)
    }

    fb.Release()
}

Clicking the regenerate cgo button doesn't work. Nothing happens.

It compiles fine but I don't get any autocompletion which is a real bummer.