cmd/cgo: generate correct column info #26692

kolyshkin · 2018-07-30T13:58:46Z

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

go version go1.11beta2 linux/amd64

Does this issue reproduce with the latest release?

yes (currently beta2)

This is a regression in go 1.11 since when using go 1.10 there is no such issue (see below).

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/kir/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/kir/go"
GOPROXY=""
GORACE=""
GOROOT="/home/kir/sdk/go1.11beta2"
GOTMPDIR=""
GOTOOLDIR="/home/kir/sdk/go1.11beta2/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build131899104=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Ran unconvert (https://github.com/mdempsky/unconvert) on Moby (https://github.com/moby/moby) sources.

What did you expect to see?

Filenames and positions (file:line:column) for every error it finds. Example (when using Go 1.10.1):

kir@kd:~/go/src/github.com/mdempsky/unconvert$ go run unconvert.go github.com/docker/docker/daemon/graphdriver/copy
/home/kir/go/src/github.com/docker/docker/daemon/graphdriver/copy/copy.go:240:28: unnecessary conversion
/home/kir/go/src/github.com/docker/docker/daemon/graphdriver/copy/copy.go:240:50: unnecessary conversion
/home/kir/go/src/github.com/docker/docker/daemon/graphdriver/copy/copy.go:241:28: unnecessary conversion
/home/kir/go/src/github.com/docker/docker/daemon/graphdriver/copy/copy.go:241:50: unnecessary conversion

What did you see instead?

Column is 0 for files that are cgo-preprocessed (i.e. contain import "C"):

kir@kd:~/go/src/github.com/mdempsky/unconvert$ go1.11beta2 run unconvert.go github.com/docker/docker/daemon/graphdriver/copy
/home/kir/go/src/github.com/docker/docker/daemon/graphdriver/copy/copy.go:240:0: unnecessary conversion
/home/kir/go/src/github.com/docker/docker/daemon/graphdriver/copy/copy.go:240:0: unnecessary conversion
/home/kir/go/src/github.com/docker/docker/daemon/graphdriver/copy/copy.go:241:0: unnecessary conversion
/home/kir/go/src/github.com/docker/docker/daemon/graphdriver/copy/copy.go:241:0: unnecessary conversion

Analisys

This appears in cgo-processed files having //line:<file>:<line> directive (without column field), so column is 0 in call to go/token's AddLineColumnInfo() method. Next, unpack() does this

