Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
nshalman opened this issue Mar 18, 2021 · 3 comments
Closed

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

nshalman opened this issue Mar 18, 2021 · 3 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. OS-Solaris
Milestone
Unreleased

Comments

@nshalman
Copy link

nshalman commented Mar 18, 2021 •
edited

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 gopherbot added this to the Unreleased milestone Mar 18, 2021
nshalman added a commit to nshalman/sys that referenced this issue Mar 18, 2021
@nshalman
unix: make mksyscall_solaris deduplicate
c47c08c 
Fixes golang/go#45097

Change-Id: I043f7a90d58767df68388f60c4cc7dc34c5baea1
@nshalman nshalman mentioned this issue Mar 18, 2021
Closed
@gopherbot
Copy link

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

@tklauser
Copy link
Member

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.

@tklauser tklauser added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. OS-Solaris labels Mar 18, 2021
@tklauser tklauser changed the title x/sys: unix/mksyscall_solaris.go doesn't deduplicate so libc functions can only be referenced once x/sys/unix: mksyscall_solaris.go doesn't deduplicate so libc functions can only be referenced once Mar 18, 2021
@nshalman
Copy link
Author

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.

@golang golang locked and limited conversation to collaborators Mar 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. OS-Solaris
Projects
None yet
Milestone
Unreleased
Development

Successfully merging a pull request may close this issue.

unix: make mksyscall_solaris deduplicate nshalman/sys
3 participants
@nshalman @tklauser @gopherbot