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

syscall: Syscall6 not implemented on Solaris #24357

Open
gen2brain opened this issue Mar 12, 2018 · 18 comments
Open

syscall: Syscall6 not implemented on Solaris #24357

gen2brain opened this issue Mar 12, 2018 · 18 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Solaris
Milestone

Comments

@gen2brain
Copy link

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.10 linux/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/milann/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/milann/golang"
GORACE=""
GOROOT="/home/milann/go"
GOTMPDIR=""
GOTOOLDIR="/home/milann/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build031994401=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I am trying to call shmget, shmat etc. via syscall. On OpenSolaris there is one code for SHM (52) and then shmget etc. functions are sub calls with codes 0,1,2 etc.

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

package main

import (
	"runtime"
	"syscall"
)

const (
	IPC_PRIVATE = 0
	IPC_CREAT   = 01000
)

func main() {
	if runtime.GOOS == "solaris" {
		id, _, errno := syscall.Syscall6(52, 3, uintptr(int32(IPC_PRIVATE)), uintptr(int32(65536)), uintptr(int32(IPC_CREAT|0777)), 0, 0)
		if int(id) == -1 {
			println(errno)
		}
	} else if runtime.GOOS == "linux" {
                // 29 is syscall.SYS_SHMGET on amd64, missing on solaris
		id, _, errno := syscall.Syscall(29, uintptr(int32(IPC_PRIVATE)), uintptr(int32(65536)), uintptr(int32(IPC_CREAT|0777)))
		if int(id) == -1 {
			println(errno)
		}
	}
}

What did you expect to see?

Program compiles ok with GOOS=solaris.

What did you see instead?

GOOS=solaris go build test.go 
# command-line-arguments
syscall.Syscall6: call to external function
main.main: relocation target syscall.Syscall6 not defined
main.main: undefined: "syscall.Syscall6"

Is this something you are aware of and is there a workaround maybe? For now I also implemented cgo version but I hope just temporary. You can also test with library I am working on:
GOOS=solaris go get -tags syscall github.com/gen2brain/shm

@andybons andybons added OS-Solaris NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 13, 2018
@andybons
Copy link
Member

@ianlancetaylor

@andybons andybons added this to the Go1.11 milestone Mar 13, 2018
@ianlancetaylor ianlancetaylor changed the title cmd/link: syscall.Syscall6: call to external function on Solaris syscall: Syscall6 not implemented on Solaris Mar 13, 2018
@ianlancetaylor ianlancetaylor added NeedsFix The path to resolution is known, but the work has not been done. help wanted labels Mar 13, 2018
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 13, 2018
@ianlancetaylor
Copy link
Contributor

Someone just needs to implement syscall.Syscall6 for Solaris. syscall.Syscall already exists, but for some reason Syscall6 is missing. RawSyscall6 is also missing. And they are also missing from the x/sys/unix package.

@tklauser
Copy link
Member

/cc @binarycrusader @4ad

I think syscalls are a bit different on Solaris and shouldn't be used directly. The comment above runtime.syscall_syscall (which is called by syscall.Syscall) says:

// This is syscall.Syscall, it exists to satisfy some build dependency,
// but it doesn't work correctly.
//
// DO NOT USE!
//
// TODO(aram): make this panic once we stop calling fcntl(2) in net using it.