if alt.Column == 0 {
        // alternative column is unknown => relative column is unknown
        // (the current specification for line directives requires
        // this to apply until the next PosBase/line directive,
        // not just until the new newline)
        column = 0

and as a result in cgo-preprocessed files we have no column information at all. This is caused by commit 99c3021.

Suggestions

I see two ways of fixing this.

Add column 1 to cmd/cgo generated //line: marks.
Treat missing column information as column 1 (not 0 as it is now).

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

kolyshkin · 2018-07-30T14:49:50Z

Add column 1 to cmd/cgo generated //line: marks.

Fix is here: https://go-review.googlesource.com/c/go/+/126675. Tested manually by using self-compiled cgo with the patch; it works!

unconvert output is broken when using Go 1.11. A quick example: With Go 1.10: > go run unconvert.go github.com/docker/docker/daemon/graphdriver/copy > /home/kir/go/src/github.com/docker/docker/daemon/graphdriver/copy/copy.go:242:28: unnecessary conversion With Go 1.11 beta 2: > go run unconvert.go github.com/docker/docker/daemon/graphdriver/copy > copy.go:242:0: unnecessary conversion Note that there are two issues: - column is 0; - file names have path stripped. The issue with column is filed as golang/go#26692; there is no way to fix it in unconvert. The second issue is currently discussed in golang/go#26671, but it might not be fixed since the new documentation says: > The filenames in line directives now remain untouched by the scanner; > there is no cleanup or conversion of relative into absolute paths > anymore, in sync with what the compiler's scanner/parser are doing. > Any kind of filename transformation has to be done by a client. This > makes the scanner code simpler and also more predictable. So, here's the alternative: if the file name is not absolute, prepend the path as we know it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2018-07-30T15:06:46Z

@griesemer PTAL

mdempsky · 2018-07-30T19:25:33Z

cmd/cgo inserts/deletes some lines of code (e.g., removing the import "C" declaration, and adding other imports necessary for cgo), but it also rewrites some lines of code (e.g., changing C.foo() function calls into _Cfunc_foo()).

In previous releases, when parsing cmd/cgo processed source files, the //line directives gave correct line positions everywhere, but column positions were potentially wrong for lines that were rewritten due to C.foo references.

Due to go/scanner changes for Go 1.11, column information is no longer being tracked for cmd/cgo processed files. This is arguably more correct, because it no longer gives bogus column information for rewritten lines. However, it's also arguably a regression, because column information is now lost for lines that weren't rewritten.

From a casual inspection, CL 126675 restores the (questionable) behavior from Go 1.10.

I think the proper long-term fix is for cmd/cgo to emit /*line*/ directives within rewritten lines, so that we get precise column information everywhere.

For Go 1.11, I'm leaning towards it's better to give suboptimal output consistent with Go 1.10, rather than give a differently-suboptimal output. (I.e., I'm leaning towards CL 126675.)

/cc @ianlancetaylor

unconvert output is broken when using Go 1.11. A quick example: With Go 1.10: > go run unconvert.go github.com/docker/docker/daemon/graphdriver/copy > /home/kir/go/src/github.com/docker/docker/daemon/graphdriver/copy/copy.go:242:28: unnecessary conversion With Go 1.11 beta 2: > go run unconvert.go github.com/docker/docker/daemon/graphdriver/copy > copy.go:242:0: unnecessary conversion Note that there are two issues: - column is 0; - file names have path stripped. The issue with column is filed as golang/go#26692; there is no way to fix it in unconvert. The second issue is currently discussed in golang/go#26671, but it might not be fixed since the new documentation says: > The filenames in line directives now remain untouched by the scanner; > there is no cleanup or conversion of relative into absolute paths > anymore, in sync with what the compiler's scanner/parser are doing. > Any kind of filename transformation has to be done by a client. This > makes the scanner code simpler and also more predictable. So, here's the alternative: if the file name is not absolute, prepend the path as we know it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

gopherbot · 2018-07-31T19:20:05Z

Change https://golang.org/cl/126675 mentions this issue: cmd/cgo: add column number to line directives

ianlancetaylor · 2018-07-31T21:28:44Z

Reopening to get correct column info.

mdempsky · 2018-07-31T21:39:44Z

I thought I remembered one of the motivations for /*line*/ directives was to support cmd/cover, but it looks like cmd/cover actually only needs //line directives? (I was thinking cmd/cover could serve as an example of how cmd/cgo needs to change.)

Also, it looks like cmd/cover/cover.go could use the same //line %s:1:1 treatment as cmd/cgo/out.go.

ianlancetaylor · 2018-07-31T22:02:10Z

I believe that cmd/cover could use /*line*/ directives. It currently edits the code by inserting text.

That said I think cmd/cover is a less interesting case since it seems less likely that people will ask for coverage of code that generates compilation errors. Getting compilation errors on cgo-generated code, on the other hand, seems like a normal stage of development.

mdempsky · 2018-07-31T22:17:31Z

Oops, I was referring to commit 2a39d1e ("Now that cover does not modify the formatting of the original file or add any newline characters"), but forgot to actually link to that in my comment.

ianlancetaylor · 2018-07-31T22:23:17Z

cmd/cover no longer inserts lines, so error messages will show the correct line number. But cmd/cover does edit lines, so error messages will not show the correct column number.

I think the current state of things is reasonable: cmd/cover does not state a column in the //line directive, so my understanding is that the compiler will not state a column in any error messages. That seems right.

Later perhaps we should change cmd/cover to use /*line*/ directives with column information, so that the compiler gives not only correct line information but also correct column information.

mdempsky · 2018-07-31T22:34:43Z

I see. It seems I misunderstood "does not modify the formatting" to mean "does not modify at all."

ianlancetaylor · 2018-07-31T23:45:44Z

Sure, but if you think about it, "does not modify at all" doesn't really make sense here. The cmd/cover tool has to do something.

kolyshkin · 2018-08-01T05:21:42Z

Reopening to get correct column info.

I believe it should be filed as a separate issue. The original report is about missing column info for cgo-generated files which is Go-1.11beta{1,2} regression and is now fixed. The issue of skewed (not missing) column info in some (not all) lines of cgo-generated files is not a regression and is valid not just for Go 1.11beta but also earlier versions. The fact that it emerged as a followup to this issue does not mean it belongs here.

ianlancetaylor · 2018-08-01T14:36:29Z

Doesn't seem like a big deal to me in this case, but, OK, filed #26745.

kolyshkin mentioned this issue Jul 30, 2018

go/scanner: file's dir lost in go1.11 #26671

Closed

kolyshkin mentioned this issue Jul 30, 2018

Fix file name reporting for Go 1.11 [v2] mdempsky/unconvert#31

Closed

bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 31, 2018

bradfitz added this to the Go1.11 milestone Jul 31, 2018

gopherbot closed this as completed in 6bea321 Jul 31, 2018

ianlancetaylor reopened this Jul 31, 2018

ianlancetaylor changed the title ~~go/token: missing Column info for cgo-preprocessed files~~ cmd/cgo: generate correct column info Jul 31, 2018

ianlancetaylor modified the milestones: Go1.11, Go1.12 Jul 31, 2018

ianlancetaylor added the help wanted label Jul 31, 2018

kolyshkin mentioned this issue Aug 1, 2018

Bump golang to 1.11.0 moby/moby#37358

Merged

ianlancetaylor closed this as completed Aug 1, 2018

kolyshkin mentioned this issue Oct 4, 2018

file names and column info broken by go1.11beta mdempsky/unconvert#29

Closed

golang locked and limited conversation to collaborators Aug 1, 2019

gopherbot added the FrozenDueToAge label Aug 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/cgo: generate correct column info #26692

cmd/cgo: generate correct column info #26692

kolyshkin commented Jul 30, 2018 •

edited

kolyshkin commented Jul 30, 2018

kolyshkin commented Jul 30, 2018

mdempsky commented Jul 30, 2018

gopherbot commented Jul 31, 2018

ianlancetaylor commented Jul 31, 2018

mdempsky commented Jul 31, 2018 •

edited

ianlancetaylor commented Jul 31, 2018

mdempsky commented Jul 31, 2018

ianlancetaylor commented Jul 31, 2018

mdempsky commented Jul 31, 2018

ianlancetaylor commented Jul 31, 2018

kolyshkin commented Aug 1, 2018 •

edited

ianlancetaylor commented Aug 1, 2018

cmd/cgo: generate correct column info #26692

cmd/cgo: generate correct column info #26692

Comments

kolyshkin commented Jul 30, 2018 • edited

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?

What did you expect to see?

What did you see instead?

Analisys

Suggestions

kolyshkin commented Jul 30, 2018

kolyshkin commented Jul 30, 2018

mdempsky commented Jul 30, 2018

gopherbot commented Jul 31, 2018

ianlancetaylor commented Jul 31, 2018

mdempsky commented Jul 31, 2018 • edited

ianlancetaylor commented Jul 31, 2018

mdempsky commented Jul 31, 2018

ianlancetaylor commented Jul 31, 2018

mdempsky commented Jul 31, 2018

ianlancetaylor commented Jul 31, 2018

kolyshkin commented Aug 1, 2018 • edited

ianlancetaylor commented Aug 1, 2018

kolyshkin commented Jul 30, 2018 •

edited

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

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

mdempsky commented Jul 31, 2018 •

edited

kolyshkin commented Aug 1, 2018 •

edited