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: does not replace references to paths in GOROOT/ with $GOROOT #21825

Closed
orivej opened this issue Sep 10, 2017 · 18 comments

Comments

@orivej
Copy link

commented Sep 10, 2017

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.

@orivej orivej referenced this issue Sep 10, 2017
5 of 8 tasks complete
@odeke-em

This comment has been minimized.

Copy link
Member

commented Sep 10, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2017

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.

@ianlancetaylor 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 ianlancetaylor added this to the Go1.10 milestone Sep 10, 2017

@orivej

This comment has been minimized.

Copy link
Author

commented Sep 10, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2017

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

@crawshaw

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2017

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

This comment has been minimized.

Copy link
Member

commented Sep 10, 2017

@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

This comment has been minimized.

Copy link
Author

commented Sep 13, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

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

This comment has been minimized.

Copy link
Author

commented Sep 13, 2017

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

This comment has been minimized.

Copy link
Author

commented Sep 13, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

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

This comment has been minimized.

Copy link

commented Sep 13, 2017

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

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

@gopherbot gopherbot closed this in 27e80f7 Sep 15, 2017

@orivej

This comment has been minimized.

Copy link
Author

commented Sep 15, 2017

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

@orivej orivej referenced this issue Sep 17, 2017
6 of 8 tasks complete
@crawshaw

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2017

Reopening for 1.9.1 cherry-pick.

@crawshaw crawshaw reopened this Sep 22, 2017

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

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

CL 63693 OK for Go 1.9.2.

@gopherbot

This comment has been minimized.

Copy link

commented Oct 15, 2017

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

gopherbot pushed a commit that referenced this issue Oct 25, 2017
[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

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2017

go1.9.2 has been packaged and includes:

The release is posted at golang.org/dl.

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

@rsc rsc closed this Oct 26, 2017

@golang golang locked and limited conversation to collaborators Oct 26, 2018

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