Also, syscall.RawSyscall calls runtime.syscall_rawsyscall on Solaris which just panics (see #20833), so I guess we shouldn't add RawSyscall6 (or just an implementation that panics as well).

So I think the proper solution here would be to implement Shmget in x/sys/unix and then use that.

@4ad
Copy link
Member

4ad commented Mar 14, 2018

syscall.Syscall is safe to use, that comment is obsolete. It predates the use of libc by the Solaris port. It's illegal to issue system calls directly, but we're just calling libc. syscall.RawSyscall however is not supported.

However, if you want Shmget, you need to implement it in x/sys/unix and use that, though SYS_semsys should work too.

@ianlancetaylor
Copy link
Contributor

I think we should an implementation of RawSyscall6 that panics. The function is supposed to exist even if it is not called. It ought to be possible to write code as in the original issue that links even if RawSyscall6 is never called, by run time checks of GOOS (even if it would probably be better to use build tags).

@4ad
Copy link
Member

4ad commented Mar 14, 2018

Why do we need syscall.RawSyscall6 for Solaris? The standard library doesn't need it. Why needlessly add it?

I agree however that the "6" variants need to be added to x/sys/unix. The original code should use x/sys/unix instead of syscall. I don't see why it's desired to normalize the symbols exported by syscall across operating systems, even though they don't work on every operating system.

As posted, the code would fail to compile on Plan 9, nacl, and possibly Windows too (unsure). That's ok, the author should be using build tags. Why is Solaris different than Plan 9, nacl or Windows?

@ianlancetaylor
Copy link
Contributor

The problem now is that the syscall package currently declares RawSyscall6 but does not define it. So it's already there. Code that calls it will compile, but will not link. In fact, worse, it will compile but whether it links or not depends on what compiler optimizations are applied.

Had RawSyscall6 never been declared for Solaris, then I think it would be fine to continue to leave it out. But the current situation seems bad, and I think that defining that function so that links succeed would be the better fix.

It's not a big deal, though.

@4ad
Copy link
Member

4ad commented Mar 14, 2018

You're right, it's declared by mistake. This is all my fault.

When I wrote the Solaris port I intended the syscall package to only support the standard library, unlike the other Unix ports which export all kinds of functionality. Evidently, I failed.

In this light I think defining all the declared, but undefined functions and making them panic is a good idea.

@ianlancetaylor
Copy link
Contributor

Don't beat yourself up, it's a very easy mistake to make the way the code is written currently.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/100655 mentions this issue: runtime, syscall: add RawSyscall6 on Solaris and make it panic

@tklauser
Copy link
Member

Thanks @4ad and @ianlancetaylor for the clarification. I sent https://golang.org/cl/100655 which adds a panicing RawSyscall6 and removes the outdated comment above runtime.syscall_syscall.

gopherbot pushed a commit that referenced this issue Mar 14, 2018
The syscall package currently declares RawSyscall6 for every GOOS, but
does not define it on Solaris. This leads to code using said function
to compile but it will not link. Fix it by adding RawSyscall6 and make
it panic.

Also remove the obsolete comment above runtime.syscall_syscall as
pointed out by Aram.

Updates #24357

Change-Id: I1b1423121d1c99de2ecc61cd9a935dba9b39e3a4
Reviewed-on: https://go-review.googlesource.com/100655
Reviewed-by: Aram Hăvărneanu <aram@mgk.ro>
@gen2brain
Copy link
Author

Thanks. So I should use for now cgo on solaris, if implemented, functions will be in x/sys/unix right?

And what about freebsd, it doesn't have syscall.SYS_SHMGET and other related consts defined, I defined them in my lib but I somehow expected to find this defined in all unixes. Should that be in standard library?

@4ad
Copy link
Member

4ad commented Mar 14, 2018

Until the XSI shared memory functions are added to x/sys/unix you can use cgo, yes. But you should consider adding these functions to x/sys/unix. It's relatively straightforward.

As for FreeBSD, and any other Unix system, these functions should also go in x/sys/unix. All low-level syscall-like interfaces should go there. It's unfortunate we expose so much in syscall, but there's too late to change anything now. The syscall package predates internal Go packages by many years.

@gen2brain
Copy link
Author

Ok, for functions I understand, but I meant syscall.SYS_SHMGET is defined for example on dragonfly, netbsd and openbsd but is missing on freebsd, is just that one allowed to be added to standard library.
It does work when defined, solaris is the only one that works totally differently, others just have different structs.

@tklauser
Copy link
Member

The syscall package is frozen and no new public API should be added (except for some special casses where it is required by Go itself) as per:

// NOTE: This package is locked down. Code outside the standard
// Go repository should be migrated to use the corresponding
// package in the golang.org/x/sys repository. That is also where updates
// required by new systems or versions should be applied.
// Signal, Errno and SysProcAttr are not yet available in
// golang.org/x/sys and must still be referenced from the
// syscall package. See https://golang.org/s/go1.4-syscall
// for more information.

Instead, the golang.org/x/sys package should be used instead of syscall by all code outside the standard Go library. New functions, constants (such as SYS_SHMGET on FreeBSD) etc. will be added and kept updated there.

@4ad
Copy link
Member

4ad commented Mar 14, 2018

@gen2brain It's totally fine to define some constant that exists only on some platform, though in this case note that you could just define generic shmget(2)-like functions that just use the underlying mechanism (shmget(2) on Solaris, SYS_SHMGET on some other platforms, etc), instead of exposing the lower-level mechanism itself. That way the calling code could stay portable and not care about the differences between platforms.

@tklauser
Copy link
Member

Sorry, somehow I misread Ian's comment at #24357 (comment) and didn't add syscall.Syscall6 in https://golang.org/cl/100655. I'll send a follow-up CL, adding it as well and fixing the actual bug that was reported.

This will then also allow to add Syscall and Syscall6 to x/sys/unix, calling their respective counterparts in syscall.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/101135 mentions this issue: runtime, syscall: add Syscall6 on Solaris

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Solaris
Projects
None yet
Development

No branches or pull requests

6 participants