cmd/cgo, runtime, runtime/cgo: TSAN support for malloc

Acquire and release the TSAN synchronization point when calling malloc,
just as we do when calling any other C function. If we don't do this,
TSAN will report false positive errors about races calling malloc and
free.

We used to have a special code path for malloc and free, going through
the runtime functions cmalloc and cfree. The special code path for cfree
was no longer used even before this CL. This CL stops using the special
code path for malloc, because there is no place along that path where we
could conditionally insert the TSAN synchronization. This CL removes
the support for the special code path for both functions.

Instead, cgo now automatically generates the malloc function as though
it were referenced as C.malloc.  We need to automatically generate it
even if C.malloc is not called, even if malloc and size_t are not
declared, to support cgo-provided functions like C.CString.

Change-Id: I829854ec0787a80f33fa0a8a0dc2ee1d617830e2
Reviewed-on: https://go-review.googlesource.com/23260
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>

diff --git a/misc/cgo/testsanitizers/test.bash b/misc/cgo/testsanitizers/test.bash
index 8718815d3e19c229f7d94607401b124fc730044a..c30df3b6c2a5ec72f4f3dd17436e7bb94b8528cc 100755 (executable)
--- a/misc/cgo/testsanitizers/test.bash
+++ b/misc/cgo/testsanitizers/test.bash
@@ -144,6 +144,16 @@ if test "$tsan" = "yes"; then
        status=1
     fi
 
+    if ! go run tsan4.go 2>$err; then
+       cat $err
+       echo "FAIL: tsan4"
+       status=1
+    elif grep -i warning $err >/dev/null 2>&1; then
+       cat $err
+       echo "FAIL: tsan4"
+       status=1
+    fi
+
     rm -f $err
 fi
 

diff --git a/misc/cgo/testsanitizers/tsan4.go b/misc/cgo/testsanitizers/tsan4.go
new file mode 100644 (file)
index 0000000..f0c76d8
--- /dev/null
+++ b/misc/cgo/testsanitizers/tsan4.go
@@ -0,0 +1,34 @@
+// 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.
+
+package main
+
+// Check that calls to C.malloc/C.free do not trigger TSAN false
+// positive reports.
+
+// #cgo CFLAGS: -fsanitize=thread
+// #cgo LDFLAGS: -fsanitize=thread
+// #include <stdlib.h>
+import "C"
+
+import (
+       "runtime"
+       "sync"
+)
+
+func main() {
+       var wg sync.WaitGroup
+       for i := 0; i < 10; i++ {
+               wg.Add(1)
+               go func() {
+                       defer wg.Done()
+                       for i := 0; i < 100; i++ {
+                               p := C.malloc(C.size_t(i * 10))
+                               runtime.Gosched()
+                               C.free(p)
+                       }
+               }()
+       }
+       wg.Wait()
+}

diff --git a/src/cmd/cgo/out.go b/src/cmd/cgo/out.go
index 256b059e57211b2286fad7cb3cd55d95acc8baae..5d6930d3ea402ca09fd9afdefa7f7ebd4d479626 100644 (file)
--- a/src/cmd/cgo/out.go
+++ b/src/cmd/cgo/out.go
@@ -175,10 +175,11 @@ func (p *Package) writeDefs() {
        }
        fmt.Fprintf(fgo2, "\n")
 
+       callsMalloc := false
        for _, key := range nameKeys(p.Name) {
                n := p.Name[key]
                if n.FuncType != nil {
-                       p.writeDefsFunc(fgo2, n)
+                       p.writeDefsFunc(fgo2, n, &callsMalloc)
                }
        }
 
@@ -189,6 +190,12 @@ func (p *Package) writeDefs() {
        } else {
                p.writeExports(fgo2, fm, fgcc, fgcch)
        }
