[go] cmd/cgo: always use a function literal for pointer checking

[Ian Lance Taylor (Gerrit)]

Ian Lance Taylor uploaded a change:
https://go-review.googlesource.com/31233

cmd/cgo: always use a function literal for pointer checking

The pointer checking code needs to know the exact type of the parameter
expected by the C function, so that it can use a type assertion to
convert the empty interface returned by cgoCheckPointer to the correct
type. Previously this was done by using a type conversion, but that
meant that the code accepted arguments that were convertible to the
parameter type, rather than arguments that were assignable as in a
normal function call. In other words, some code that should not have
passed type checking was accepted.

This CL changes cgo to always use a function literal for pointer
checking. Now the argument is passed to the function literal, which has
the correct argument type, so type checking is performed just as for a
function call as it should be. Within the function literal, the value
always has a known type, so the type assertion always succeeds.

This does have the cost of introducing another function call into any
call to a C function that requires pointer checking, but the cost of the
additional call should be minimal compared to the cost of pointer
checking.

Fixes #16591.

Change-Id: I220165564cf69db9fd5f746532d7f977a5b2c989
---
A misc/cgo/errors/issue16591.go
M misc/cgo/errors/test.bash
M misc/cgo/test/callback.go
M src/cmd/cgo/gcc.go
4 files changed, 96 insertions(+), 67 deletions(-)



diff --git a/misc/cgo/errors/issue16591.go b/misc/cgo/errors/issue16591.go
new file mode 100644
index 0000000..10eb840
--- /dev/null
+++ b/misc/cgo/errors/issue16591.go
@@ -0,0 +1,17 @@
+// Copyright 2016 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Issue 16591: Test that we detect an invalid call that was being
+// hidden by a type conversion inserted by cgo checking.
+
+package p
+
+// void f(int** p) { }
+import "C"
+
+type x *C.int
+
+func F(p *x) {
+ C.f(p) // ERROR HERE
+}
diff --git a/misc/cgo/errors/test.bash b/misc/cgo/errors/test.bash
index 84d44d8..cb44250 100755
--- a/misc/cgo/errors/test.bash
+++ b/misc/cgo/errors/test.bash
@@ -46,6 +46,7 @@
expect issue13635.go C.uchar C.schar C.ushort C.uint C.ulong C.longlong
C.ulonglong C.complexfloat C.complexdouble
check issue13830.go
check issue16116.go
+check issue16591.go

if ! go build issue14669.go; then
exit 1
diff --git a/misc/cgo/test/callback.go b/misc/cgo/test/callback.go
index 21d1df5..b88bf13 100644
--- a/misc/cgo/test/callback.go
+++ b/misc/cgo/test/callback.go
@@ -186,6 +186,7 @@
"runtime.asmcgocall",
"runtime.cgocall",
"test._Cfunc_callback",
+ "test.nestedCall.func1",
"test.nestedCall",
"test.testCallbackCallers",
"test.TestCallbackCallers",
diff --git a/src/cmd/cgo/gcc.go b/src/cmd/cgo/gcc.go
index 5df94ac..1aab69c 100644
--- a/src/cmd/cgo/gcc.go
+++ b/src/cmd/cgo/gcc.go
@@ -639,34 +639,42 @@

