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: generate correct column info #26692

Closed
kolyshkin opened this issue Jul 30, 2018 · 13 comments

Comments

@kolyshkin
Copy link
Contributor

commented Jul 30, 2018

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.

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2018

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!

kolyshkin added a commit to kolyshkin/unconvert that referenced this issue Jul 30, 2018
Fix reporting for Go 1.11
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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2018

@griesemer PTAL

@mdempsky

This comment has been minimized.

Copy link
Member

commented Jul 30, 2018

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

kolyshkin added a commit to kolyshkin/unconvert that referenced this issue Jul 31, 2018
Fix reporting for Go 1.11
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>

@bradfitz bradfitz added the NeedsFix label Jul 31, 2018

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

@gopherbot

This comment has been minimized.

Copy link

commented Jul 31, 2018

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

@gopherbot gopherbot closed this in 6bea321 Jul 31, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

Reopening to get correct column info.

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

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

@mdempsky

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

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

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

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

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

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

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2018

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

@golang golang locked and limited conversation to collaborators Aug 1, 2019

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