From patchwork Wed Oct 8 14:03:15 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ian Lance Taylor X-Patchwork-Id: 397643 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 7954114009E for ; Thu, 9 Oct 2014 01:03:33 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:subject:date:message-id:mime-version:content-type; q=dns; s= default; b=ED16jzsNOEybagRcnBQE5WmlpCFS9H90i1zgRiEJQjV7o5W5bbB9g pxdYXWMv/CFn6lipYLAqy2IkT/a4FlF0J1aOMrqhY+/aC8/rZ4gaGA5kHgxqotEp swxS3hRkUlCeqsADOduSoE6k1UjdmgBQPZDQEZ0psI0q+K10su/tFY= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:subject:date:message-id:mime-version:content-type; s= default; bh=dH/bor9TtQARgr7luVx9zGVOsss=; b=HXgPvZC4so9S9HQvLbxU FKZ1DWpfZuWqP8W/8yU7y7AJtb5F2e1Jw9FuwNmdhQRODB1h5Ipzyq+lffgKf+0H hmmlGlf5tMsldZUWPvudLy5zOlYH+DWs54TjFQEUJsd1DfNKcDAPD/mx5Ni25T6/ 1b1pBvs7EJVoEJTRW7g+Vc8= Received: (qmail 15339 invoked by alias); 8 Oct 2014 14:03:26 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 15329 invoked by uid 89); 8 Oct 2014 14:03:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS, T_RP_MATCHES_RCVD, T_TVD_MIME_NO_HEADERS autolearn=ham version=3.3.2 X-HELO: mail-pa0-f50.google.com Received: from mail-pa0-f50.google.com (HELO mail-pa0-f50.google.com) (209.85.220.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 08 Oct 2014 14:03:23 +0000 Received: by mail-pa0-f50.google.com with SMTP id kx10so9032096pab.23 for ; Wed, 08 Oct 2014 07:03:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:subject:date:message-id:user-agent :mime-version:content-type; bh=xMpBloGmACiQUC5seojHB+6xxRty0TqQh9jUi+D7IFI=; b=CBa1qiS2wUkwr4pw5o4xfNmep7uDYzbIjnoR7Q8ct5oNmUjon8ZpxsvI+uXGTNgiiA SbiGeOW3c7btzhExUWaVP2kgjo1ZYpj9LDCeztAoDjBHXuyw9E2g5rjT5PBKJs63p8EE QT82/OJf1fIWK0N8xJJHg6O8JG58m+8wZQJMF7UdiFNgCuPTJGVZkq9wfya8wv/+0MqB ovrVr969LJLTd7TtfsJIfljeMh/EhSTyyXqaWRKxIGsgTDM2xipDalmnq/qUX262eUEW enCzVceyR6TCL6uL18j1P5fqriZ13CaozS3st+L3prh4BwPc2KwwiciYxrwYG3aZOaP/ 0+Tw== X-Gm-Message-State: ALoCoQlPN7bXtBaz+jTV03ZoRslDQVDMPPWfxmdSrmui4J13TRKenvO8r6y2JLAXDabrrdCCnNVO X-Received: by 10.70.3.194 with SMTP id e2mr2205968pde.161.1412776999913; Wed, 08 Oct 2014 07:03:19 -0700 (PDT) Received: from iant-glaptop.roam.corp.google.com.google.com ([207.198.105.20]) by mx.google.com with ESMTPSA id k9sm138553pdj.41.2014.10.08.07.03.17 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 08 Oct 2014 07:03:19 -0700 (PDT) From: Ian Lance Taylor To: gcc-patches@gcc.gnu.org, gofrontend-dev@googlegroups.com Subject: libgo patch committed: Fix recover.go test for large args, FFI Date: Wed, 08 Oct 2014 07:03:15 -0700 Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes This patch to libgo fixes PR 60406, in which a problem arises when passing a large argument to a deferred function. The current code generates a thunk that looks like this: if (__go_set_defer_retaddr (&&L)) goto L; deferred_function(args); L: Then the deferred_function calls __go_can_recover (__builtin_return_address()) and the code in libgo compares the address passed to __go_can_recover to the address passed to __go_set_defer_retaddr. This rather baroque approach is so that a defer of a variable of function type will work correctly. This code normally works fine. However, if args is very large, then it will cause GCC to generate a block copy. And on systems where that block copy introduces a loop into the code, the exact location of L may be split to a different block so that it no longer immediately follows the call to deferred_function. In that case the comparison in __go_can_recover will fail, and the call to recover will fail when it should succeed. This is an unusual case. Most calls to defer pass no arguments to the deferred function. It's quite unusual to pass a large argument. However, we should of course handle it correctly. This patch does this by adding new code when the return address fails to match. We fetch the callers to see whether we are being directly invoked as a deferred function. We detect this case by the simple approach of seeing whether the caller starts with "__go_". The patch also adjusts one case that calls a deferred function to start with __go_ to match this. This is a heuristic that is safe at present and be made more safe in the future. This patch also tightens up the checks for deferring a function created by reflect.MakeFunc, to make that case more reliable. This patch also introduces uses of __builtin_extract_return_addr for systems that need it, which permits us to remove a #ifdef __sparc__. Bootstrapped and ran Go testsuite on x86_64-unknown-linux-gnu and powerpc64le-unnown-linux-gnu. Tested by others on Alpha and S/390. Thanks to Dominik Vogt for forcing the issue and examining the underlying assumptions. Committed to mainline. Although this fixes a bug this seems to me to be too risky for the 4.9 branch, especially given that the bug has only ever been seen in the testsuite, and is extremely unlikely to arise in ordinary user code. Ian diff -r 50bdfe92960f libgo/go/reflect/makefunc_ffi_c.c --- a/libgo/go/reflect/makefunc_ffi_c.c Fri Oct 03 11:27:25 2014 -0700 +++ b/libgo/go/reflect/makefunc_ffi_c.c Wed Oct 08 06:45:51 2014 -0700 @@ -36,21 +36,23 @@ Go callback function (passed in user_data) with the pointer to the arguments and the results area. */ +static void ffi_callback (ffi_cif *, void *, void **, void *) + __asm__ ("reflect.ffi_callback"); + static void ffi_callback (ffi_cif* cif __attribute__ ((unused)), void *results, void **args, void *user_data) { - Location locs[6]; + Location locs[8]; int n; int i; - const void *pc; FuncVal *fv; void (*f) (void *, void *); /* This function is called from some series of FFI closure functions - called by a Go function. We want to pass the PC of the Go - function to makefunc_can_recover. Look up the stack for a - function that is definitely not an FFI function. */ + called by a Go function. We want to see whether the caller of + the closure functions can recover. Look up the stack and skip + the FFI functions. */ n = runtime_callers (1, &locs[0], sizeof locs / sizeof locs[0], true); for (i = 0; i < n; i++) { @@ -61,28 +63,19 @@ if (locs[i].function.len < 4) break; name = locs[i].function.str; - if (*name == '_') - { - if (locs[i].function.len < 5) - break; - ++name; - } if (name[0] != 'f' || name[1] != 'f' || name[2] != 'i' || name[3] != '_') break; } if (i < n) - pc = (const void *) locs[i].pc; - else - pc = __builtin_return_address (0); - - __go_makefunc_can_recover (pc); + __go_makefunc_ffi_can_recover (locs + i, n - i); fv = (FuncVal *) user_data; __go_set_closure (fv); f = (void *) fv->fn; f (args, results); - __go_makefunc_returning (); + if (i < n) + __go_makefunc_returning (); } /* Allocate an FFI closure and arrange to call ffi_callback. */ diff -r 50bdfe92960f libgo/runtime/go-defer.c --- a/libgo/runtime/go-defer.c Fri Oct 03 11:27:25 2014 -0700 +++ b/libgo/runtime/go-defer.c Wed Oct 08 06:45:51 2014 -0700 @@ -80,6 +80,6 @@ g = runtime_g (); if (g->defer != NULL) - g->defer->__retaddr = retaddr; + g->defer->__retaddr = __builtin_extract_return_addr (retaddr); return 0; } diff -r 50bdfe92960f libgo/runtime/go-panic.h --- a/libgo/runtime/go-panic.h Fri Oct 03 11:27:25 2014 -0700 +++ b/libgo/runtime/go-panic.h Wed Oct 08 06:45:51 2014 -0700 @@ -38,9 +38,12 @@ extern struct __go_empty_interface __go_recover (void); -extern _Bool __go_can_recover (const void *); +extern _Bool __go_can_recover (void *); -extern void __go_makefunc_can_recover (const void *retaddr); +extern void __go_makefunc_can_recover (void *retaddr); + +struct Location; +extern void __go_makefunc_ffi_can_recover (struct Location *, int); extern void __go_makefunc_returning (void); diff -r 50bdfe92960f libgo/runtime/go-recover.c --- a/libgo/runtime/go-recover.c Fri Oct 03 11:27:25 2014 -0700 +++ b/libgo/runtime/go-recover.c Wed Oct 08 06:45:51 2014 -0700 @@ -9,6 +9,36 @@ #include "go-panic.h" #include "go-defer.h" +/* If the top of the defer stack can be recovered, then return it. + Otherwise return NULL. */ + +static struct __go_defer_stack * +current_defer () +{ + G *g; + struct __go_defer_stack *d; + + g = runtime_g (); + + d = g->defer; + if (d == NULL) + return NULL; + + /* The panic which would be recovered is the one on the top of the + panic stack. We do not want to recover it if that panic was on + the top of the panic stack when this function was deferred. */ + if (d->__panic == g->panic) + return NULL; + + /* The deferred thunk will call _go_set_defer_retaddr. If this has + not happened, then we have not been called via defer, and we can + not recover. */ + if (d->__retaddr == NULL) + return NULL; + + return d; +} + /* This is called by a thunk to see if the real function should be permitted to recover a panic value. Recovering a value is permitted if the thunk was called directly by defer. RETADDR is @@ -16,79 +46,126 @@ __go_can_recover--this is, the thunk. */ _Bool -__go_can_recover (const void *retaddr) +__go_can_recover (void *retaddr) { - G *g; struct __go_defer_stack *d; const char* ret; const char* dret; - Location loc; + Location locs[16]; const byte *name; + intgo len; + int n; + int i; + _Bool found_ffi_callback; - g = runtime_g (); - - d = g->defer; + d = current_defer (); if (d == NULL) return 0; - /* The panic which this function would recover is the one on the top - of the panic stack. We do not want to recover it if that panic - was on the top of the panic stack when this function was - deferred. */ - if (d->__panic == g->panic) - return 0; - - /* D->__RETADDR is the address of a label immediately following the - call to the thunk. We can recover a panic if that is the same as - the return address of the thunk. We permit a bit of slack in - case there is any code between the function return and the label, - such as an instruction to adjust the stack pointer. */ - - ret = (const char *) retaddr; - -#ifdef __sparc__ - /* On SPARC the address we get, from __builtin_return_address, is - the address of the call instruction. Adjust forward, also - skipping the delayed instruction following the call. */ - ret += 8; -#endif + ret = (const char *) __builtin_extract_return_addr (retaddr); dret = (const char *) d->__retaddr; if (ret <= dret && ret + 16 >= dret) return 1; + /* On some systems, in some cases, the return address does not work + reliably. See http://gcc.gnu.org/PR60406. If we are permitted + to call recover, the call stack will look like this: + __go_panic, __go_undefer, etc. + thunk to call deferred function (calls __go_set_defer_retaddr) + function that calls __go_can_recover (passing return address) + __go_can_recover + Calling runtime_callers will skip the thunks. So if our caller's + caller starts with __go, then we are permitted to call + recover. */ + + if (runtime_callers (1, &locs[0], 2, false) < 2) + return 0; + + name = locs[1].function.str; + len = locs[1].function.len; + + /* Although locs[1].function is a Go string, we know it is + NUL-terminated. */ + if (len > 4 + && __builtin_strchr ((const char *) name, '.') == NULL + && __builtin_strncmp ((const char *) name, "__go_", 4) == 0) + return 1; + + /* If we are called from __go_makefunc_can_recover, then we need to + look one level higher. */ + if (locs[0].function.len > 0 + && __builtin_strcmp ((const char *) locs[0].function.str, + "__go_makefunc_can_recover") == 0) + { + if (runtime_callers (3, &locs[0], 1, false) < 1) + return 0; + name = locs[0].function.str; + len = locs[0].function.len; + if (len > 4 + && __builtin_strchr ((const char *) name, '.') == NULL + && __builtin_strncmp ((const char *) name, "__go_", 4) == 0) + return 1; + } + /* If the function calling recover was created by reflect.MakeFunc, - then RETADDR will be somewhere in libffi. Our caller is - permitted to recover if it was called from libffi. */ + then __go_makefunc_can_recover or __go_makefunc_ffi_can_recover + will have set the __makefunc_can_recover field. */ if (!d->__makefunc_can_recover) return 0; - if (runtime_callers (2, &loc, 1, false) < 1) - return 0; + /* We look up the stack, ignoring libffi functions and functions in + the reflect package, until we find reflect.makeFuncStub or + reflect.ffi_callback called by FFI functions. Then we check the + caller of that function. */ - /* If we have no function name, then we weren't called by Go code. - Guess that we were called by libffi. */ - if (loc.function.len == 0) - return 1; + n = runtime_callers (2, &locs[0], sizeof locs / sizeof locs[0], false); + found_ffi_callback = 0; + for (i = 0; i < n; i++) + { + const byte *name; - if (loc.function.len < 4) - return 0; - name = loc.function.str; - if (*name == '_') - { - if (loc.function.len < 5) - return 0; - ++name; + if (locs[i].function.len == 0) + { + /* No function name means this caller isn't Go code. Assume + that this is libffi. */ + continue; + } + + /* Ignore functions in libffi. */ + name = locs[i].function.str; + if (__builtin_strncmp ((const char *) name, "ffi_", 4) == 0) + continue; + + if (found_ffi_callback) + break; + + if (__builtin_strcmp ((const char *) name, "reflect.ffi_callback") == 0) + { + found_ffi_callback = 1; + continue; + } + + if (__builtin_strcmp ((const char *) name, "reflect.makeFuncStub") == 0) + { + i++; + break; + } + + /* Ignore other functions in the reflect package. */ + if (__builtin_strncmp ((const char *) name, "reflect.", 8) == 0) + continue; + + /* We should now be looking at the real caller. */ + break; } - if (name[0] == 'f' && name[1] == 'f' && name[2] == 'i' && name[3] == '_') - return 1; - - /* We may also be called by reflect.makeFuncImpl.call or - reflect.ffiCall, for a function created by reflect.MakeFunc. */ - if (__builtin_strstr ((const char *) name, "makeFuncImpl") != NULL - || __builtin_strcmp ((const char *) name, "reflect.ffiCall") == 0) - return 1; + if (i < n && locs[i].function.len > 0) + { + name = locs[i].function.str; + if (__builtin_strncmp ((const char *) name, "__go_", 4) == 0) + return 1; + } return 0; } @@ -99,14 +176,58 @@ real MakeFunc function is permitted to call recover. */ void -__go_makefunc_can_recover (const void *retaddr) +__go_makefunc_can_recover (void *retaddr) { struct __go_defer_stack *d; - d = runtime_g ()->defer; - if (d != NULL - && !d->__makefunc_can_recover - && __go_can_recover (retaddr)) + d = current_defer (); + if (d == NULL) + return; + + /* If we are already in a call stack of MakeFunc functions, there is + nothing we can usefully check here. */ + if (d->__makefunc_can_recover) + return; + + if (__go_can_recover (retaddr)) + d->__makefunc_can_recover = 1; +} + +/* This function is called when code is about to enter a function + created by the libffi version of reflect.MakeFunc. This function + is passed the names of the callers of the libffi code that called + the stub. It uses to decide whether it is permitted to call + recover, and sets d->__makefunc_can_recover so that __go_recover + can make the same decision. */ + +void +__go_makefunc_ffi_can_recover (struct Location *loc, int n) +{ + struct __go_defer_stack *d; + const byte *name; + intgo len; + + d = current_defer (); + if (d == NULL) + return; + + /* If we are already in a call stack of MakeFunc functions, there is + nothing we can usefully check here. */ + if (d->__makefunc_can_recover) + return; + + /* LOC points to the caller of our caller. That will be a thunk. + If its caller was a runtime function, then it was called directly + by defer. */ + + if (n < 2) + return; + + name = (loc + 1)->function.str; + len = (loc + 1)->function.len; + if (len > 4 + && __builtin_strchr ((const char *) name, '.') == NULL + && __builtin_strncmp ((const char *) name, "__go_", 4) == 0) d->__makefunc_can_recover = 1; } diff -r 50bdfe92960f libgo/runtime/panic.c --- a/libgo/runtime/panic.c Fri Oct 03 11:27:25 2014 -0700 +++ b/libgo/runtime/panic.c Wed Oct 08 06:45:51 2014 -0700 @@ -49,8 +49,10 @@ } // Run all deferred functions for the current goroutine. +// This is noinline for go_can_recover. +static void __go_rundefer (void) __attribute__ ((noinline)); static void -rundefer(void) +__go_rundefer(void) { G *g; Defer *d; @@ -219,7 +221,7 @@ void runtime_Goexit(void) { - rundefer(); + __go_rundefer(); runtime_goexit(); }