// We need to rewrite this call.
//
- // We are going to rewrite C.f(p) to C.f(_cgoCheckPointer(p)).
- // If the call to C.f is deferred, that will check p at the
- // point of the defer statement, not when the function is called, so
- // rewrite to func(_cgo0 ptype) { C.f(_cgoCheckPointer(_cgo0)) }(p)
-
+ // We are going to rewrite C.f(p) to
+ // func(_cgo0 ptype) { C.f(_cgoCheckPointer(_cgo0).(ptype)) }(p)
+ // Using a function literal like this lets us do correct
+ // argument type checking, and works correctly if the call is
+ // deferred.
needsUnsafe := false
- var dargs []ast.Expr
- if call.Deferred {
- dargs = make([]ast.Expr, len(name.FuncType.Params))
- }
+ params := make([]*ast.Field, len(name.FuncType.Params))
+ args := make([]ast.Expr, len(name.FuncType.Params))
for i, param := range name.FuncType.Params {
- origArg := call.Call.Args[i]
- darg := origArg
+ // params is going to become the parameters of the
+ // function literal.
+ // args is going to become the list of arguments to the
+ // function literal.
+ // nparam is the parameter of the function literal that
+ // corresponds to this parameter of the real function.

- if call.Deferred {
- dargs[i] = darg
- darg = ast.NewIdent(fmt.Sprintf("_cgo%d", i))
- call.Call.Args[i] = darg
+ origArg := call.Call.Args[i]
+ args[i] = origArg
+ nparam := ast.NewIdent(fmt.Sprintf("_cgo%d", i))
+
+ params[i] = &ast.Field{
+ Names: []*ast.Ident{nparam},
+ Type: param.Go,
}

if !p.needsPointerCheck(f, param.Go, origArg) {
+ call.Call.Args[i] = nparam
continue
}

+ // Change the function literal to call the real function
+ // with the parameter passed through _cgoCheckPointer.
c := &ast.CallExpr{
Fun: ast.NewIdent("_cgoCheckPointer"),
Args: []ast.Expr{
- darg,
+ nparam,
},
}

@@ -682,69 +690,71 @@
needsUnsafe = true
}

- // In order for the type assertion to succeed, we need
- // it to match the actual type of the argument. The
- // only type we have is the type of the function
- // parameter. We know that the argument type must be
- // assignable to the function parameter type, or the
- // code would not compile, but there is nothing
- // requiring that the types be exactly the same. Add a
- // type conversion to the argument so that the type
- // assertion will succeed.
- c.Args[0] = &ast.CallExpr{
- Fun: ptype,
- Args: []ast.Expr{
- c.Args[0],
- },
- }
-
+ // Add a type assertion from the empty interface returned
+ // by cgoCheckPointer to the real type.
call.Call.Args[i] = &ast.TypeAssertExpr{
X: c,
Type: ptype,
}
+
+ // In case the type changed.
+ params[i].Type = ptype
}

