cmd/cgo: does not replace references to paths in GOROOT/ with $GOROOT #21825

orivej · 2017-09-10T06:52:14Z

Normally Go compiles its own sources such that debug info pathnames look like $GOROOT/src/package/path.go rather than /absolute/path/to/go/src/package/path.go. However, this does not happen when compiling sources with cgo. In particular src/plugin/plugin_dlopen.go produces this temporary file plugin_dlopen.cgo1.go:

…
//line /tmp/nds-install-go_1_9/share/go/src/plugin/plugin_dlopen.go:43
func lastIndexByte(s string, c byte) int {
…

and after unpacking the resulting plugin.a (ar x plugin.a) and disassemly (go tool objdump _go_.o) it can be seen that pure Go sources are refered through $GOROOT, unlike cgo sources:

…
TEXT %22%22.Open(SB) gofile..$GOROOT/src/plugin/plugin.go
…
TEXT %22%22.lastIndexByte(SB) gofile../tmp/nds-install-go_1_9/share/go/src/plugin/plugin_dlopen.go
…

I would like the second line to be:

TEXT %22%22.lastIndexByte(SB) gofile..$GOROOT/src/plugin/plugin_dlopen.go

Here is a possible fix that replaces absolute path names in *.cgo1.go:

diff --git a/src/go/printer/printer.go b/src/go/printer/printer.go
index dbb4bbd90c..16a0b84fa5 100644
--- a/src/go/printer/printer.go
+++ b/src/go/printer/printer.go
@@ -11,6 +11,7 @@ import (
 	"go/token"
 	"io"
 	"os"
+	"runtime"
 	"strconv"
 	"strings"
 	"text/tabwriter"
@@ -207,7 +208,8 @@ func (p *printer) lineFor(pos token.Pos) int {
 func (p *printer) writeLineDirective(pos token.Position) {
 	if pos.IsValid() && (p.out.Line != pos.Line || p.out.Filename != pos.Filename) {
 		p.output = append(p.output, tabwriter.Escape) // protect '\n' in //line from tabwriter interpretation
-		p.output = append(p.output, fmt.Sprintf("//line %s:%d\n", pos.Filename, pos.Line)...)
+		filename := strings.Replace(pos.Filename, runtime.GOROOT(), "$GOROOT", -1)
+		p.output = append(p.output, fmt.Sprintf("//line %s:%d\n", filename, pos.Line)...)
 		p.output = append(p.output, tabwriter.Escape)
 		// p.out must match the //line directive
 		p.out.Filename = pos.Filename

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

go1.9

Does this issue reproduce with the latest release?

yes

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

NixOS Linux amd64

What did you do?

I tried to build journalbeat that uses the plugin package with Go 1.9. It failed Nix check that executables should not refer to Go root via an absolute pathname.

The text was updated successfully, but these errors were encountered:

odeke-em · 2017-09-10T07:34:15Z

Thank you for the report @orivej and for the proposed fix.

So we can't yet review patches on Github, but we do on Gerrit. If you are interested in submitting one, please take a look at https://golang.org/doc/contribute.html just in case you haven't yet.

Otherwise I'll also tag the cgo crew @ianlancetaylor @hirochachacha who might be interested in this issue.

ianlancetaylor · 2017-09-10T09:42:47Z

Changing go/printer is not going to be the right approach. go/printer is a general purpose package and it shouldn't be in the business of modifying file names. The place to change this would be cmd/cgo or cmd/go.

Also I think @crawshaw has been looking at something in this area.

orivej · 2017-09-10T12:45:59Z

Somehow Go 1.8 was not affected: even though its cgo files were the same, object files referred to $GOROOT/src/plugin/plugin_dlopen.go. This bug was introduced in 4808fc4.

CC @griesemer

ianlancetaylor · 2017-09-10T12:48:23Z

Since it worked in 1.8, tentatively setting milestone to 1.9.1. We'll see how invasive the fix is.

crawshaw · 2017-09-10T12:55:11Z

I experimented with a fix for this as part of #21373. It's easy enough not to use absolute paths, but I assume @griesemer introduced them for a reason, so I'll leave this bug to him.

odeke-em · 2017-09-10T15:31:19Z

@crawshaw, just in case 1.9.1 has a tight shipping deadline, @griesemer is out of office for the perhaps the next 6 days, at least as per his message on this review https://go-review.googlesource.com/c/go/+/59450#message-84f433cbd5a65c13bc58e251331687a87d01748f.

orivej · 2017-09-13T19:54:53Z

FWIW I believe that @griesemer considered $GOROOT/ paths absolute too: see this function. He must have missed application of this logic to pathnames read from //line comments.

crawshaw · 2017-09-13T22:13:45Z

Thanks @orivej that makes the problem in cmd/compile clear.

The Go binary distribution is built in /tmp/workdir/go. GOROOT_FINAL is set to /usr/local/go. This is the string that cmd/dist writes into cmd/internal/objabi/zbootstrap.go. This string, /usr/local/go is what AbsFile uses to replace the GOROOT with the string '$GOROOT'. But because the .a files we ship are built in /tmp/workdir/go, the string replace fails.

I believe the most robust fix is to have cmd/go pass its computed GOROOT to cmd/compile as an environment variable (if it's not already set). That will mean cmd/compile does the right thing when the initial .a files are set, and also do the right thing when the entire GOROOT is moved later to an arbitrary directory.

That will resolve the release blocker part of this issue, though you may want to keep it around to look at changing the cmd/cgo program to not write absolute paths.

orivej · 2017-09-13T22:28:31Z

That will resolve the release blocker part of this issue, though you may want to keep it around to look at changing the cmd/cgo program to not write absolute paths.

My initial patch was only for demonstration. The substitution of $GOROOT is logical either before or after the generation of *cgo*.go files with //line comments, and I don't mind if Go continues to prefer after.

orivej · 2017-09-13T22:44:54Z

This does not explain why substitution worked before 4808fc4 : it does not seem to have used the environment to pass the value of GOROOT.

crawshaw · 2017-09-13T22:47:46Z

Turns out cmd/go is already passing GOROOT into cmd/compile, so the value of GOROOT used in AbsFile is correct. So somehow the path coming from a //line directive is bypassing that function's logic.

crawshaw · 2017-09-13T22:56:34Z

Oh I see, I was misreading some code.

So bexport.go uses AbsFilename, which returns whatever string is inside PosBase. That comes from the //line directive, which is an unmangled absolute path, which is why we don't see GOROOT.

For positions not coming from a //line, those are built in tokenizer.go and use objabi.AbsFile to replace the GOROOT with '$GOROOT'.

gopherbot · 2017-09-13T23:23:00Z

Change https://golang.org/cl/63693 mentions this issue: cmd/compile: replace GOROOT in //line directives

orivej · 2017-09-15T17:35:42Z

Thank you for fixing this! I hope it will be shipped with 1.9.1.

crawshaw · 2017-09-22T16:48:22Z

Reopening for 1.9.1 cherry-pick.

rsc · 2017-10-13T19:04:10Z

CL 63693 OK for Go 1.9.2.

gopherbot · 2017-10-15T01:48:05Z

Change https://golang.org/cl/70975 mentions this issue: [release-branch.go1.9] cmd/compile: replace GOROOT in //line directives

The compiler replaces any path of the form /path/to/goroot/src/net/port.go with GOROOT/src/net/port.go so that the same object file is produced if the GOROOT is moved. It was skipping this transformation for any absolute path into the GOROOT that came from //line directives, such as those generated by cmd/cgo. Fixes #21373 Fixes #21720 Fixes #21825 Change-Id: I2784c701b4391cfb92e23efbcb091a84957d61dd Reviewed-on: https://go-review.googlesource.com/63693 Run-TryBot: David Crawshaw <crawshaw@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com> Reviewed-on: https://go-review.googlesource.com/70975 Run-TryBot: Russ Cox <rsc@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>

rsc · 2017-10-26T21:09:13Z

go1.9.2 has been packaged and includes:

CL 63693

The release is posted at golang.org/dl.

— golang.org/x/build/cmd/releasebot, Oct 26 21:09:13 UTC

orivej mentioned this issue Sep 10, 2017

go: buildGoPackage: 1.8 -> 1.9 NixOS/nixpkgs#29173

Merged

8 tasks

ianlancetaylor changed the title ~~cmd/cgo does not replace references to paths in GOROOT/ with $GOROOT~~ cmd/cgo: does not replace references to paths in GOROOT/ with $GOROOT Sep 10, 2017

ianlancetaylor added this to the Go1.10 milestone Sep 10, 2017

ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 10, 2017

ianlancetaylor assigned griesemer Sep 10, 2017

ianlancetaylor added the release-blocker label Sep 10, 2017

ianlancetaylor modified the milestones: Go1.9.1, Go1.10 Sep 10, 2017

orivej referenced this issue in triton/triton Sep 14, 2017

build-go-package: temporarily disable buildDirCheck for go 1.9

6e0136b

gopherbot closed this as completed in 27e80f7 Sep 15, 2017

orivej mentioned this issue Sep 17, 2017

go: fix tests and impurity NixOS/nixpkgs#29489

Merged

8 tasks

crawshaw reopened this Sep 22, 2017

rsc removed this from the Go1.9.1 milestone Oct 4, 2017

rsc added this to the Go1.9.2 milestone Oct 4, 2017

crawshaw mentioned this issue Oct 10, 2017

plugin: package can't open plugin built with same Go runtime #21373

Closed

rsc added the CherryPickApproved Used during the release process for point releases label Oct 14, 2017

rsc mentioned this issue Oct 15, 2017

all: Go 1.9.2 release status #22277

Closed

rsc closed this as completed Oct 26, 2017

golang locked and limited conversation to collaborators Oct 26, 2018

gopherbot added the FrozenDueToAge label Oct 26, 2018

rsc unassigned griesemer Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/cgo: does not replace references to paths in GOROOT/ with $GOROOT #21825

cmd/cgo: does not replace references to paths in GOROOT/ with $GOROOT #21825

orivej commented Sep 10, 2017

odeke-em commented Sep 10, 2017

ianlancetaylor commented Sep 10, 2017

orivej commented Sep 10, 2017

ianlancetaylor commented Sep 10, 2017

crawshaw commented Sep 10, 2017

odeke-em commented Sep 10, 2017

orivej commented Sep 13, 2017

crawshaw commented Sep 13, 2017

orivej commented Sep 13, 2017

orivej commented Sep 13, 2017

crawshaw commented Sep 13, 2017

crawshaw commented Sep 13, 2017

gopherbot commented Sep 13, 2017

orivej commented Sep 15, 2017

crawshaw commented Sep 22, 2017

rsc commented Oct 13, 2017

gopherbot commented Oct 15, 2017

rsc commented Oct 26, 2017

Navigation Menu

cmd/cgo: does not replace references to paths in GOROOT/ with $GOROOT #21825

cmd/cgo: does not replace references to paths in GOROOT/ with $GOROOT #21825

Comments

orivej commented Sep 10, 2017

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

Does this issue reproduce with the latest release?

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

What did you do?

odeke-em commented Sep 10, 2017

ianlancetaylor commented Sep 10, 2017

orivej commented Sep 10, 2017

ianlancetaylor commented Sep 10, 2017

crawshaw commented Sep 10, 2017

odeke-em commented Sep 10, 2017

orivej commented Sep 13, 2017

crawshaw commented Sep 13, 2017

orivej commented Sep 13, 2017

orivej commented Sep 13, 2017

crawshaw commented Sep 13, 2017

crawshaw commented Sep 13, 2017

gopherbot commented Sep 13, 2017

orivej commented Sep 15, 2017

crawshaw commented Sep 22, 2017

rsc commented Oct 13, 2017

gopherbot commented Oct 15, 2017

rsc commented Oct 26, 2017

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

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