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/23260Reviewed-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>

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
 

misc/cgo/testsanitizers/tsan4.go

+// 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()
+}

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)

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

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

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)

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) {

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")