+
+       if callsMalloc && !*gccgo {
+               fmt.Fprint(fgo2, strings.Replace(cMallocDefGo, "PREFIX", cPrefix, -1))
+               fmt.Fprint(fgcc, strings.Replace(strings.Replace(cMallocDefC, "PREFIX", cPrefix, -1), "PACKED", p.packedAttribute(), -1))
+       }
+
        if err := fgcc.Close(); err != nil {
                fatalf("%s", err)
        }
@@ -352,7 +359,7 @@ func (p *Package) structType(n *Name) (string, int64) {
        return buf.String(), off
 }
 
-func (p *Package) writeDefsFunc(fgo2 io.Writer, n *Name) {
+func (p *Package) writeDefsFunc(fgo2 io.Writer, n *Name, callsMalloc *bool) {
        name := n.Go
        gtype := n.FuncType.Go
        void := gtype.Results == nil || len(gtype.Results.List) == 0
@@ -441,6 +448,9 @@ func (p *Package) writeDefsFunc(fgo2 io.Writer, n *Name) {
 
        if inProlog {
                fmt.Fprint(fgo2, builtinDefs[name])
+               if strings.Contains(builtinDefs[name], "_cgo_cmalloc") {
+                       *callsMalloc = true
+               }
                return
        }
 
@@ -712,11 +722,13 @@ func (p *Package) writeExports(fgo2, fm, fgcc, fgcch io.Writer) {
        p.writeExportHeader(fgcch)
 
        fmt.Fprintf(fgcc, "/* Created by cgo - DO NOT EDIT. */\n")
+       fmt.Fprintf(fgcc, "#include <stdlib.h>\n")
        fmt.Fprintf(fgcc, "#include \"_cgo_export.h\"\n\n")
 
        fmt.Fprintf(fgcc, "extern void crosscall2(void (*fn)(void *, int, __SIZE_TYPE__), void *, int, __SIZE_TYPE__);\n")
        fmt.Fprintf(fgcc, "extern __SIZE_TYPE__ _cgo_wait_runtime_init_done();\n")
        fmt.Fprintf(fgcc, "extern void _cgo_release_context(__SIZE_TYPE__);\n\n")
+       fmt.Fprintf(fgcc, "extern char* _cgo_topofstack(void);")
        fmt.Fprintf(fgcc, "%s\n", tsanProlog)
 
        for _, exp := range p.ExpFunc {
@@ -1352,9 +1364,6 @@ const goProlog = `
 //go:linkname _cgo_runtime_cgocall runtime.cgocall
 func _cgo_runtime_cgocall(unsafe.Pointer, uintptr) int32
 
-//go:linkname _cgo_runtime_cmalloc runtime.cmalloc
-func _cgo_runtime_cmalloc(uintptr) unsafe.Pointer
-
 //go:linkname _cgo_runtime_cgocallback runtime.cgocallback
 func _cgo_runtime_cgocallback(unsafe.Pointer, unsafe.Pointer, uintptr, uintptr)
 
@@ -1400,7 +1409,7 @@ func _Cfunc_GoBytes(p unsafe.Pointer, l _Ctype_int) []byte {
 
 const cStringDef = `
 func _Cfunc_CString(s string) *_Ctype_char {
-       p := _cgo_runtime_cmalloc(uintptr(len(s)+1))
+       p := _cgo_cmalloc(uint64(len(s)+1))
        pp := (*[1<<30]byte)(p)
        copy(pp[:], s)
        pp[len(s)] = 0
@@ -1410,7 +1419,7 @@ func _Cfunc_CString(s string) *_Ctype_char {
 
 const cBytesDef = `
 func _Cfunc_CBytes(b []byte) unsafe.Pointer {
-       p := _cgo_runtime_cmalloc(uintptr(len(b)))
+       p := _cgo_cmalloc(uint64(len(b)))
        pp := (*[1<<30]byte)(p)
        copy(pp[:], b)
        return p
@@ -1419,7 +1428,7 @@ func _Cfunc_CBytes(b []byte) unsafe.Pointer {
 
 const cMallocDef = `
 func _Cfunc__CMalloc(n _Ctype_size_t) unsafe.Pointer {
-       return _cgo_runtime_cmalloc(uintptr(n))
+       return _cgo_cmalloc(uint64(n))
 }
 `
 
@@ -1432,6 +1441,50 @@ var builtinDefs = map[string]string{
        "_CMalloc":  cMallocDef,
 }
 
+// Definitions for C.malloc in Go and in C. We define it ourselves
+// since we call it from functions we define, such as C.CString.
+// Also, we have historically ensured that C.malloc does not return
+// nil even for an allocation of 0.
+
+const cMallocDefGo = `
+//go:cgo_import_static _cgoPREFIX_Cfunc__Cmalloc
+//go:linkname __cgofn__cgoPREFIX_Cfunc__Cmalloc _cgoPREFIX_Cfunc__Cmalloc
+var __cgofn__cgoPREFIX_Cfunc__Cmalloc byte
+var _cgoPREFIX_Cfunc__Cmalloc = unsafe.Pointer(&__cgofn__cgoPREFIX_Cfunc__Cmalloc)
+
+//go:cgo_unsafe_args
+func _cgo_cmalloc(p0 uint64) (r1 unsafe.Pointer) {
+       _cgo_runtime_cgocall(_cgoPREFIX_Cfunc__Cmalloc, uintptr(unsafe.Pointer(&p0)))
+       return
+}
+`
+
+// cMallocDefC defines the C version of C.malloc for the gc compiler.
+// It is defined here because C.CString and friends need a definition.
+// We define it by hand, rather than simply inventing a reference to
+// C.malloc, because <stdlib.h> may not have been included.
+// This is approximately what writeOutputFunc would generate, but
+// skips the cgo_topofstack code (which is only needed if the C code
+// calls back into Go). This also avoids returning nil for an
+// allocation of 0 bytes.
+const cMallocDefC = `
+CGO_NO_SANITIZE_THREAD
+void _cgoPREFIX_Cfunc__Cmalloc(void *v) {
+       struct {
+               unsigned long long p0;
+               void *r1;
+       } PACKED *a = v;
+       void *ret;
+       _cgo_tsan_acquire();
+       ret = malloc(a->p0);
+       if (ret == 0 && a->p0 == 0) {
+               ret = malloc(1);
+       }
+       a->r1 = ret;
+       _cgo_tsan_release();
+}
+`
+
 func (p *Package) cPrologGccgo() string {
        return strings.Replace(strings.Replace(cPrologGccgo, "PREFIX", cPrefix, -1),
                "GCCGOSYMBOLPREF", p.gccgoSymbolPrefix(), -1)

diff --git a/src/runtime/cgo.go b/src/runtime/cgo.go
index 4fb4a613e044d4fdeecba9b3ea96900a3583e994..9cf7b58a2f7a2ec301f66956a4fe20411c68bf96 100644 (file)
--- a/src/runtime/cgo.go
+++ b/src/runtime/cgo.go
@@ -11,8 +11,6 @@ import "unsafe"
 // Filled in by runtime/cgo when linked into binary.
 
 //go:linkname _cgo_init _cgo_init
-//go:linkname _cgo_malloc _cgo_malloc
-//go:linkname _cgo_free _cgo_free
 //go:linkname _cgo_thread_start _cgo_thread_start
 //go:linkname _cgo_sys_thread_create _cgo_sys_thread_create
 //go:linkname _cgo_notify_runtime_init_done _cgo_notify_runtime_init_done
@@ -21,8 +19,6 @@ import "unsafe"
 
 var (
        _cgo_init                     unsafe.Pointer
-       _cgo_malloc                   unsafe.Pointer
-       _cgo_free                     unsafe.Pointer
        _cgo_thread_start             unsafe.Pointer
        _cgo_sys_thread_create        unsafe.Pointer
        _cgo_notify_runtime_init_done unsafe.Pointer

diff --git a/src/runtime/cgo/callbacks.go b/src/runtime/cgo/callbacks.go
index d0f63fb4ff9d74a583f7991554783e64adef9b2a..9bde5a933fcd15da4e1e41031c73c4da01199302 100644 (file)
--- a/src/runtime/cgo/callbacks.go
+++ b/src/runtime/cgo/callbacks.go
@@ -52,18 +52,6 @@ func _cgo_panic(a unsafe.Pointer, n int32) {
 var x_cgo_init byte
 var _cgo_init = &x_cgo_init
 
-//go:cgo_import_static x_cgo_malloc
-//go:linkname x_cgo_malloc x_cgo_malloc
-//go:linkname _cgo_malloc _cgo_malloc
-var x_cgo_malloc byte
-var _cgo_malloc = &x_cgo_malloc
-
-//go:cgo_import_static x_cgo_free
-//go:linkname x_cgo_free x_cgo_free
-//go:linkname _cgo_free _cgo_free
-var x_cgo_free byte
-var _cgo_free = &x_cgo_free
-
 //go:cgo_import_static x_cgo_thread_start
 //go:linkname x_cgo_thread_start x_cgo_thread_start
 //go:linkname _cgo_thread_start _cgo_thread_start

diff --git a/src/runtime/cgo/gcc_util.c b/src/runtime/cgo/gcc_util.c
index e20d206be6d7996407b9cb84e6118bdff161d270..4111fe11951df885c29052c6e2a4b634b842bd13 100644 (file)
--- a/src/runtime/cgo/gcc_util.c
+++ b/src/runtime/cgo/gcc_util.c
@@ -4,31 +4,6 @@
 
 #include "libcgo.h"
 
-/* Stub for calling malloc from Go */
-void
-x_cgo_malloc(void *p)
-{
-       struct a {
-               long long n;
-               void *ret;
-       } *a = p;
-
-       a->ret = malloc(a->n);
-       if(a->ret == NULL && a->n == 0)
-               a->ret = malloc(1);
-}
-
-/* Stub for calling free from Go */
-void
-x_cgo_free(void *p)
-{
-       struct a {
-               void *arg;
-       } *a = p;
-
-       free(a->arg);
-}
-
 /* Stub for creating a new thread */
 void
 x_cgo_thread_start(ThreadStart *arg)

diff --git a/src/runtime/cgocall.go b/src/runtime/cgocall.go
index 1e0d4c7f19504a795d38e0f122e88b19650a80bf..0f8386b10f54d50e26a569c5acd69f0a8f41e9ff 100644 (file)
--- a/src/runtime/cgocall.go
+++ b/src/runtime/cgocall.go
@@ -145,25 +145,6 @@ func endcgo(mp *m) {
        unlockOSThread() // invalidates mp
 }
 
-// Helper functions for cgo code.
-
-func cmalloc(n uintptr) unsafe.Pointer {
-       var args struct {
-               n   uint64
-               ret unsafe.Pointer
-       }
-       args.n = uint64(n)
-       cgocall(_cgo_malloc, unsafe.Pointer(&args))
-       if args.ret == nil {
-               throw("C malloc failed")
-       }
-       return args.ret
-}
-
-func cfree(p unsafe.Pointer) {
-       cgocall(_cgo_free, p)
-}
-
 // Call from C back to Go.
 //go:nosplit
 func cgocallbackg(ctxt uintptr) {

diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index 8f98cfa8a4a91157a1fdf96e2ec2bffe51dd6906..ee895471046e62decc2c79b4872433d69a775ad6 100644 (file)
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -155,12 +155,6 @@ func main() {
                if _cgo_thread_start == nil {
                        throw("_cgo_thread_start missing")
                }
-               if _cgo_malloc == nil {
-                       throw("_cgo_malloc missing")
-               }
-               if _cgo_free == nil {
-                       throw("_cgo_free missing")
-               }
                if GOOS != "windows" {
                        if _cgo_setenv == nil {
                                throw("_cgo_setenv missing")