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/cgo: searches in current directory for includes made with <angle brackets> #41059

Closed
KJTsanaktsidis opened this issue Aug 27, 2020 · 6 comments
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@KJTsanaktsidis
Copy link
Contributor

KJTsanaktsidis commented Aug 27, 2020

What version of Go are you using (go version)?

$ go version
go version go1.15 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/ktsanaktsidis/Library/Caches/go-build"
GOENV="/Users/ktsanaktsidis/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/ktsanaktsidis/Code/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/ktsanaktsidis/Code/go"
GOPRIVATE=""
GOPROXY="https://ktsanaktsidis:$REDACTED$@zdrepo.jfrog.io/zdrepo/api/go/zen-go"
GOROOT="/usr/local/Cellar/go/1.15/libexec"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.15/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/zs/22t0g49n2mz1vpv0fgkptv1c0000gp/T/go-build648868978=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

When importing a header file in cgo using the angle-bracket syntax (#include <the/file.h>), cgo looks in the code's directory for a file called the/file.h, and if it finds it there, uses that file in preference to any other version of the file that might be found with a -I CFLAGS or the system header directory.

See this github repo for a reproduction of the issue: https://github.com/KJTsanaktsidis/cgo-header-repro

This is a problem in particular when using tags to allow users of a go module to switch between a bundled or system-installed version of a C library. See https://github.com/confluentinc/confluent-kafka-go for example, which, ordinarily, has build_glibc_linux.go selected by the default set of tags:

// +build !dynamic
// +build !musl

// This file was auto-generated by librdkafka/bundle-import.sh, DO NOT EDIT.

package kafka

// #cgo CFLAGS: -I${SRCDIR}
// #cgo LDFLAGS: ${SRCDIR}/librdkafka/librdkafka_glibc_linux.a  -lm -ldl -lpthread -lrt
import "C"

// LibrdkafkaLinkInfo explains how librdkafka was linked to the Go client
const LibrdkafkaLinkInfo = "static glibc_linux from librdkafka-static-bundle-v1.4.2.tgz"

It puts ${SRCDIR} in the include path, so that #include <librdkafka/rdkafka.h> includes in other files find the bundled librdkafka/kafka.h.

However, if the library is built with tags=dynamic, build_dynamic.go is selected instead:

// +build dynamic

package kafka

// #cgo pkg-config: rdkafka
import "C"

// LibrdkafkaLinkInfo explains how librdkafka was linked to the Go client
const LibrdkafkaLinkInfo = "dynamically linked to librdkafka"

Now, since -I${SRCDIR} is no longer in the include path, we expect that #include <librdkafka/kafka.h> will find the system version of the header file located by pkgconfig. However, instead, it still finds the bundled header file, which is obviously a problem if they're different!

What did you expect to see?

I expect that header includes are looked for in the SRCDIR only if

  • The include is made with quotes (i.e. #include "the/file.h"), or
  • -I${SRCDIR} is explicitly added to CFLAGS

This is the behaviour of gcc (I checked in my reproduction)

What did you see instead?

It seems like if an include is made with angle brackets, the header is looked for in SRCDIR even if SRCDIR isn't explicitly added to the include path.

@ianlancetaylor ianlancetaylor changed the title cgo searches in current directory for includes made with <angle brackets> cmd/cgo: searches in current directory for includes made with <angle brackets> Aug 27, 2020
@ianlancetaylor
Copy link
Contributor

This is happening because we add -I . to the C compiler argument list, as explained at https://golang.org/src/cmd/cgo/util.go#L40. I'm not sure offhand how to fix it.

@ianlancetaylor ianlancetaylor added help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 27, 2020
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Aug 27, 2020
@KJTsanaktsidis
Copy link
Contributor Author

Oh. Well, for GCC, I think this could be fixed by using -iquote . instead of -I .

Directories specified with -iquote apply only to the quote form of the directive, #include "file".

https://gcc.gnu.org/onlinedocs/gcc/Directory-Options.html

Clang also seems to support -iquote, at a glance. Not sure what other C compilers cgo is supposed to be able to work with though.

@KJTsanaktsidis
Copy link
Contributor Author

@ianlancetaylor I did some more research into this issue and I think it should be closed, because it turns out to be a breaking change (and not just theoretically).

I started by putting together a patch that changed -I . to -iquote . in a couple of places:

% git diff
diff --git a/src/cmd/cgo/util.go b/src/cmd/cgo/util.go
index 921306b7aa..0efd26ff68 100644
--- a/src/cmd/cgo/util.go
+++ b/src/cmd/cgo/util.go
@@ -45,7 +45,7 @@ func run(stdin []byte, argv []string) (stdout, stderr []byte, ok bool) {
                // We've also run into compilers that reject "-I." but allow "-I", ".",
                // so be sure to use two arguments.
                // This matters mainly for people invoking cgo -godefs by hand.
-               new = append(new, "-I", ".")
+               new = append(new, "-iquote", ".")
 
                // Finish argument list with path to C file.
                new = append(new, name+".c")
diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go
index d975c36306..a6bb298781 100644
--- a/src/cmd/go/internal/work/exec.go
+++ b/src/cmd/go/internal/work/exec.go
@@ -2363,7 +2363,7 @@ func (b *Builder) compilerExe(envValue string, def string) []string {
 func (b *Builder) compilerCmd(compiler []string, incdir, workdir string) []string {
        // NOTE: env.go's mkEnv knows that the first three
        // strings returned are "gcc", "-I", incdir (and cuts them off).
-       a := []string{compiler[0], "-I", incdir}
+       a := []string{compiler[0], "-iquote", incdir}
        a = append(a, compiler[1:]...)
 
        // Definitely want -fPIC but on Windows gcc complains

That makes my test case work the way I expected (once I'd worked out I needed to invoke go clean -cache when debugging cgo like this - trick for new players 😂 )

Then, I ran the test suite against this. There was one failure, from TestCgo in go/internal/srcimporter. The failure was caused by this line in misc/cgo/test/test.go

// issue 4339
// We've historically permitted #include <>, so test it here.  Issue 29333.
#include <issue4339.h>

Since I'd changed the -I flags to -iquote, this obviously stopped working since the import is looking for a file in the current directory, without quotes.

I looked at these issues - #4339 just happens to be the header that's being included, but #29333 ("cmd/cgo: can not include header file in package directory with Go 1.12beta1") is relevant. Someone noticed that the cgo build of the module crawshaw.io/sqlite had stopped working, because someone had accidently stopped the -I . flag getting appended on the call to run in eeb8aeb, and you personally fixed this in d9e2ba4 and added this explicit test for this behaviour.

tl;dr - being able to include files from the current directory with #include<> is explicitly tested for, and something that at least somebody in the wild is doing, and it would cause some breakage to fix this even though on its own this behaviour seems pretty wrong to me (and stops use-cases like the bundled/dynamic switch like I mentioned in my original report).

Can you see a viable path forward here? At the very least, I figure there should be a warning in the cgo docs about the include path working this way - I'm happy to send a CL to add it if you think that's the way we should go.

@ianlancetaylor
Copy link
Contributor

Thanks for doing the investigation. It does seem that we aren't going to be able to change this.

I agree that this should be documented somewhere in cmd/cgo/doc.go.

Perhaps you can solve your original problem using a #cgo CFLAGS: -DUSE_LOCAL_HEADER, and rename the header file that everything includes, and do something like

#ifdef USE_LOCAL_HEADERS
    local definitions
#else
#include <real-header-name.h>
#endif

@ianlancetaylor ianlancetaylor added Documentation NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 31, 2020
@gopherbot
Copy link

Change https://golang.org/cl/251758 mentions this issue: cmd/cgo: document #include <> search path behaviour

@KJTsanaktsidis
Copy link
Contributor Author

Thanks for the suggestion - putting a define in the CFLAGS sounds like a good option for fixing my original problem.

I've sent a patch to document what's going on ☝️

@golang golang locked and limited conversation to collaborators Sep 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants