compiler, runtime: apply review feedback for darwin cgo import lowering

Shorten the new comment blocks and write them in Simplified Technical
English. Give the reason for the manual ioctl declaration. Change
syscall_libc_open to the uintptr_t parameter style of the other
wrappers. Add t.Parallel and llvm.VerifyModule to
TestDarwinCgoImportDynamic.

Author: neomantra

compiler/compiler_test.go

@@ -353,6 +353,8 @@ func TestValidateWasmFunctionParameters(t *testing.T) {
 }
 
 func TestDarwinCgoImportDynamic(t *testing.T) {
+	t.Parallel()
+
 	options := &compileopts.Options{GOOS: "darwin", GOARCH: "arm64"}
 	mod, errs := testCompilePackage(t, options, "cgo-import-dynamic.go")
 	if len(errs) != 0 {
@@ -363,6 +365,10 @@ func TestDarwinCgoImportDynamic(t *testing.T) {
 	}
 	defer mod.Dispose()
 
+	if err := llvm.VerifyModule(mod, llvm.ReturnStatusAction); err != nil {
+		t.Fatal(err)
+	}
+
 	ir := mod.String()
 	if !strings.Contains(ir, `declare void @"remote$INODE64"()`) {
 		t.Error("missing external declaration for cgo_import_dynamic remote symbol")

compiler/symbol.go

@@ -774,19 +774,8 @@ func (c *compilerContext) fileForFunc(f *ssa.Function) *ast.File {
 // declaration and apply to the whole package.
 func (c *compilerContext) loadASTComments(pkg *loader.Package) {
 	for _, file := range pkg.Files {
-		// cgo_import_dynamic directives are file-level pragmas. Darwin's
-		// generated syscall wrappers use the local symbol to name an assembly
-		// trampoline and the remote symbol to name the actual dylib function.
-		// Like the gc compiler, accept all three operand forms:
-		//
-		//	//go:cgo_import_dynamic local [remote ["library"]]
-		//
-		// The remote symbol defaults to the local symbol when omitted. The
-		// library operand is not needed here (the linker resolves the symbol
-		// against the libraries it already links) and is ignored, so a library
-		// path containing spaces does not break parsing. A repeated local
-		// symbol keeps the last remote symbol, matching gc's behavior of
-		// simply recording each directive.
+		// Collect //go:cgo_import_dynamic local [remote ["library"]] directives.
+		// The remote symbol defaults to local. The library operand is ignored.
 		for _, group := range file.Comments {
 			for _, comment := range group.List {
 				parts := strings.Fields(comment.Text)

compiler/syscall.go

@@ -525,8 +525,8 @@ func (b *builder) createDarwinFuncPCABI0Call(instr *ssa.CallCommon) llvm.Value {
 	// Extract the libc function name.
 	name := strings.TrimPrefix(strings.TrimSuffix(calledFn.Name(), "_trampoline"), "libc_")
 	if wrapper, ok := darwinVariadicImports[name]; ok {
-		// Variadic functions can't be called like a regular function, so use a
-		// wrapper implemented in C. See the comment on darwinVariadicImports.
+		// A variadic function does not use the standard calling convention.
+		// Use its C wrapper. See darwinVariadicImports.
 		name = wrapper
 	}
 	if b.GOARCH == "amd64" {
@@ -541,25 +541,8 @@ func (b *builder) createDarwinFuncPCABI0Call(instr *ssa.CallCommon) llvm.Value {
 	return b.createDarwinImportedFunctionAddr(name)
 }
 
-// darwinVariadicImports maps the variadic libc functions imported by Darwin
-// syscall wrappers to fixed-signature C wrappers defined in
-// src/runtime/os_darwin.c. The syscall engine calls an imported address
-// through a fixed-signature function pointer (tinygo_syscallX and friends in
-// src/runtime/os_darwin.c), which passes every argument in a register. A
-// variadic callee, however, takes its variadic arguments from the stack on
-// darwin/arm64, so calling one of these functions directly makes it read
-// garbage arguments (a direct ioctl call observably failed with EFAULT, and
-// a direct fcntl(F_SETFL) wrote garbage flags). This applies to both
-// trampoline flavors: the standard library's function-based pattern
-// (createDarwinFuncPCABI0Call above) and the address-global pattern used by
-// golang.org/x/sys (createDarwinCgoImportDynamicLoad below).
-//
-// The set comes from cross-referencing the symbols that darwin's generated
-// syscall wrappers import (the //go:cgo_import_dynamic directives in
-// zsyscall_darwin_*.go, both in golang.org/x/sys/unix and in the standard
-// library) against their Darwin SDK declarations: of those imports, exactly
-// open(2), openat(2), fcntl(2), and ioctl(2) are declared variadic (see
-// sys/fcntl.h and sys/ioctl.h, or lib/macos-minimal-sdk's copies).
+// darwinVariadicImports maps each variadic libc import to a fixed-signature C
+// wrapper in src/runtime/os_darwin.c. See sys/fcntl.h and sys/ioctl.h.
 var darwinVariadicImports = map[string]string{
 	"fcntl":  "syscall_libc_fcntl",
 	"ioctl":  "syscall_libc_ioctl",

src/runtime/os_darwin.c

@@ -4,44 +4,29 @@
 
 #include <fcntl.h>
 
+// sys/ioctl.h is not in lib/macos-minimal-sdk. Declare ioctl here.
 extern int ioctl(int fd, unsigned long request, ...);
 
-// Wrapper function because 'open' is a variadic function and variadic functions
-// use a different (incompatible) calling convention on darwin/arm64.
-// This function is referenced from the compiler, when it sees a
-// syscall.libc_open_trampoline function.
-int syscall_libc_open(const char *pathname, int flags, mode_t mode) {
-    return open(pathname, flags, mode);
+// Fixed-signature wrappers for the variadic libc imports. Variadic functions
+// take stack arguments on darwin/arm64. See darwinVariadicImports in
+// compiler/syscall.go.
+
+int syscall_libc_open(uintptr_t pathname, uintptr_t flags, uintptr_t mode) {
+    return open((const char *)pathname, (int)flags, (mode_t)mode);
 }
 
-// Wrapper for ioctl, which is variadic just like open and therefore also uses
-// an incompatible calling convention on darwin/arm64. Use uintptr_t arguments
-// to match the fixed-signature call made by tinygo_syscall below.
 int syscall_libc_ioctl(uintptr_t fd, uintptr_t request, uintptr_t arg) {
-    return ioctl((int)fd, request, (void *)arg);
+    return ioctl((int)fd, (unsigned long)request, (void *)arg);
 }
 
-// Wrappers for the remaining variadic libc functions that darwin syscall
-// wrappers import with //go:cgo_import_dynamic (see darwinVariadicImports in
-// compiler/syscall.go): of the symbols imported by the generated
-// zsyscall_darwin_*.go files in golang.org/x/sys/unix and the standard
-// library, exactly open, openat, fcntl, and ioctl are declared variadic in
-// the Darwin SDK headers (sys/fcntl.h and sys/ioctl.h). The tinygo_syscall*
-// functions below call through fixed-signature function pointers that pass
-// every argument in a register, while a variadic callee takes its variadic
-// arguments from the stack on darwin/arm64, so each of these needs a
-// fixed-signature wrapper. The uintptr_t parameters match the uintptr
-// arguments the Go syscall engine passes.
-
-// fcntl's third argument is an int for some commands and a pointer for
-// others; passing the raw pointer-sized value covers both.
+// The third fcntl argument is an int for some commands and a pointer for
+// other commands. The raw pointer-sized value is correct for both.
 int syscall_libc_fcntl(uintptr_t fd, uintptr_t cmd, uintptr_t arg) {
     return fcntl((int)fd, (int)cmd, (void *)arg);
 }
 
-// openat is invoked by x/sys through syscall6 with six arguments, the last
-// two of which are zero padding; the two extra register arguments are
-// harmless to a four-parameter callee.
+// x/sys calls openat through syscall6 with two trailing zero arguments. The
+// two extra register arguments are harmless to a four-parameter callee.
 int syscall_libc_openat(uintptr_t dirfd, uintptr_t pathname, uintptr_t flags, uintptr_t mode) {
     return openat((int)dirfd, (const char *)pathname, (int)flags, (mode_t)mode);
 }