]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/link, runtime: Apple libc atfork workaround take 3
authorRuss Cox <rsc@golang.org>
Wed, 4 Jan 2023 14:21:14 +0000 (09:21 -0500)
committerGopher Robot <gobot@golang.org>
Tue, 10 Jan 2023 20:34:22 +0000 (20:34 +0000)
CL 451735 worked around bugs in Apple's atfork handlers by calling
notify_is_valid_token and xpc_atfork_child at startup, so that init
code that wouldn't be safe in the child process would be warmed up in
the parent process instead, but xpc_atfork_child broke use of the xpc
library in Go programs, and xpc is internally used by various macOS
frameworks (#57263).

CL 459175 reverted that change, and then CL 459176 tried a new
approach: use __fork, which doesn't call any of the atfork handlers at all.
That worked, but an Apple engineer reviewing the change in private
email suggests that since __fork is not public API, it should be avoided.
The same engineer (with access to the source code for the xpc library)
suggests that the breakage in #57263 is caused by xpc_atfork_child
marking the library as unusable, expecting an imminent call to exec,
and that calling xpc_date_create_from_current instead would do the
necessary initialization without marking xpc as unusable.

CL 460475 reverted that change, to prepare for this one.

This CL goes back to the original “call functions to warm things up”
approach, replacing xpc_atfork_child with xpc_date_create_from_current.

The CL also updates cmd/link to use OS and SDK version 10.13.0 for
x86 macOS binaries, up from 10.9.0, also suggested by the Apple engineer.
Combined with the two warmup calls, this makes the fork hangs go away.
The minimum macOS version has been 10.13 High Sierra since Go 1.17,
so there should be no problem with writing that in the binaries too.

Fixes #33565.
Fixes #56784.
Fixes #57263.
Fixes #57577.

Change-Id: I20769d9daa1fe9ea930f8009481335f8a14dc21b
Reviewed-on: https://go-review.googlesource.com/c/go/+/460476
Auto-Submit: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
src/cmd/link/internal/ld/macho.go
src/runtime/os_darwin.go
src/runtime/sys_darwin.go
src/runtime/sys_darwin_amd64.s
src/runtime/sys_darwin_arm64.s

index 11cbf814861b5d093ce8517f184666a4447063b5..d6c28e4790ad7d5e2f6dedf61ce73335b98e980c 100644 (file)
@@ -479,8 +479,11 @@ func (ctxt *Link) domacho() {
                        var version uint32
                        switch ctxt.Arch.Family {
                        case sys.AMD64:
-                               // The version must be at least 10.9; see golang.org/issues/30488.
-                               version = 10<<16 | 9<<8 | 0<<0 // 10.9.0
+                               // This must be fairly recent for Apple signing (go.dev/issue/30488).
+                               // Having too old a version here was also implicated in some problems
+                               // calling into macOS libraries (go.dev/issue/56784).
+                               // In general this can be the most recent supported macOS version.
+                               version = 10<<16 | 13<<8 | 0<<0 // 10.13.0
                        case sys.ARM64:
                                version = 11<<16 | 0<<8 | 0<<0 // 11.0.0
                        }
index af5c18c30195c1b70c4aa173c79972b709d34789..c4f3bb6a818585ca78284c39cc4011fbeaabe141 100644 (file)
@@ -136,6 +136,8 @@ func osinit() {
 
        ncpu = getncpu()
        physPageSize = getPageSize()
+
+       osinit_hack()
 }
 
 func sysctlbynameInt32(name []byte) (int32, int32) {
index 61b7f8c728ffab4197c9c60ed09a2303d654e7e2..8bff695f5729d8d29459bf4772615f8768967260 100644 (file)
@@ -179,6 +179,45 @@ func pthread_kill(t pthread, sig uint32) {
 }
 func pthread_kill_trampoline()
 
+// osinit_hack is a clumsy hack to work around Apple libc bugs
+// causing fork+exec to hang in the child process intermittently.
+// See go.dev/issue/33565 and go.dev/issue/56784 for a few reports.
+//
+// The stacks obtained from the hung child processes are in
+// libSystem_atfork_child, which is supposed to reinitialize various
+// parts of the C library in the new process.
+//
+// One common stack dies in _notify_fork_child calling _notify_globals
+// (inlined) calling _os_alloc_once, because _os_alloc_once detects that
+// the once lock is held by the parent process and then calls
+// _os_once_gate_corruption_abort. The allocation is setting up the
+// globals for the notification subsystem. See the source code at [1].
+// To work around this, we can allocate the globals earlier in the Go
+// program's lifetime, before any execs are involved, by calling any
+// notify routine that is exported, calls _notify_globals, and doesn't do
+// anything too expensive otherwise. notify_is_valid_token(0) fits the bill.
+//
+// The other common stack dies in xpc_atfork_child calling
+// _objc_msgSend_uncached which ends up in
+// WAITING_FOR_ANOTHER_THREAD_TO_FINISH_CALLING_+initialize. Of course,
+// whatever thread the child is waiting for is in the parent process and
+// is not going to finish anything in the child process. There is no
+// public source code for these routines, so it is unclear exactly what
+// the problem is. An Apple engineer suggests using xpc_date_create_from_current,
+// which empirically does fix the problem.
+//
+// So osinit_hack_trampoline (in sys_darwin_$GOARCH.s) calls
+// notify_is_valid_token(0) and xpc_date_create_from_current(), which makes the
+// fork+exec hangs stop happening. If Apple fixes the libc bug in
+// some future version of macOS, then we can remove this awful code.
+//
+//go:nosplit
+func osinit_hack() {
+       libcCall(unsafe.Pointer(abi.FuncPCABI0(osinit_hack_trampoline)), nil)
+       return
+}
+func osinit_hack_trampoline()
+
 // mmap is used to do low-level memory allocation via mmap. Don't allow stack
 // splits, since this function (used by sysAlloc) is called in a lot of low-level
 // parts of the runtime and callers often assume it won't acquire any locks.
@@ -548,3 +587,6 @@ func setNonblock(fd int32) {
 //go:cgo_import_dynamic libc_pthread_cond_wait pthread_cond_wait "/usr/lib/libSystem.B.dylib"
 //go:cgo_import_dynamic libc_pthread_cond_timedwait_relative_np pthread_cond_timedwait_relative_np "/usr/lib/libSystem.B.dylib"
 //go:cgo_import_dynamic libc_pthread_cond_signal pthread_cond_signal "/usr/lib/libSystem.B.dylib"
+
+//go:cgo_import_dynamic libc_notify_is_valid_token notify_is_valid_token "/usr/lib/libSystem.B.dylib"
+//go:cgo_import_dynamic libc_xpc_date_create_from_current xpc_date_create_from_current "/usr/lib/libSystem.B.dylib"
index 369b12e8f9f86d86b3b9f63d432c5abaa01f9a11..6eaeeb915f7e242037c7274c6a08e093c05f3c21 100644 (file)
@@ -597,6 +597,15 @@ TEXT runtime·pthread_kill_trampoline(SB),NOSPLIT,$0
        POPQ    BP
        RET
 
+TEXT runtime·osinit_hack_trampoline(SB),NOSPLIT,$0
+       PUSHQ   BP
+       MOVQ    SP, BP
+       MOVQ    $0, DI  // arg 1 val
+       CALL    libc_notify_is_valid_token(SB)
+       CALL    libc_xpc_date_create_from_current(SB)
+       POPQ    BP
+       RET
+
 // syscall calls a function in libc on behalf of the syscall package.
 // syscall takes a pointer to a struct like:
 // struct {
index 4fa99cc0f98cff8d8f02b9a8eb446239aeb67e47..4a51fb3a86349a85a976d4bce8c439f0c510366c 100644 (file)
@@ -458,6 +458,12 @@ TEXT runtime·pthread_setspecific_trampoline(SB),NOSPLIT,$0
        BL      libc_pthread_setspecific(SB)
        RET
 
+TEXT runtime·osinit_hack_trampoline(SB),NOSPLIT,$0
+       MOVD    $0, R0  // arg 1 val
+       BL      libc_notify_is_valid_token(SB)
+       BL      libc_xpc_date_create_from_current(SB)
+       RET
+
 // syscall calls a function in libc on behalf of the syscall package.
 // syscall takes a pointer to a struct like:
 // struct {