Skip to content
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

cmd/go: swig files result in cgo-processed files with line directives referencing build temp dirs #36129

Open
jackielii opened this issue Dec 13, 2019 · 9 comments

Comments

@jackielii
Copy link

@jackielii jackielii commented Dec 13, 2019

Go build supports swig file, as a simple example:

golang.org/x/tools/gopls 0.2.2
    golang.org/x/tools/gopls@v0.2.2 h1:ujGisyytgY1VGcmd66wIJ9+wVAfmodXj6daHM43HRXk=

simple.zip

gopls check main.go works fine, however go to definition doesn't work:

vim-go: no file for file:///tmp/go-build900005810/b001/_main_swig.go in package simple

This does seem to be a combination of how go build works, however, from the users perspective, if go tools generates the go files, then gopls should handle it.

@gopherbot gopherbot added this to the Unreleased milestone Dec 13, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 13, 2019

Thank you for filing a gopls issue! Please take a look at the Troubleshooting guide, and make sure that you have provided all of the relevant information here.

@jackielii jackielii changed the title x/tools/gopls: no swig support x/tools/gopls: better swig support Dec 13, 2019
@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Dec 13, 2019

This is substantially a dupe of #35721, but with the extra twist that the //line directives in the generated code point to temporary directories that don't exist once the build is finished.

@jayconrod @bcmills I think the line directives should point into the build cache, just as they do for cgo?

@jayconrod

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented Dec 13, 2019

Not really familiar with swig, but cgo //line directives point to source directories or the module cache, not the build cache. I think that's what you meant though.

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Dec 13, 2019

Sorry. I did mean the build cache, I was just also confused.

cgo processing generates a .go file in the build cache that contains //line directives that point to the original source file, as you said.

swig generates a cgo file in a build temp directory (/tmp/go-build900005810/b001/_main_swig.go above) that gets cgo processed into a .go file which points to the "original" cgo file, which is both not user-authored and not there any more.

So: is this a Go bug, in that we're not saving a useful file in the build cache? Or is it a SWIG bug, in that the _main_swig.go file didn't have its own line directive pointing back to the .swig? (I don't know if cgo can propagate line directives. I hope so.)

@ianlancetaylor

@jayconrod

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented Dec 13, 2019

Huh, that is confusing.

We shouldn't cache or compile anything with //line directives that point to the build cache or temporary build directories. So there's definitely a bug here.

I don't know enough about swig or how the go command invokes it to say where the bug is though.

@heschik heschik modified the milestones: Unreleased, Go1.15 Dec 13, 2019
@heschik heschik changed the title x/tools/gopls: better swig support cmd/go: swig files result in cgo-processed files with line directives referencing build temp dirs Dec 13, 2019
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 14, 2019

Essentially nothing from the .swig file will appear in the generated Go file, so there is no point to SWIG adding a //line directive there.

If we want people to be able to look at the definitions of Go types in files generated by SWIG, then I guess we have to save those files somewhere, though I don't know where.

I have to say also that these generated files are highly cryptic and contain no information usable to anyone who isn't deeply familiar with exactly how SWIG generates Go files. So even if we do make the Go files available somewhere I don't think that that will actually help anybody. Here is the generated SWIG file for the simple.zip example linked in the original issue comment.

/* ----------------------------------------------------------------------------
 * This file was automatically generated by SWIG (http://www.swig.org).
 * Version 4.0.2
 *
 * This file is not intended to be easily readable and contains a number of
 * coding conventions designed to improve portability and efficiency. Do not make
 * changes to this file unless you know what you are doing--modify the SWIG
 * interface file instead.
 * ----------------------------------------------------------------------------- */

// source: main.swig

package main

/*
#define intgo swig_intgo
typedef void *swig_voidp;

#include <stdint.h>


typedef long long intgo;
typedef unsigned long long uintgo;



typedef struct { char *p; intgo n; } _gostring_;
typedef struct { void* array; intgo len; intgo cap; } _goslice_;


extern void _wrap_Swig_free_main_f6945e5ac43a4dda(uintptr_t arg1);
extern uintptr_t _wrap_Swig_malloc_main_f6945e5ac43a4dda(swig_intgo arg1);
extern swig_intgo _wrap_gcd_main_f6945e5ac43a4dda(swig_intgo arg1, swig_intgo arg2);
extern void _wrap_Foo_set_main_f6945e5ac43a4dda(double arg1);
extern double _wrap_Foo_get_main_f6945e5ac43a4dda(void);
#undef intgo
*/
import "C"

import "unsafe"
import _ "runtime/cgo"
import "sync"


type _ unsafe.Pointer



var Swig_escape_always_false bool
var Swig_escape_val interface{}


type _swig_fnptr *byte
type _swig_memberptr *byte


type _ sync.Mutex

func Swig_free(arg1 uintptr) {
	_swig_i_0 := arg1
	C._wrap_Swig_free_main_f6945e5ac43a4dda(C.uintptr_t(_swig_i_0))
}

func Swig_malloc(arg1 int) (_swig_ret uintptr) {
	var swig_r uintptr
	_swig_i_0 := arg1
	swig_r = (uintptr)(C._wrap_Swig_malloc_main_f6945e5ac43a4dda(C.swig_intgo(_swig_i_0)))
	return swig_r
}

func Gcd(arg1 int, arg2 int) (_swig_ret int) {
	var swig_r int
	_swig_i_0 := arg1
	_swig_i_1 := arg2
	swig_r = (int)(C._wrap_gcd_main_f6945e5ac43a4dda(C.swig_intgo(_swig_i_0), C.swig_intgo(_swig_i_1)))
	return swig_r
}

func SetFoo(arg1 float64) {
	_swig_i_0 := arg1
	C._wrap_Foo_set_main_f6945e5ac43a4dda(C.double(_swig_i_0))
}

func GetFoo() (_swig_ret float64) {
	var swig_r float64
	swig_r = (float64)(C._wrap_Foo_get_main_f6945e5ac43a4dda())
	return swig_r
}

Note that this works with a C file that is also generated. That file has even more cryptic boilerplate, but, for example, here is the generated function called by Gcd:

intgo _wrap_gcd_main_f6945e5ac43a4dda(intgo _swig_go_0, intgo _swig_go_1) {
  int arg1 ;
  int arg2 ;
  int result;
  intgo _swig_go_result;
  
  arg1 = (int)_swig_go_0; 
  arg2 = (int)_swig_go_1; 
  
  result = (int)gcd(arg1,arg2);
  _swig_go_result = result; 
  return _swig_go_result;
}
@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Dec 14, 2019

I agree that they're not likely to be terribly useful, but anything that's trying to analyze cgo source files is going to want to find the authored file. So either we need to have that, or we have to add fallback code to gopls and at least one analysis. I'd prefer to iron out the corner case if possible, especially because we're already planning not to support cgo packages until 1.15 in #35721.

@jackielii

This comment has been minimized.

Copy link
Author

@jackielii jackielii commented Dec 16, 2019

Thanks all for replying. My initial intentions are to get the generated Go function signatures out. I'm a beginner to get usage out from the go build tool. From swig documentation, it's not obvious exactly what's generated.

Obviously I could just manually run swig to generate the code and check the signature. Or to use the -work to keep the temporary files. However, I still think that from gopls's point of view, if the go build do something obscure, gopls should understand it.

Hope this make sense

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Dec 16, 2019

@jackielii Understood, but I'm afraid that #35721 is a blocker here and I'm not expecting much progress on that until later this year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.