code review 118690044: cmd/cgo: make C function pointers non-assignable

cmd/cgo: make C function pointers non-assignable

Fixes issue 7757.
Fixes issue 8488.

[iant, 2014-08-06 23:59:03]

You can add a test case to misc/cgo/errors.

Also please run "hg mail 118690044", it looks like this was sent to me but not
to golang-codereviews.

[mdempsky, 2014-08-07 00:10:45]

On 2014/08/06 23:59:03, iant wrote:
> You can add a test case to misc/cgo/errors.

Done, but I think there's an error in cgo's "//line" output:

$ ./test.bash
misc/cgo/errors/test.bash: BUG: expected error on line 13 but saw:
# command-line-arguments
./issue7757.go:12: cannot assign to _Cgo_ptr(_Cfpvar_fp_foo)

$ go tool cgo issue7757.go

$ cat _obj/issue7757.cgo1.go
// Created by cgo - DO NOT EDIT

//line /usr/local/google/home/mdempsky/wd/go2/misc/cgo/errors/issue7757.go:5
package main
//line /usr/local/google/home/mdempsky/wd/go2/misc/cgo/errors/issue7757.go:13

//line /usr/local/google/home/mdempsky/wd/go2/misc/cgo/errors/issue7757.go:12
func main() {
//line /usr/local/google/home/mdempsky/wd/go2/misc/cgo/errors/issue7757.go:12
	_Cgo_ptr(_Cfpvar_fp_foo) = _Cgo_ptr(_Cfpvar_fp_foo)
//line /usr/local/google/home/mdempsky/wd/go2/misc/cgo/errors/issue7757.go:14
}


> Also please run "hg mail 118690044", it looks like this was sent to me but not
> to golang-codereviews.

Will do in the future.  Should I do that still for this CL?

[mdempsky, 2014-08-07 00:23:19]

On 2014/08/07 00:10:45, mdempsky wrote:
> Done, but I think there's an error in cgo's "//line" output:

Filed issue 8488 and came up with a not-*too*-ugly workaround for now.

[mdempsky, 2014-08-07 00:23:45]

Hello iant@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[mdempsky, 2014-08-08 22:17:00]

I tracked down the cause of issue 8488 and included a fix in this CL.  It ended
up being a variant of issue 6563 in the new code I added.

https://codereview.appspot.com/118690044/diff/130001/src/cmd/cgo/gcc.go
File src/cmd/cgo/gcc.go (right):

https://codereview.appspot.com/118690044/diff/130001/src/cmd/cgo/gcc.go#newco...
src/cmd/cgo/gcc.go:653: expr = &ast.CallExpr{
Alternatively, this could be something like

    expr = ast.NewIdent(fmt.Sprintf("_Cgo_ptr(%s)", name.Mangle))

and it would get NamePos assigned by line 697, but creating a CallExpr node
seems more appropriate.

[mdempsky, 2014-08-12 06:14:18]

iant: Ping? Anything further you'd like from this CL before it LGTY?

[iant, 2014-08-12 18:26:54]

Sorry, this did fall off my list.

https://codereview.appspot.com/118690044/diff/130001/src/cmd/cgo/gcc.go
File src/cmd/cgo/gcc.go (right):

https://codereview.appspot.com/118690044/diff/130001/src/cmd/cgo/gcc.go#newco...
src/cmd/cgo/gcc.go:652: // Wrap pointer with _Cgo_ptr to prevent assignments;
see issue 7757.
I find this comment a little obscure.  Perhaps you could change it to something
like

"Rewrite into call to _Cgo_ptr to prevent assignments.  The _Cgo_ptr function is
defined in out.go and simply returns its argument.  See issue 7757."

[mdempsky, 2014-08-12 18:30:17]

On 2014/08/12 18:26:54, iant wrote:
> Sorry, this did fall off my list.

No problem. :)

https://codereview.appspot.com/118690044/diff/130001/src/cmd/cgo/gcc.go
File src/cmd/cgo/gcc.go (right):

https://codereview.appspot.com/118690044/diff/130001/src/cmd/cgo/gcc.go#newco...
src/cmd/cgo/gcc.go:652: // Wrap pointer with _Cgo_ptr to prevent assignments;
see issue 7757.
On 2014/08/12 18:26:54, iant wrote:
> I find this comment a little obscure.  Perhaps you could change it to
something
> like
> 
> "Rewrite into call to _Cgo_ptr to prevent assignments.  The _Cgo_ptr function
is
> defined in out.go and simply returns its argument.  See issue 7757."

Done.

[iant, 2014-08-12 18:57:43]

LGTM

[iant, 2014-08-12 19:55:17]

*** Submitted as https://code.google.com/p/go/source/detail?r=e0bf1fc9dd8e ***

cmd/cgo: make C function pointers non-assignable

Fixes issue 7757.
Fixes issue 8488.

LGTM=iant
R=iant
CC=golang-codereviews
https://codereview.appspot.com/118690044

Committer: Ian Lance Taylor <iant@golang.org>

[gobot, 2014-08-12 20:03:57]

This CL appears to have broken the freebsd-amd64-race builder.
See http://build.golang.org/log/2ae90b340a70d7ac83e50623ce5c0271f786fe9e

[mdempsky, 2014-08-12 20:34:26]

On 2014/08/12 20:03:57, gobot wrote:
> This CL appears to have broken the freebsd-amd64-race builder.
> See http://build.golang.org/log/2ae90b340a70d7ac83e50623ce5c0271f786fe9e

Unrelated flake.

--- FAIL: TestCPUProfileMultithreaded (15.11s)
	pprof_test.go:136: crc32.ChecksumIEEE: 0
	pprof_test.go:136: crc32.Update: 13
	pprof_test.go:150: crc32.ChecksumIEEE has 0 samples out of 13, want at least 1,
ideally 6
FAIL
FAIL	runtime/pprof	35.085s