cmd/cgo: generate correct column info

[ianlancetaylor]

The cgo tool reads input files written in Go referring to the magic import "C" and writes new files written in Go that use mangled names. cgo preserves line breaks, so errors reported in the new file have the correct line numbers, but they do not have correct column information. That is, column numbers in error reports about user written code will refer to the newly generated code, not the original user written code. We should the new /*line*/ comments to get correct column information.

[iamoryanmoshe]

If I understand you correctly, in generation of the new files we add a comment of the original line so we can track where the error happened?

Your last line was a bit unclear..

Anyway sounds like a pretty quick fix I can do if it's what I think it is.

[ianlancetaylor]

As of 1.11 cmd/compile supports using /*line :line:col*/ comments to set both the line and column position (see https://tip.golang.org/cmd/compile). This can be used to set the correct column position wherever we edit the input file.

That said I recommend that you not work on cgo code today, because for 1.12 I may try to add https://golang.org/cl/120615, which will substantially change how cgo's code generation works. If you change the cmd/internal/edit package to support adding /*line :line:col*/ comments, that will help with cmd/cover and may help with cmd/cgo down the road.

[iamoryanmoshe]

So I should try looking at passing the mechanism from cgo to cover?

[ianlancetaylor]

Both cmd/cgo and cmd/cover use cmd/internal/edit. If cmd/internal/edit can add /*line*/ comments, that will address this column position issue for both tools.

[iamoryanmoshe]

@ianlancetaylor
Currently cgo does something like it on line 525 in src/cmd/cgo/out.go am I right?

Just so I'll know I'm in the right direction

[ianlancetaylor]

Line 525 is //line FILE:1:1, so that gets the line numbers right. To get the column numbers right we need to use /*line*/ comment in the middle of the line. See the discussion of line comments at https://tip.golang.org/cmd/compile.

[iamoryanmoshe]

@ianlancetaylor
Ok, so if I understand you correctly, instead of doing:

//line FILE:1
fmt.Println(C.C_func(5))

We want to do:

fmt.Println(/*line FILE:1:13*/C.C_func(5))

That's what I understood from:

...and for a /*line comment this is the character position immediately following the closing */

Right?

And do you want me to add a function in edit.go that is specifically for that and then use it in cgo and cover?
What's wrong with the current Insert way, and just use it with a pos to the middle of the line with the inserted string being the /*line FILE:1:13*/?

[iamoryanmoshe]

@ianlancetaylor
Line 1294 on src/cmd/cgo/gcc.go

After mangling the names we edit the current file so it has the new mangled name instead of the original.
This is the place to add (just before the repl) the `/line :linenum:colnum/
If I understand correctly. I just need to find a way to get the line number and col number and push it before, why change the edit.go implementation if it's supposed to be generic for any buffer editing?

Edit: specifically, because we have the file object and the pos object we can just get the string by calling f.Position(pos).String()

[ianlancetaylor]

I was thinking we should change cmd/internal/edit because then we will fix both cmd/cover and cmd/cgo at the same time. But if it turns out to be simpler to do it in cgo directly, that is OK too.

[iamoryanmoshe]

I think we should fix it in cgo and cover separately, because edit in supposed to be a generic byte buffer editor, but it's your call.
If we do chance it cgo do you want me to add it to cover too?

[ianlancetaylor]

I would have to see the change.

[iamoryanmoshe]

I can't seem to reproduce this issue.
I have written some test code and I don't get any col information, not even a wrong one.

package main

//int a = 234;
//int b = 0;
/*
int div(int x, int y){
	return x / y;
}
*/
import (
	"C"
)

import "fmt"

func main() {
	fmt.Println(C.a / C.b)

}

Output of C.a / C.b:

goroutine 1 [running]:
main.main()
        C:/Users/oryan/go/src/github.com/oryanmoshe/cgotest/main.go:17 +0xab
exit status 2

Output of C.div(5,0):
exit status 3221225620

On which errors does this issue occur?

I need steps to reproduce so I'll know where to look, because so far the only place I saw that a fix should be inserted in is rewriteRef and my edits there don't effect the errors

[ianlancetaylor]

This change would only affect compilation errors, not run time errors. Here is an example:

package p

// int a;
import "C"

func F(i int) int {
	return C.a + i
}

When I try build this today, I get

foo.go:7:20: invalid operation: *_Cvar_a + i (mismatched types _Ctype_int and int)

The column number 20 is incorrect. Line 7 has only 16 columns. The column number should be 13.

[iamoryanmoshe]

@ianlancetaylor
I added a fix which did change the output go files, but it didn't seem to effect anything regarding the error message.
The new output file looks like:

// Code generated by cmd/cgo; DO NOT EDIT.

//line C:/Users/oryan/go/src/github.com/oryanmoshe/cgotest/main.go:1:1
package main

// int a;
import _ "unsafe"

func main() {
	F(5)
}

func F(i int) int {
	return /*line :11:9*/(*_Cvar_a) + i
}

But the error is still .\main.go:11:20: invalid operation: *_Cvar_a + i (mismatched types _Ctype_int and int)

Do you have any idea what might have went wrong? Is it possible the new /*line*/ comments don't work properly?

Edit: It's the same if I add the filename for the first argument in the line comment

[ianlancetaylor]

Pretty sure the /*line*/ comment is going to have to go after _Cvar_a, not before.

Try fiddling with the number you emit to see whether the comments work properly.

[iamoryanmoshe]

According to the docs:

...and for a /*line comment this is the character position immediately following the closing */

Anyway, I tried putting it after the var and it worked (sort of?)! Now the error message is .\main.go:11:10: invalid operation: *_Cvar_a + i (mismatched types _Ctype_int and int)

About the documentation, should I edit it?
Is this considered the correct column number? Or should it be be 13? If it should be 13 I might need to edit the column number to reflect the added chars to the variable name.

[ianlancetaylor]

As far as I can see the docs are correct.

The issue here is that the original code says C.a and the generated code says _Cvar_a. We want the compiler to issue column numbers for the original code, not the generated code, since the user does not see the generated code. The fact that the second string is longer means that the column number is not correct for the original code.

The correct column number for the error in my example is the one that matches the + in the original code. By my count that was 13 but maybe I got it wrong. Try using an editor that brings you to the exact position mentioned in an error message. It should bring you the + in the original code.

[iamoryanmoshe]

But the error message contains the mangled name, not the original name.

Should I fix it too? I think I understand the problem, we want the line comment to be after the cvar, but I need to add 3 to the col (because len(C.a) === 3) and then it'll be in the right position according to your count.

Are you sure the docs are correct? It says the line directive specifies the position for the character immediately following the directive, not precsiding it.. But I might be understanding it upside down

[ianlancetaylor]

The fact that the error messages contains the mangled name is a separate bug that should be fixed separately. It's supposed to be handled by (*Builder).processOutput in cmd/go/internal/work/exec.go. I don't know what that isn't helping in this case.

I expect that the docs are correct. Note that column numbers start at 1.

[gopherbot]

Change https://golang.org/cl/128036 mentions this issue: cmd/cgo: generate correct column info

[gopherbot]

Change https://golang.org/cl/151598 mentions this issue: cmd/cgo: set correct column for user-written code