From 3c1e1c30fdfbdaf7cf5f947c53245f1c28e56f91 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Wed, 12 Oct 2016 17:09:54 -0700 Subject: [PATCH] cmd/cgo: use alias for unsafe rather than separate functions When we need to generate a call to _cgoCheckPointer, we need to type assert the result back to the desired type. That is harder when the type is unsafe.Pointer, as the package can have values of unsafe.Pointer types without actually importing unsafe, by mixing C void* and :=. We used to handle this by generating a special function for each needed type, and defining that function in a separate file where we did import unsafe. Simplify the code by not generating those functions, but instead just import unsafe under the alias _cgo_unsafe. This is a simplification step toward a fix for issue #16591. Change-Id: I0edb3e04b6400ca068751709fe063397cf960a54 Reviewed-on: https://go-review.googlesource.com/30973 Run-TryBot: Ian Lance Taylor TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick --- src/cmd/cgo/gcc.go | 197 +++++++++++++++++++++----------------------- src/cmd/cgo/main.go | 4 - src/cmd/cgo/out.go | 13 --- 3 files changed, 95 insertions(+), 119 deletions(-) diff --git a/src/cmd/cgo/gcc.go b/src/cmd/cgo/gcc.go index 9c268ca494..5df94ac54e 100644 --- a/src/cmd/cgo/gcc.go +++ b/src/cmd/cgo/gcc.go @@ -167,7 +167,23 @@ func (p *Package) Translate(f *File) { if len(needType) > 0 { p.loadDWARF(f, needType) } - p.rewriteCalls(f) + if p.rewriteCalls(f) { + // Add `import _cgo_unsafe "unsafe"` as the first decl + // after the package statement. + imp := &ast.GenDecl{ + Tok: token.IMPORT, + Specs: []ast.Spec{ + &ast.ImportSpec{ + Name: ast.NewIdent("_cgo_unsafe"), + Path: &ast.BasicLit{ + Kind: token.STRING, + Value: `"unsafe"`, + }, + }, + }, + } + f.AST.Decls = append([]ast.Decl{imp}, f.AST.Decls...) + } p.rewriteRef(f) } @@ -578,7 +594,9 @@ func (p *Package) mangleName(n *Name) { // rewriteCalls rewrites all calls that pass pointers to check that // they follow the rules for passing pointers between Go and C. -func (p *Package) rewriteCalls(f *File) { +// This returns whether the package needs to import unsafe as _cgo_unsafe. +func (p *Package) rewriteCalls(f *File) bool { + needsUnsafe := false for _, call := range f.Calls { // This is a call to C.xxx; set goname to "xxx". goname := call.Call.Fun.(*ast.SelectorExpr).Sel.Name @@ -590,18 +608,22 @@ func (p *Package) rewriteCalls(f *File) { // Probably a type conversion. continue } - p.rewriteCall(f, call, name) + if p.rewriteCall(f, call, name) { + needsUnsafe = true + } } + return needsUnsafe } // rewriteCall rewrites one call to add pointer checks. We replace // each pointer argument x with _cgoCheckPointer(x).(T). -func (p *Package) rewriteCall(f *File, call *Call, name *Name) { +// This returns whether the package needs to import unsafe as _cgo_unsafe. +func (p *Package) rewriteCall(f *File, call *Call, name *Name) bool { // Avoid a crash if the number of arguments is // less than the number of parameters. // This will be caught when the generated file is compiled. if len(call.Call.Args) < len(name.FuncType.Params) { - return + return false } any := false @@ -612,7 +634,7 @@ func (p *Package) rewriteCall(f *File, call *Call, name *Name) { } } if !any { - return + return false } // We need to rewrite this call. @@ -622,6 +644,7 @@ func (p *Package) rewriteCall(f *File, call *Call, name *Name) { // point of the defer statement, not when the function is called, so // rewrite to func(_cgo0 ptype) { C.f(_cgoCheckPointer(_cgo0)) }(p) + needsUnsafe := false var dargs []ast.Expr if call.Deferred { dargs = make([]ast.Expr, len(name.FuncType.Params)) @@ -651,57 +674,42 @@ func (p *Package) rewriteCall(f *File, call *Call, name *Name) { // expression. c.Args = p.checkAddrArgs(f, c.Args, origArg) - // _cgoCheckPointer returns interface{}. - // We need to type assert that to the type we want. - // If the Go version of this C type uses - // unsafe.Pointer, we can't use a type assertion, - // because the Go file might not import unsafe. - // Instead we use a local variant of _cgoCheckPointer. - - var arg ast.Expr - if n := p.unsafeCheckPointerName(param.Go, call.Deferred); n != "" { - c.Fun = ast.NewIdent(n) - arg = c - } else { - // 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: param.Go, - Args: []ast.Expr{ - c.Args[0], - }, - } - - arg = &ast.TypeAssertExpr{ - X: c, - Type: param.Go, - } + // The Go version of the C type might use unsafe.Pointer, + // but the file might not import unsafe. + // Rewrite the Go type if necessary to use _cgo_unsafe. + ptype := p.rewriteUnsafe(param.Go) + if ptype != param.Go { + 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], + }, } - call.Call.Args[i] = arg + call.Call.Args[i] = &ast.TypeAssertExpr{ + X: c, + Type: ptype, + } } if call.Deferred { params := make([]*ast.Field, len(name.FuncType.Params)) for i, param := range name.FuncType.Params { - ptype := param.Go - if p.hasUnsafePointer(ptype) { - // Avoid generating unsafe.Pointer by using - // interface{}. This works because we are - // going to call a _cgoCheckPointer function - // anyhow. - ptype = &ast.InterfaceType{ - Methods: &ast.FieldList{}, - } + ptype := p.rewriteUnsafe(param.Go) + if ptype != param.Go { + needsUnsafe = true } params[i] = &ast.Field{ Names: []*ast.Ident{ @@ -740,6 +748,8 @@ func (p *Package) rewriteCall(f *File, call *Call, name *Name) { } } } + + return needsUnsafe } // needsPointerCheck returns whether the type t needs a pointer check. @@ -935,69 +945,52 @@ func (p *Package) isType(t ast.Expr) bool { return false } -// unsafeCheckPointerName is given the Go version of a C type. If the -// type uses unsafe.Pointer, we arrange to build a version of -// _cgoCheckPointer that returns that type. This avoids using a type -// assertion to unsafe.Pointer in our copy of user code. We return -// the name of the _cgoCheckPointer function we are going to build, or -// the empty string if the type does not use unsafe.Pointer. -// -// The deferred parameter is true if this check is for the argument of -// a deferred function. In that case we need to use an empty interface -// as the argument type, because the deferred function we introduce in -// rewriteCall will use an empty interface type, and we can't add a -// type assertion. This is handled by keeping a separate list, and -// writing out the lists separately in writeDefs. -func (p *Package) unsafeCheckPointerName(t ast.Expr, deferred bool) string { - if !p.hasUnsafePointer(t) { - return "" - } - var buf bytes.Buffer - conf.Fprint(&buf, fset, t) - s := buf.String() - checks := &p.CgoChecks - if deferred { - checks = &p.DeferredCgoChecks - } - for i, t := range *checks { - if s == t { - return p.unsafeCheckPointerNameIndex(i, deferred) - } - } - *checks = append(*checks, s) - return p.unsafeCheckPointerNameIndex(len(*checks)-1, deferred) -} - -// hasUnsafePointer returns whether the Go type t uses unsafe.Pointer. -// t is the Go version of a C type, so we don't need to handle every case. -// We only care about direct references, not references via typedefs. -func (p *Package) hasUnsafePointer(t ast.Expr) bool { +// rewriteUnsafe returns a version of t with references to unsafe.Pointer +// rewritten to use _cgo_unsafe.Pointer instead. +func (p *Package) rewriteUnsafe(t ast.Expr) ast.Expr { switch t := t.(type) { case *ast.Ident: // We don't see a SelectorExpr for unsafe.Pointer; // this is created by code in this file. - return t.Name == "unsafe.Pointer" + if t.Name == "unsafe.Pointer" { + return ast.NewIdent("_cgo_unsafe.Pointer") + } case *ast.ArrayType: - return p.hasUnsafePointer(t.Elt) + t1 := p.rewriteUnsafe(t.Elt) + if t1 != t.Elt { + r := *t + r.Elt = t1 + return &r + } case *ast.StructType: + changed := false + fields := *t.Fields + fields.List = nil for _, f := range t.Fields.List { - if p.hasUnsafePointer(f.Type) { - return true + ft := p.rewriteUnsafe(f.Type) + if ft == f.Type { + fields.List = append(fields.List, f) + } else { + fn := *f + fn.Type = ft + fields.List = append(fields.List, &fn) + changed = true } } + if changed { + r := *t + r.Fields = &fields + return &r + } case *ast.StarExpr: // Pointer type. - return p.hasUnsafePointer(t.X) - } - return false -} - -// unsafeCheckPointerNameIndex returns the name to use for a -// _cgoCheckPointer variant based on the index in the CgoChecks slice. -func (p *Package) unsafeCheckPointerNameIndex(i int, deferred bool) string { - if deferred { - return fmt.Sprintf("_cgoCheckPointerInDefer%d", i) + x1 := p.rewriteUnsafe(t.X) + if x1 != t.X { + r := *t + r.X = x1 + return &r + } } - return fmt.Sprintf("_cgoCheckPointer%d", i) + return t } // rewriteRef rewrites all the C.xxx references in f.AST to refer to the diff --git a/src/cmd/cgo/main.go b/src/cmd/cgo/main.go index 72ac19ad39..52cffd6520 100644 --- a/src/cmd/cgo/main.go +++ b/src/cmd/cgo/main.go @@ -42,10 +42,6 @@ type Package struct { GoFiles []string // list of Go files GccFiles []string // list of gcc output files Preamble string // collected preamble for _cgo_export.h - - // See unsafeCheckPointerName. - CgoChecks []string - DeferredCgoChecks []string } // A File collects information about a single Go input file. diff --git a/src/cmd/cgo/out.go b/src/cmd/cgo/out.go index 50d6b728b7..e0b9cc46a8 100644 --- a/src/cmd/cgo/out.go +++ b/src/cmd/cgo/out.go @@ -126,19 +126,6 @@ func (p *Package) writeDefs() { fmt.Fprint(fgo2, goProlog) } - for i, t := range p.CgoChecks { - n := p.unsafeCheckPointerNameIndex(i, false) - fmt.Fprintf(fgo2, "\nfunc %s(p %s, args ...interface{}) %s {\n", n, t, t) - fmt.Fprintf(fgo2, "\treturn _cgoCheckPointer(p, args...).(%s)\n", t) - fmt.Fprintf(fgo2, "}\n") - } - for i, t := range p.DeferredCgoChecks { - n := p.unsafeCheckPointerNameIndex(i, true) - fmt.Fprintf(fgo2, "\nfunc %s(p interface{}, args ...interface{}) %s {\n", n, t) - fmt.Fprintf(fgo2, "\treturn _cgoCheckPointer(p, args...).(%s)\n", t) - fmt.Fprintf(fgo2, "}\n") - } - gccgoSymbolPrefix := p.gccgoSymbolPrefix() cVars := make(map[string]bool) -- 2.48.1