From a5d1a72a40b59db0d2f3f5d3fbb2ed60aafb7fdf Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Thu, 19 May 2016 16:27:23 -0700 Subject: [PATCH] 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 Run-TryBot: Ian Lance Taylor TryBot-Result: Gobot Gobot Reviewed-by: Austin Clements --- misc/cgo/testsanitizers/test.bash | 10 +++++ misc/cgo/testsanitizers/tsan4.go | 34 +++++++++++++++ src/cmd/cgo/out.go | 69 +++++++++++++++++++++++++++---- src/runtime/cgo.go | 4 -- src/runtime/cgo/callbacks.go | 12 ------ src/runtime/cgo/gcc_util.c | 25 ----------- src/runtime/cgocall.go | 19 --------- src/runtime/proc.go | 6 --- 8 files changed, 105 insertions(+), 74 deletions(-) create mode 100644 misc/cgo/testsanitizers/tsan4.go diff --git a/misc/cgo/testsanitizers/test.bash b/misc/cgo/testsanitizers/test.bash index 8718815d3e1..c30df3b6c2a 100755 --- 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 index 00000000000..f0c76d84116 --- /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 +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 256b059e572..5d6930d3ea4 100644 --- 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 \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 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 4fb4a613e04..9cf7b58a2f7 100644 --- 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 d0f63fb4ff9..9bde5a933fc 100644 --- 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 e20d206be6d..4111fe11951 100644 --- 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 1e0d4c7f195..0f8386b10f5 100644 --- 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 8f98cfa8a4a..ee895471046 100644 --- 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")