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: 'go build' of a SWIG only package fails with 'C source files not supported without cgo' #9410

Closed
michael-schaller opened this issue Dec 21, 2014 · 29 comments
Milestone

Comments

@michael-schaller
Copy link
Contributor

What version of Go are you using (go version)?
go version devel +f34964e Sun Dec 21 00:24:39 2014 +0000 linux/amd64

What operating system and processor architecture are you using?
linux/amd64 (Ubuntu 14.04 amd64)

What did you do?
Run 'go build' on this example: https://gist.github.com/michael-schaller/94a2ee39bde248c83742

What did you expect to see?
That 'go build' or 'go test' work as previously under Go 1.4 and earlier.

What did you see instead?
The error message 'C source files not supported without cgo'.

What else?
SWIG version: 3.0.3 (commit 93d58cd3ed1cc2cf0490e95d43eed4e8de62763d; master on 2014-12-21)

@michael-schaller michael-schaller changed the title 'go build' of a SWIG only package fails with 'C source files not supported without cgo' cmd/go: 'go build' of a SWIG only package fails with 'C source files not supported without cgo' Dec 21, 2014
@ianlancetaylor ianlancetaylor added this to the Go1.5 milestone Dec 21, 2014
@slimsag
Copy link

slimsag commented Dec 22, 2014

I think there is an issue for this already somewhere.. but I can't find it.

AFAIK you can throw an empty file like this one into your package directory as a workaround:

package foo

import "C"

@michael-schaller
Copy link
Contributor Author

@slimsag I couldn't find an issue for this bug either. Thanks for adding the missing workaround.

@davecheney
Copy link
Contributor

If the question is "how to stop go considering a directory containing only
C source a Go package" the answer is, a use build tags, b,rename the
directory, a prefix of . or _ will do.

If this was not the question, ignore me.
On 22 Dec 2014 15:54, "Michael Schaller" notifications@github.com wrote:

@slimsag https://github.com/slimsag I couldn't find an issue for this
bug either. Thanks for adding the missing workaround.


Reply to this email directly or view it on GitHub
#9410 (comment).

@michael-schaller
Copy link
Contributor Author

@davecheney Clarification:
This issue is about a package with Go/SWIG code that doesn't use cgo at all. This works under previous Go releases but the Go master gives the error 'C source files not supported without cgo' on a 'go build' run. The temporary workaround is to add a dummy 'import "C"' to bypass this error. See also the example code under: https://gist.github.com/michael-schaller/94a2ee39bde248c83742

@michael-schaller
Copy link
Contributor Author

The commit that caused this regression is: 729847c

@rsc
Russ, I think the code you've removed in the gcToolchain.cc method is still needed for SWIG.

@ianlancetaylor
Copy link
Contributor

It's more complicated than that. The current version of SWIG generates code that is expected to be compiled by 6c, but 6c no longer exists. Simply fixing gcToolchain.cc won't help.

@michael-schaller
Copy link
Contributor Author

I see. So the C compilers have been already removed for Go 1.5. Unfortunately the doc http://golang.org/s/dev.cc doesn't mention what needs to happen to support SWIG again.

Ian, what is the plan for SWIG? Is there a bug to follow other than this bug?

@ianlancetaylor
Copy link
Contributor

I know of three possible approaches to fix SWIG (there is no tracking bug).

The first approach is to change SWIG to generate cgo input rather than doing the same thing as cgo. This is the best approach because it should fix most SWIG issues for all time. It is probably also more complicated.

The second approach is for the go tool to convert the _gc.c file generated by SWIG into Go. This is a mechanical translation, and will not require a new SWIG release. It's very ugly.

The third approach is to change SWIG to add the _gc.c information to the .go file. This is not very satisfactory as the result will not work with versions of Go before 1.4.

@michael-schaller
Copy link
Contributor Author

Hmm....

The second approach doesn't really sound like a viable solution to me. Letting the go tool parse the '_gc.c' file just to generate a Go source file does sound wrong to me as SWIG should generate working wrapper source out of the box.

