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/compile: //line problems #22660

Closed
rsc opened this issue Nov 10, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@rsc
Copy link
Contributor

commented Nov 10, 2017

I'm trying to fix and test some issues around cmd/cover and line numbers, but I've run into two compiler problems with the processing of //line directives. These seem like they should be easy fixes.

Literal $GOROOT

g=$(go env GOROOT)
cd $g
echo "//line $g/file:1" >x.go
cat x.go
go tool compile x.go

This creates a trivial x.go with a line directive that gives a full path to a non-existent file in the GOROOT root. (The fact that the file does not exist is irrelevant.)

Then the compiler prints an error because there is no package statement in the file. The error should be reported to the fake file name, and it is, except the path name literally says $GOROOT instead of the actual GOROOT directory.

% go tool compile x.go
$GOROOT/file:1[x.go:2:1]: syntax error: package statement must be first
%

The $GOROOT appears because that's what we do want to write as the file name in the object file and export data. But for error messages we want to see the actual path name. It looks like objabi.AbsFile is being called incorrectly (a bit too early) while handling the //line directive. It looks like the code needs to distinguish between computing PosBase.filename and PosBase.absFilename, and it makes no such distinction.

Line number stack

The syntax $line[$actualLine] in error messages traces back to the Plan 9 C compiler, but it is problematic for code generated during a build because the bracketed path is useless noise. We should turn it off by default. If we want to preserve the ability to see those annotations, we can reintroduce the old -L (show full file names) flag to add this.

For example, the error in the case above needs to say just:

/Users/rsc/go/file:1: syntax error: package statement must be first

without the [x.go:2:1].

@rsc rsc added this to the Go1.10 milestone Nov 10, 2017

@rsc

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2017

@griesemer griesemer self-assigned this Nov 10, 2017

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2017

This used to work correctly. It appears that https://go-review.googlesource.com/c/go/+/63693 broke this code (while fixing a bunch of related issues).

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2017

/cc @crawshaw

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2017

@crawshaw I'm inclined to revert https://go-review.googlesource.com/c/go/+/63693, it calls objabi.AbsFile early, at a place where we expect the actual filename of the //line directive, unchanged. I don't understand how this is supposed to work.

@crawshaw

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2017

@griesemer I suspect I didn't consider this (obvious in retrospect) case.

CL 63693 also fixes #21720 which I would say is worse than this one, and was backported to 1.9 for it. So I'm disinclined to just revert it without finding a solution to both cases.

I am out today, but can take a look on Monday if you like.

@gopherbot

This comment has been minimized.

Copy link

commented Nov 10, 2017

Change https://golang.org/cl/77090 mentions this issue: cmd/compile: record original and absolute file names for line directives

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2017

@crawshaw I think actually the bug is that we don't provide both the unchanged and the cleaned absolute file name. I have a try-out CL coming shortly.

@gopherbot

This comment has been minimized.

Copy link

commented Nov 10, 2017

Change https://golang.org/cl/77151 mentions this issue: cmd/cover: add //line comment pointing to original file

@gopherbot

This comment has been minimized.

Copy link

commented Jun 29, 2018

Change https://golang.org/cl/121735 mentions this issue: all: update stale test skips

gopherbot pushed a commit that referenced this issue Aug 20, 2018

all: update stale test skips
Issues #10043, #15405, and #22660 appear to have been fixed, and
whatever tests I could run locally do succeed, so remove the skips.

Issue #7237 was closed in favor of #17906, so update its skip line.

Issue #7634 was closed as it had not appeared for over three years.
Re-enable it for now. An issue should be open if the test starts being
skipped again.

Change-Id: I67daade906744ed49223291035baddaad9f56dca
Reviewed-on: https://go-review.googlesource.com/121735
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.