- if call.Deferred {
- params := make([]*ast.Field, len(name.FuncType.Params))
- for i, param := range name.FuncType.Params {
- ptype := p.rewriteUnsafe(param.Go)
- if ptype != param.Go {
- needsUnsafe = true
- }
- params[i] = &ast.Field{
- Names: []*ast.Ident{
- ast.NewIdent(fmt.Sprintf("_cgo%d", i)),
- },
- Type: ptype,
- }
+ fcall := &ast.CallExpr{
+ Fun: call.Call.Fun,
+ Args: call.Call.Args,
+ }
+ ftype := &ast.FuncType{
+ Params: &ast.FieldList{
+ List: params,
+ },
+ }
+ var fbody ast.Stmt
+ if name.FuncType.Result == nil {
+ fbody = &ast.ExprStmt{
+ X: fcall,
}
-
- dbody := &ast.CallExpr{
- Fun: call.Call.Fun,
- Args: call.Call.Args,
+ } else {
+ fbody = &ast.ReturnStmt{
+ Results: []ast.Expr{fcall},
}
- call.Call.Fun = &ast.FuncLit{
- Type: &ast.FuncType{
- Params: &ast.FieldList{
- List: params,
- },
- },
- Body: &ast.BlockStmt{
- List: []ast.Stmt{
- &ast.ExprStmt{
- X: dbody,
- },
+ rtype := p.rewriteUnsafe(name.FuncType.Result.Go)
+ if rtype != name.FuncType.Result.Go {
+ needsUnsafe = true
+ }
+ ftype.Results = &ast.FieldList{
+ List: []*ast.Field{
+ &ast.Field{
+ Type: rtype,
},
},
}
- call.Call.Args = dargs
- call.Call.Lparen = token.NoPos
- call.Call.Rparen = token.NoPos
+ }
+ call.Call.Fun = &ast.FuncLit{
+ Type: ftype,
+ Body: &ast.BlockStmt{
+ List: []ast.Stmt{
+ fbody,
+ },
+ },
+ }
+ call.Call.Args = args
+ call.Call.Lparen = token.NoPos
+ call.Call.Rparen = token.NoPos

- // There is a Ref pointing to the old call.Call.Fun.
- for _, ref := range f.Ref {
- if ref.Expr == &call.Call.Fun {
- ref.Expr = &dbody.Fun
+ // There is a Ref pointing to the old call.Call.Fun.
+ for _, ref := range f.Ref {
+ if ref.Expr == &call.Call.Fun {
+ ref.Expr = &fcall.Fun
+
+ // If this call expects two results, we have to
+ // adjust the results of the function we generated.
+ if ref.Context == "call2" {
+ ftype.Results.List = append(ftype.Results.List,
+ &ast.Field{
+ Type: ast.NewIdent("error"),
+ })
}
}
}

--
https://go-review.googlesource.com/31233

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 1:

TryBots beginning. Status page: http://farmer.golang.org/try?commit=d1d7e3dc

--
https://go-review.googlesource.com/31233
Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 1: TryBot-Result+1

TryBots are happy.

--
https://go-review.googlesource.com/31233
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-HasComments: No

[Matthew Dempsky (Gerrit)]

Matthew Dempsky has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 1:

(1 comment)

https://go-review.googlesource.com/#/c/31233/1/src/cmd/cgo/gcc.go
File src/cmd/cgo/gcc.go:

Line 643: // func(_cgo0 ptype) { C.f(_cgoCheckPointer(_cgo0).(ptype))
}(p)
Can you instead emit

func(_cgo0 ptype) {
_cgoCheckPointer(_cgo0)
C.f(_cgo0)
}(p)

and get rid of _cgoCheckPointer's return parameter?

--
https://go-review.googlesource.com/31233
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-HasComments: Yes

[Ian Lance Taylor (Gerrit)]

Ian Lance Taylor has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 1:

(1 comment)

https://go-review.googlesource.com/#/c/31233/1/src/cmd/cgo/gcc.go
File src/cmd/cgo/gcc.go:

Line 643: // func(_cgo0 ptype) { C.f(_cgoCheckPointer(_cgo0).(ptype))
}(p)
> Can you instead emit
Good idea. Thanks. Done.

--
https://go-review.googlesource.com/31233
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>
Gerrit-HasComments: Yes

[Ian Lance Taylor (Gerrit)]

Reviewers: Matthew Dempsky, Gobot Gobot

Ian Lance Taylor uploaded a new patch set:

https://go-review.googlesource.com/31233

cmd/cgo: always use a function literal for pointer checking

The pointer checking code needs to know the exact type of the parameter
expected by the C function, so that it can use a type assertion to
convert the empty interface returned by cgoCheckPointer to the correct
type. Previously this was done by using a type conversion, but that
meant that the code accepted arguments that were convertible to the
parameter type, rather than arguments that were assignable as in a
normal function call. In other words, some code that should not have
passed type checking was accepted.

This CL changes cgo to always use a function literal for pointer
checking. Now the argument is passed to the function literal, which has
the correct argument type, so type checking is performed just as for a
function call as it should be.

Since we now always use a function literal, simplify the checking code
to run as a statement by itself. It now no longer needs to return a
value, and we no longer need a type assertion.

This does have the cost of introducing another function call into any
call to a C function that requires pointer checking, but the cost of the
additional call should be minimal compared to the cost of pointer
checking.

Fixes #16591.

Change-Id: I220165564cf69db9fd5f746532d7f977a5b2c989
---
A misc/cgo/errors/issue16591.go
M misc/cgo/errors/test.bash
M misc/cgo/test/callback.go
M src/cmd/cgo/gcc.go

M src/cmd/cgo/out.go
M src/runtime/cgocall.go
6 files changed, 121 insertions(+), 92 deletions(-)

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 2:

TryBots beginning. Status page: http://farmer.golang.org/try?commit=1d929db0

--
https://go-review.googlesource.com/31233
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>

Gerrit-HasComments: No

[Matthew Dempsky (Gerrit)]

Matthew Dempsky has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 2: Code-Review+2

(1 comment)

https://go-review.googlesource.com/#/c/31233/2/src/runtime/cgocall.go
File src/runtime/cgocall.go:

Line 418: return
Nit: Unnecessary.

--
https://go-review.googlesource.com/31233
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>

Gerrit-HasComments: Yes

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 2: TryBot-Result+1

TryBots are happy.

--
https://go-review.googlesource.com/31233
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>

Gerrit-HasComments: No

[Ian Lance Taylor (Gerrit)]

Ian Lance Taylor has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 2:

(1 comment)

https://go-review.googlesource.com/#/c/31233/2/src/runtime/cgocall.go
File src/runtime/cgocall.go:

Line 418: return
> Nit: Unnecessary.

Done

--
https://go-review.googlesource.com/31233
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>

Gerrit-HasComments: Yes

[Ian Lance Taylor (Gerrit)]

Reviewers: Matthew Dempsky, Gobot Gobot

Ian Lance Taylor uploaded a new patch set:

https://go-review.googlesource.com/31233

cmd/cgo: always use a function literal for pointer checking

The pointer checking code needs to know the exact type of the parameter
expected by the C function, so that it can use a type assertion to
convert the empty interface returned by cgoCheckPointer to the correct
type. Previously this was done by using a type conversion, but that
meant that the code accepted arguments that were convertible to the
parameter type, rather than arguments that were assignable as in a
normal function call. In other words, some code that should not have
passed type checking was accepted.

This CL changes cgo to always use a function literal for pointer
checking. Now the argument is passed to the function literal, which has
the correct argument type, so type checking is performed just as for a
function call as it should be.

Since we now always use a function literal, simplify the checking code
to run as a statement by itself. It now no longer needs to return a
value, and we no longer need a type assertion.

This does have the cost of introducing another function call into any
call to a C function that requires pointer checking, but the cost of the
additional call should be minimal compared to the cost of pointer
checking.

Fixes #16591.

Change-Id: I220165564cf69db9fd5f746532d7f977a5b2c989
---
A misc/cgo/errors/issue16591.go
M misc/cgo/errors/test.bash
M misc/cgo/test/callback.go
M src/cmd/cgo/gcc.go

M src/cmd/cgo/out.go
M src/runtime/cgocall.go
6 files changed, 120 insertions(+), 92 deletions(-)

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 3:

TryBots beginning. Status page: http://farmer.golang.org/try?commit=15dc5a46

--
https://go-review.googlesource.com/31233
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>

Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 3: TryBot-Result+1

TryBots are happy.

[Matthew Dempsky (Gerrit)]

Matthew Dempsky has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 3:

Still LGTM

[Matthew Dempsky (Gerrit)]

Matthew Dempsky has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 3:

(1 comment)

https://go-review.googlesource.com/#/c/31233/3/src/cmd/cgo/out.go
File src/cmd/cgo/out.go:

PS3, Line 1578: return
Oh, actually "return" here and below need to be removed.

--
https://go-review.googlesource.com/31233
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>

Gerrit-HasComments: Yes

[Ian Lance Taylor (Gerrit)]

Ian Lance Taylor has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 3:

(1 comment)

https://go-review.googlesource.com/#/c/31233/3/src/cmd/cgo/out.go
File src/cmd/cgo/out.go:

Line 1578: return runtimeCgoCheckPointer(ptr, args);

> Oh, actually "return" here and below need to be removed.

Done

[Ian Lance Taylor (Gerrit)]

Reviewers: Matthew Dempsky, Gobot Gobot

Ian Lance Taylor uploaded a new patch set:

https://go-review.googlesource.com/31233

cmd/cgo: always use a function literal for pointer checking

The pointer checking code needs to know the exact type of the parameter
expected by the C function, so that it can use a type assertion to
convert the empty interface returned by cgoCheckPointer to the correct
type. Previously this was done by using a type conversion, but that
meant that the code accepted arguments that were convertible to the
parameter type, rather than arguments that were assignable as in a
normal function call. In other words, some code that should not have
passed type checking was accepted.

This CL changes cgo to always use a function literal for pointer
checking. Now the argument is passed to the function literal, which has
the correct argument type, so type checking is performed just as for a
function call as it should be.

Since we now always use a function literal, simplify the checking code
to run as a statement by itself. It now no longer needs to return a
value, and we no longer need a type assertion.

This does have the cost of introducing another function call into any
call to a C function that requires pointer checking, but the cost of the
additional call should be minimal compared to the cost of pointer
checking.

Fixes #16591.

Change-Id: I220165564cf69db9fd5f746532d7f977a5b2c989
---
A misc/cgo/errors/issue16591.go
M misc/cgo/errors/test.bash
M misc/cgo/test/callback.go
M src/cmd/cgo/gcc.go

M src/cmd/cgo/out.go
M src/runtime/cgocall.go
6 files changed, 121 insertions(+), 94 deletions(-)

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 4:

TryBots beginning. Status page: http://farmer.golang.org/try?commit=9681dffd

--
https://go-review.googlesource.com/31233
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>

Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 4: TryBot-Result+1

TryBots are happy.

[Ian Lance Taylor (Gerrit)]

Ian Lance Taylor has submitted this change and it was merged.

cmd/cgo: always use a function literal for pointer checking

The pointer checking code needs to know the exact type of the parameter
expected by the C function, so that it can use a type assertion to
convert the empty interface returned by cgoCheckPointer to the correct
type. Previously this was done by using a type conversion, but that
meant that the code accepted arguments that were convertible to the
parameter type, rather than arguments that were assignable as in a
normal function call. In other words, some code that should not have
passed type checking was accepted.

This CL changes cgo to always use a function literal for pointer
checking. Now the argument is passed to the function literal, which has
the correct argument type, so type checking is performed just as for a
function call as it should be.

Since we now always use a function literal, simplify the checking code
to run as a statement by itself. It now no longer needs to return a
value, and we no longer need a type assertion.

This does have the cost of introducing another function call into any
call to a C function that requires pointer checking, but the cost of the
additional call should be minimal compared to the cost of pointer
checking.

Fixes #16591.

Change-Id: I220165564cf69db9fd5f746532d7f977a5b2c989

Reviewed-on: https://go-review.googlesource.com/31233
Run-TryBot: Ian Lance Taylor <ia...@golang.org>
TryBot-Result: Gobot Gobot <go...@golang.org>
Reviewed-by: Matthew Dempsky <mdem...@google.com>

---
A misc/cgo/errors/issue16591.go
M misc/cgo/errors/test.bash
M misc/cgo/test/callback.go
M src/cmd/cgo/gcc.go

M src/cmd/cgo/out.go
M src/runtime/cgocall.go
6 files changed, 121 insertions(+), 94 deletions(-)

Approvals:
Matthew Dempsky: Looks good to me, approved
Ian Lance Taylor: Run TryBots
Gobot Gobot: TryBots succeeded

[Ingo Oeser (Gerrit)]

Ingo Oeser has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 5:

(1 comment)

Small comment nit.

Sorry for finding it so late, Ian. Was looking at it only out of
curiosity :-)

https://go-review.googlesource.com/#/c/31233/5/src/cmd/cgo/gcc.go
File src/cmd/cgo/gcc.go:

Line 619: // each pointer argument x with _cgoCheckPointer(x).(T).
The type-assert is gone according to the commit message, so I believe this
should read "each pointer argument x with _cgoCheckPointer(x).".

Maybe as a follow-up?

--
https://go-review.googlesource.com/31233
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: Ian Lance Taylor <ia...@golang.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>

Gerrit-HasComments: Yes

[Ian Lance Taylor (Gerrit)]

Ian Lance Taylor has posted comments on this change.

cmd/cgo: always use a function literal for pointer checking

Patch Set 5:

(1 comment)

https://go-review.googlesource.com/#/c/31233/5/src/cmd/cgo/gcc.go
File src/cmd/cgo/gcc.go:

Line 619: // each pointer argument x with _cgoCheckPointer(x).(T).
> The type-assert is gone according to the commit message, so I believe this

Thanks for noticing. Sent https://golang.org/cl/31594.