x/sys/unix: mksyscall_solaris.go doesn't deduplicate so libc functions can only be referenced once

[nshalman]

This is to document a design decision within a series of commits I will be submitting for review, but I want to lay out the rationale behind the approach for posterity. See golang/sys#102 / https://golang.org/cl/302310 for additional context.

Summary: In order to link to libc function calls more than once in syscall_{solaris,illumos} we need to prevent duplication in the resulting cgo import directives, linkname directives, and variable declarations.

Details:

unix/sycall_solaris.go already contains the following line:

//sys   ioctl(fd int, req uint, arg uintptr) (err error)

so the ioctl wrapper for the libc ioctl function is currently written to only return an error, however the C function signature is (from the man page) is:

int ioctl(int fildes, int request, /* arg */ ...);

I am working on WireGuard/wireguard-go#39 and have the need to perform some ioctls where the return value is needed.
If I attempt to add the following line to the file:

//sys  ioctlRet(fd int, req uint, arg uintptr) (ret int, err error) = libc.ioctl

And then rerun the regeneration command for unix/zsyscall_solaris_amd64.go I end up with, among other bits of the diff, these hunks (using -U1 to be concise):

diff --git a/unix/zsyscall_solaris_amd64.go b/unix/zsyscall_solaris_amd64.go
index 7099f55..0d7beab 100644
--- a/unix/zsyscall_solaris_amd64.go
+++ b/unix/zsyscall_solaris_amd64.go
@@ -33,2 +33,3 @@ import (
 //go:cgo_import_dynamic libc_ioctl ioctl "libc.so"
+//go:cgo_import_dynamic libc_ioctl ioctl "libc.so"
 //go:cgo_import_dynamic libc_poll poll "libc.so"
@@ -164,2 +165,3 @@ import (
 //go:linkname procioctl libc_ioctl
+//go:linkname procioctl libc_ioctl
 //go:linkname procpoll libc_poll
@@ -296,2 +298,3 @@ var (
        procioctl,
+       procioctl,
        procpoll,

Note the doubling. The compiler certainly does:

$ go build
# golang.org/x/sys/unix
./zsyscall_solaris_amd64.go:299:2: procioctl redeclared in this block
        previous declaration at ./zsyscall_solaris_amd64.go:279:2

If we accept that wanting to create this second wrapper for ioctl is reasonable, then we need mksyscall_solaris.go to keep track and not create duplication. Originally I wrote a version that does a very naive tracking of the previously seen values so that consecutive lines could be processed correctly, but a reviewer suggested that using maps would make it safe to do this anywhere in the file.

Possible alternatives:
Replace

//sys  ioctl(fd int, req uint, arg uintptr) (err error)

with e.g.

//sys  ioctl_ret(fd int, req uint, arg uintptr) (ret int, err error) = libc.ioctl

func ioctl(fd int, req uint, arg uintptr) error {
       _, err := ioctl_ret(fd, req, arg)
       return err
}

This on its face is nice and simple, but it means that all existing codepaths dependent on ioctl suddenly have an extra function call through ioctl_ret (or whatever we name it, this is verbatim from an older attempt). This seems like not a nice thing to do to existing code.

Another alternative would be to hardcode the generated code for ioctlRet into unix/syscall_illumos.go alongside its consumers. Probably fine, but feels inelegant.

One final note:
None of this solves the problem that moving

//sys  ioctlRet(fd int, req uint, arg uintptr) (ret int, err error) = libc.ioctl

into unix/syscall_illumos.go will cause a similar redeclaration problem because the code generating unix/zsyscall_illumos.go has no insight into the content of unix/{,z}syscall_solaris.go

Thank you, dear reader, for joining me down here in this sadness.

[gopherbot]

Change https://golang.org/cl/302310 mentions this issue: unix: make mksyscall_solaris deduplicate

[tklauser]

Thank you @nshalman for taking the time to elaborate on this and writing this issue in such detail!

Possible alternatives:
Replace

//sys  ioctl(fd int, req uint, arg uintptr) (err error)

with e.g.

//sys  ioctl_ret(fd int, req uint, arg uintptr) (ret int, err error) = libc.ioctl

func ioctl(fd int, req uint, arg uintptr) error {
       _, err := ioctl_ret(fd, req, arg)
       return err
}

This on its face is nice and simple, but it means that all existing codepaths dependent on ioctl suddenly have an extra function call through ioctl_ret (or whatever we name it, this is verbatim from an older attempt). This seems like not a nice thing to do to existing code.

If this is only about introducing ioctl_ret - and I don't think the case of redeclaring a syscall wrapper will be very common looking forward - I think this alternative is preferable to the deduping/reordering approach. It's much more straight-forward and requires fewer code changes. I'd assume the compiler to be intelligent enough to inline ioctl, so this shouldn't have a big downside. And even if this isn't inlined, I'd assume the overhead introduced by the function call to be neglectable compared to the actual libc function call. We've been re-wrapping functions in the x/sys/unix package before, so this is standard practice.

I'd be interested in @ianlancetaylor's opinion on this though.

[nshalman]

If this is only about introducing ioctl_ret - and I don't think the case of redeclaring a syscall wrapper will be very common looking forward - I think this alternative is preferable to the deduping/reordering approach. It's much more straight-forward and requires fewer code changes. I'd assume the compiler to be intelligent enough to inline ioctl, so this shouldn't have a big downside. And even if this isn't inlined, I'd assume the overhead introduced by the function call to be neglectable compared to the actual libc function call. We've been re-wrapping functions in the x/sys/unix package before, so this is standard practice.

I'd be interested in @ianlancetaylor's opinion on this though.

Well, that was @danderson's original approach and I should have just gone with it. With my original clunky deduplication code it felt like a less invasive change to the actual {,z}syscall_solaris files, but if these sorts of wrappers are common and the compiler does the right thing, let's just do that after all.

This has all been very educational for me, and I thank all the reviewers for their patience with a new contributor. I'll go clean up my branch and update gerrit.