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: list -compiled fails to populate CompiledGoFiles when the resulting package cannot be linked #34229

Open
dominikh opened this issue Sep 11, 2019 · 4 comments

Comments

@dominikh
Copy link
Member

commented Sep 11, 2019

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

$ go version
go version go1.13 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

Linux, amd64

What did you do?

Create a package with the following contents:

package pkg

// #cgo LDFLAGS: -ldoesnotexist
import "C"

Run go list -json -x -compiled on the package

What did you expect to see?

No errors, no attempted linking of object files, complete JSON output (meaning CompiledGoFiles is populated).

What did you see instead?

$ go list -e -json -compiled -x
WORK=/tmp/go-build309865873
mkdir -p $WORK/b001/
cd /home/dominikh/prj/src/sandbox/bar
CGO_LDFLAGS='"-g" "-O2" "-ldoesnotexist"' /usr/lib/go/pkg/tool/linux_amd64/cgo -objdir $WORK/b001/ -importpath sandbox/bar -- -I $WORK/b001/ -g -O2 ./bar.go
cd $WORK
gcc -fno-caret-diagnostics -c -x c - -o /dev/null || true
gcc -Qunused-arguments -c -x c - -o /dev/null || true
gcc -fdebug-prefix-map=a=b -c -x c - -o /dev/null || true
gcc -gno-record-gcc-switches -c -x c - -o /dev/null || true
cd $WORK/b001
TERM='dumb' gcc -I /home/dominikh/prj/src/sandbox/bar -fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=$WORK/b001=/tmp/go-build -gno-record-gcc-switches -I ./ -g -O2 -o ./_x001.o -c _cgo_export.c
TERM='dumb' gcc -I /home/dominikh/prj/src/sandbox/bar -fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=$WORK/b001=/tmp/go-build -gno-record-gcc-switches -I ./ -g -O2 -o ./_x002.o -c bar.cgo2.c
TERM='dumb' gcc -I /home/dominikh/prj/src/sandbox/bar -fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=$WORK/b001=/tmp/go-build -gno-record-gcc-switches -I ./ -g -O2 -o ./_cgo_main.o -c _cgo_main.c
cd /home/dominikh/prj/src/sandbox/bar
TERM='dumb' gcc -I . -fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=$WORK/b001=/tmp/go-build -gno-record-gcc-switches -o $WORK/b001/_cgo_.o $WORK/b001/_cgo_main.o $WORK/b001/_x001.o $WORK/b001/_x002.o -g -O2 -ldoesnotexist
# sandbox/bar
/usr/bin/ld: cannot find -ldoesnotexist
collect2: error: ld returned 1 exit status
{
	"Dir": "/home/dominikh/prj/src/sandbox/bar",
	"ImportPath": "sandbox/bar",
	"Name": "pkg",
	"Target": "/home/dominikh/prj/pkg/linux_amd64/sandbox/bar.a",
	"Root": "/home/dominikh/prj/",
	"Match": [
		"."
	],
	"Stale": true,
	"StaleReason": "build ID mismatch",
	"CgoFiles": [
		"bar.go"
	],
	"CgoLDFLAGS": [
		"-ldoesnotexist"
	],
	"Imports": [
		"C",
		"unsafe",
		"runtime/cgo",
		"syscall"
	],
	"Deps": [
		"errors",
		"internal/bytealg",
		"internal/cpu",
		"internal/oserror",
		"internal/race",
		"internal/reflectlite",
		"runtime",
		"runtime/cgo",
		"runtime/internal/atomic",
		"runtime/internal/math",
		"runtime/internal/sys",
		"sync",
		"sync/atomic",
		"syscall",
		"unsafe"
	]
}

That is, Go actually attempts to build the package and fails due to the missing library we're trying to link against. It also doesn't populate CompiledGoFiles, even though cgo preprocessing succeeded.

As I understand it, this is trying to do more work than is necessary. It also unnecessarily fails.

The -compiled flag is documented as follows:

The -compiled flag causes list to set CompiledGoFiles to the Go source
files presented to the compiler. Typically this means that it repeats
the files listed in GoFiles and then also adds the Go code generated
by processing CgoFiles and SwigFiles. The Imports list contains the
union of all imports from both GoFiles and CompiledGoFiles.

It does not imply actual compilation. Also note that we're not passing the -exported flag. Furthermore, in order to do cgo processing, we do not need the linker. For that, looking at headers suffices, and go tool cgo foo.go does succeed.

After a cursory look at cmd/go/internal/work/exec.go, (*Builder).build, it seems that if we require CompiledGoFiles, and they can't be found in the cache, we execute the full cgo pipeline used for building. In theory, it should be possible to optimize this.

The current behavior is problematic for two reasons, from the point of view of tools that analyze Go code:

  1. it is slower than necessary. Analyzing a large cgo-using program will be significantly faster if we can skip the linking phase.
  2. it breaks certain setups of running static analysis. For example, a test environment may have the necessary headers available, but no object files for the libraries. We should be able to do cgo preprocessing on the code, and thus be able to do our static analysis. Requiring the linker to run successfully prevents this from happening.

/cc @matloob @ianthehat @bcmills

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

No errors,

That is #26755.

no attempted linking of object files,

That's a subset of #29666, I think? But we can leave this issue open as a specific instance to address.

(CC @jayconrod)

@bcmills bcmills added the ToolSpeed label Sep 12, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

We should certainly prioritize a fix for #26755, which is what breaks static analysis.

Let's make this issue about the “slower than necessary” part — I agree that it would be nice to avoid the unnecessary work, but I don't think that's likely to happen for 1.14.

@bcmills bcmills added this to the Unplanned milestone Sep 12, 2019

@bcmills bcmills added the help wanted label Sep 12, 2019

@dominikh

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

These issues are certainly related, but even if both #26755 and #29666 were fixed, this issue wouldn't be fixed.

#26755 is about -e not being honored in all cases, which means go list will exit non-zero for packages that fail to compile. However, when that issue gets fixed, its only effect on this issue is that the linker error will be moved into the per-package structure. It would still fail to populate CompiledGoFiles because go list would still try to compile and link.

#29666 is related only insofar as it, too, wants to improve the performance of go list. However, #29666 is about only computing those fields that a template actually uses, while this issue is about doing more work than is necessary to compute a field that is being used.

I regret bringing up performance in the first place, because this issue is also one of correctness, in its own right. Currently, to compute CompiledGoFiles, go list invokes the full cgo pipeline, including compilation and linking. This does not work on systems that don't have the necessary libraries for linking available. And strictly speaking, computation of CompiledGoFiles shouldn't need libraries. Computing it should only require the cgo preprocessing step, i.e. the processing of headers and rewriting of Go code. In other words, we fail to populate CompiledGoFiles even when we should be able to populate it.

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

Ah, I see. Will update and retag accordingly.

@bcmills bcmills changed the title cmd/go: list -compiled does too much work when using cgo cmd/go: list -compiled fails to populate CompiledGoFiles when the resulting package cannot be linked Sep 12, 2019

@bcmills bcmills removed the ToolSpeed label Sep 12, 2019

@bcmills bcmills modified the milestones: Unplanned, Go1.14 Sep 12, 2019

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