The third approach might be a viable solution but I'm not sold on the idea. I'm sure that SWIG could be changed to support pre-1.4 and post-1.3 simultaneously. In the worst case a SWIG command line switch like -py3 for Python 3 could switch between the versions. It looks relatively straight-forward to generate a Go source file similarly to the '_gc.c' file as 'runtime.h' and 'cgocall.h' have been replaced with / converted to Go equivalents. Maintaining two code paths in SWIG doesn't sound like a good idea though.

The first approach does sound good to me and relatively straight-forward. If I'm not mistaken then SWIG has to generate a header file for cgo instead of a '_gc.c' file and the '.go' source file needs to make cgo calls (C. calls) instead of using _cgo_runtime_cgocall.

In any case the go tool will need to be changed though.
Ian, did I miss anything? Which Go versions would be covered with the first approach?

@ianlancetaylor
Copy link
Contributor

The point of the second approach is that it works for all versions of Go.

I'm sure that SWIG could be changed to support pre-1.4 and post-1.3 simultaneously.

Using cgo, yes; otherwise, no. I'm really pretty sure I'm right about this. A command line option is painful for users, but it is true that we could have the go tool pass it.

The first approach should work for all versions of Go when SWIG is invoked manually. It would not work for older versions of Go when using the older go tool. The main benefit of this approach is that it would work for all foreseeable versions of Go going forward, unlike the other approaches. Go is not going to break existing cgo programs.

@minux
Copy link
Member

minux commented Jan 6, 2015

I propose this stop-gap solution before we switch swig to generate cgo
files (but that will still require changes to cmd/go, so I don't understand
how that could handle the compatibility with older Go releases.)

We make swig generate two files x_gc.c for older pre 1.5 Go releases,
and x_gc.go for Go 1.5+. They contains the same information, just in
C or Go.

The older Go release don't know about x_gc.go file, so it will continue to
use x_gc.c.

cmd/go from Go 1.5+ explicitly ignore x_gc.c and use x_gc.go. Go 1.5
will require SWIG 3.0.5 or above.

What do you think? As long as people are only using cmd/go to build
swig packages, it should provide compatibility with both pre-1.5 and
post-1.5 Go releases.

If people are using custom makefiles to build swig package, they will
have to adapt to Go 1.5 changes, but they need to do that no matter
what anyway.

@minux
Copy link
Member

minux commented Jan 6, 2015

Based on my proposal, I've hacked a proof-of-concept:
swig: https://github.com/minux/swig/tree/go1.5 (will force update, be aware!)
cmd/go: https://gist.github.com/minux/13f18a1735f51edeaf7e

The patches don't fully work yet, but I believe there is no fundamental
problems with my approach.

(Only one minor issue: because the wrapper variable are declared in
the other Go file, I have to resort to use init function trick to
initialize it
in the new _gc.go file. But I don't think that's a big issue, and even if
it turns out to be a problem, we can make gc optimize this case better)

@michael-schaller
Copy link
Contributor Author

@minux Regarding your init trick:
For the Go 1.5 go tool you could try to add a new build tag (http://golang.org/pkg/go/build/#Context -> BuildTags). You can then restrict to only use one of the two Go files for a build depending on the presence of the new build tag.
Older Go 1.4 and older go tool versions won't have the new build tag and hence they will independently of the SWIG version use the old code path. Go 1.5 and newer go tool versions will require SWIG 3.0.4+ and with the new build tag will always use the new code path.

@minux
Copy link
Member

minux commented Jan 6, 2015

That will be too incompatible with current cmd/go code:
Right now after swig processing, the Go files are directly compiled,
and build tags are not honored. I want to keep it that way.

I don't think init trick is too big a problem. Actually, current cmd/gc
will compile:
var _wrap_fopen = unsafe.Pointer(&_cgo__wrap_fopen)
into executable code in init() anyway (because it needs write barrier.)
And this is the underlying problem of #9411.

@minux
Copy link
Member

minux commented Jan 6, 2015

OK, my POC implementation is working now.

both misc/swig/stdio and misc/swig/callback are working.

Most of the swig's Go examples are working fine, but these 3
examples are currently failing:
constants
enum
funcptr

I will investigate more tomorrow.

@minux
Copy link
Member

minux commented Jan 6, 2015

OK, all these failing tests have one thing in common,
they are calling cgo calls in init(), so it seems we do
need to make the assignment without init()?

Suggestions?

@ianlancetaylor
Copy link
Contributor

Generating both files sounds like an interesting stopgap solution.

Having SWIG generate cgo code can work with the Go tool by having the Go tool check the SWIG version, as indeed it already does. If SWIG is sufficiently new, the go tool can pass an option selecting cgo mode. (If SWIG is not sufficiently new, it won't work anyhow.) This is far less than ideal but I don't see another way to do it. The main advantage of the cgo approach is future proofing, as otherwise we are going to wind up doing this dance with every foreseeable Go release. It was a mistake of mine to not do this in the first place.

Are you running the SWIG testsuite as well as the examples? (make check-go-test-suite).

Instead of using an init function, you can use a real function that initializes everything once, and have every generated function call it.

But in the long run we're going to want to do the cgo approach anyhow.

@slimsag
Copy link

slimsag commented Jan 6, 2015

The cgo approach would allow package maintainers to utilize swig from a
//go:generate comment as well, would it not?

That would be neat.

On Tue, Jan 6, 2015 at 7:41 AM, Ian Lance Taylor notifications@github.com
wrote:

Generating both files sounds like an interesting stopgap solution.

Having SWIG generate cgo code can work with the Go tool by having the Go
tool check the SWIG version, as indeed it already does. If SWIG is
sufficiently new, the go tool can pass an option selecting cgo mode. (If
SWIG is not sufficiently new, it won't work anyhow.) This is far less than
ideal but I don't see another way to do it. The main advantage of the cgo
approach is future proofing, as otherwise we are going to wind up doing
this dance with every foreseeable Go release. It was a mistake of mine to
not do this in the first place.

Are you running the SWIG testsuite as well as the examples? (make
check-go-test-suite).

Instead of using an init function, you can use a real function that
initializes everything once, and have every generated function call it.

But in the long run we're going to want to do the cgo approach anyhow.


Reply to this email directly or view it on GitHub
#9410 (comment).

Follow me on twitter @slimsag https://twitter.com/slimsag.

@minux
Copy link
Member

minux commented Jan 6, 2015

If I use a real function, then I will need to declare that function in main
Go file
so that pre-Go 1.5 will compile because the function is actually defined in
C,
however, that will make it impossible to define that function in the _gc.go
file.

If we will need to add a new switch to swig for the cgo approach, could we
add a -go1.5 and just generate all the code in the main Go file and skip the
_gc.c file.

Running the SWIG Go test suite showed that a lot of C++ tests cases are
failing to link due to missing C++ symbols. The reason is that I didn't
update
the Makefile in Examples/test-suite/go/Makefile.in, why do the swig test
suite and example not sharing the same set of Makefile rules?

@minux
Copy link
Member

minux commented Jan 6, 2015

Now I fixed the Makefile for the test-suite, and most of the tsets
are passing. I think all the remaining failing tests are due to the
running cgo call during init() issue.

@minux
Copy link
Member

minux commented Jan 8, 2015

I found a way around the problem of the need to
add a function declaration in the original Go file
for pre-Go 1.5 and the _gc.c and also the need to
override the same function in the _gc.go file.

This solution used the fact that //go:linkname is
only recognized in Go 1.5 and above, and Go 1.5+
will disregard the _gc.c file and compile the _gc.go:

The idea is that in the main Go file, we use //go:linkname
to rename the declared function swig_init_c to another
name, say swig_init_go. Because this is ignored by
pre Go 1.5, it serves as declaration for the function
implemented in _gc.c. However, in Go 1.5, this renames
the function to a global swig_init_go, and in the _gc.go
file, we similarly rename the actual swig_init function
to a global swig_init_go.

In real swig implementations, we will need to rename
the function to something that contains the package
import path, so that the init function from different packages
won't collide.

A simple demo program:
// a.go, this simulate the main swig generated Go file
package main

import (
"sync"
_ "unsafe"
)

var _swig_once sync.Once

var A uintptr // to be initialized in swig_init

//go:linkname swig_init_c swig_init_go
func swig_init_c()

func init() {
// simulate that swig needs to initialize
// some variables during init
_swig_once.Do(swig_init_c)
}

func main() {
println("main")
}

// b_gc.c, file compiled by pre 1.5 go tool
// +build !go1.5

#include "runtime.h"
void ·swig_init_c(void) {
// init ·A here
runtime·printf("swig_init in C\n");
}

// b_gc.go, file compiled by Go 1.5+ go tool
// +build go1.5

package main

import _ "unsafe"

//go:linkname swig_init swig_init_go
func swig_init() {
// init A here
println("swig_init in Go")
}

and we need a dummy c.s file to make sure go tool
doesn't think the package is completely in Go.

Then both go build -tags go1.5 and go133 build will work.
(Go 1.4 won't work because it doesn't allow C source files
without cgo, but when the files are generated by swig, there
is no such restriction.)

@ianlancetaylor, what do you think about this solution?
If you think this is the right way to go, I will update my
swig fork and propose a pull request to swig.

@minux minux self-assigned this Jan 8, 2015
@ianlancetaylor
Copy link
Contributor

That is clever. That seems like a good temporary workaround. Please go ahead.

You mention a dummy c.s file, but I assume that is only for the unusual case of running SWIG yourself (as the SWIG testsuite does). In the normal case SWIG will be run by the go tool, and the build tag and the dummy file are unnecessary.

@minux
Copy link
Member

minux commented Jan 8, 2015

Yes. the dummy .s file and build tag is only needed to
build my demo program, and they are not necessary for
actual swig generated files.

@minux
Copy link
Member

minux commented Jan 8, 2015

Done, all examples and tests have passed for Go 1.5 (tip).
I've tested misc/swig/{callback,stdio} with both Go 1.3.3 and
Go 1.4, and they both work. It's fully backward compatible
change.

(Ian, do you mind me making the misc/swig/* tests a little more
comprehensive by adding some code that causes swig to issue
cgo calls inside init()?)

swig: https://github.com/minux/swig/tree/go1.5 (beware force updates)
cmd/go: https://gist.github.com/minux/13f18a1735f51edeaf7e

One question: I'm naming the global swig_init_go_PACKAGENAME,
but what if the user uses two packages with the same package
name but different import paths? I guess cmd/go should always
provide -fgo-pkgpath so that we can use pkgpath here?

Please provide review comments, thanks!

@ianlancetaylor
Copy link
Contributor

The -go-pkgpath option is really a gccgo concept, I'm worried that it might be confusing to use it with gc.

Since the name can be anything, and doesn't have to be consistent, I would suggest sticking on the mangled absolute path of the swig file. If you can figure out how to get that.

@minux
Copy link
Member

minux commented Jan 8, 2015

While it's easy to get the input file name (through Getattr(n, "infile")),
however, getting absolute path requires platform dependent code,
and it depends on the file location, so it's not as stable as hash of
the swig input file.
How about using a hash of the swig input files just like we did for cgo?

Is there any predefined hash algorithms in SWIG? MD5 is probably
too big to import.

My current code uses SipHash-2-4, with hardcoded key. It adds
about 50 lines. But it requires uint64_t and stdint.h. Is that OK?

The changes are ready. Fully tested on Go 1.1.2, 1.2.1, 1.3.3, 1.4
and tip.
Please review (https://golang.org/cl/2540 and https://github.com/minux/swig/tree/go1.5).
Thanks.

@ianlancetaylor
Copy link
Contributor

I don't know if it's a good idea to introduce stdint.h into SWIG, nothing else seems to use it.

SWIG actually uses pull requests. Can you send a pull request to the main SWIG repo?

@minux
Copy link
Member

minux commented Jan 8, 2015

Sent swig/swig#301.

The only concern for stdint.h is that MSVC used to not provide it, but
2012 and 2013 do provide it. (And I think SWIG is mostly built with
mingw/msys and cygwin on Windows anyway)

@rsc rsc removed the repo-main label Apr 14, 2015
@michael-schaller
Copy link
Contributor Author

This bug has been fixed with commit swig/swig@9ad497c.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants