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, x/sys/unix: Time(), Utime(), Getrlimit() use invalid syscall number on linux/arm #14524

Closed
hirochachacha opened this issue Feb 26, 2016 · 25 comments
Milestone

Comments

@hirochachacha
Copy link
Contributor

@hirochachacha hirochachacha commented Feb 26, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    any
  2. What operating system and processor architecture are you using (go env)?
    linux/arm
  3. What did you do?
    (Use play.golang.org to provide a runnable example, if possible.)
    nothing
  4. What did you expect to see?
    invalid syscall numbers are not used
  5. What did you see instead?
    invalid syscall numbers are used

As far as is known, linux/arm sycalls are slightly limited.
Here is the list taken from kernel header:

/*
 * The following syscalls are obsolete and no longer available for EABI.
 */
#if !defined(__KERNEL__)
#if defined(__ARM_EABI__)
#undef __NR_time
#undef __NR_umount
#undef __NR_stime
#undef __NR_alarm
#undef __NR_utime
#undef __NR_getrlimit
#undef __NR_select
#undef __NR_readdir
#undef __NR_mmap
#undef __NR_socketcall
#undef __NR_syscall
#undef __NR_ipc
#endif
#endif

I confirmed time, utime and getrlimit are used, others are not used in both syscall package and x/sys/unix package.

I also suggest such syscall numbers should be removed from these packages on linux/arm.
(except syscall package? because of compatibility promise)

Thank you.

@hirochachacha hirochachacha changed the title syscall, x/sys/unix: Time(), Utime(), Getrlimit() use invalid syscall number syscall, x/sys/unix: Time(), Utime(), Getrlimit() use invalid syscall number on linux/arm Feb 26, 2016
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Feb 26, 2016

Please show a complete program which misbehaves, and the strace -f ./yourbinary output when run on linux/arm. Thanks.

@hirochachacha
Copy link
Contributor Author

@hirochachacha hirochachacha commented Feb 26, 2016

Actually, I found this by just reading source code. I have no accessible linux/arm here.
You are right. I should have tried it before filing as a issue. Sorry.
I'll try it later by QEMU or something. Thank you for your reply.

@bradfitz bradfitz closed this Feb 26, 2016
@hirochachacha
Copy link
Contributor Author

@hirochachacha hirochachacha commented Feb 26, 2016

Input:

package main

import (
    "syscall"
)
func main() {
    t := new(syscall.Time_t)

    _, err := syscall.Time(t)
    if err != nil {
        println(err.Error())
    }
}

Output:

function not implemented

As I expected, linux return ENOSYS. is it enough info?

@hirochachacha
Copy link
Contributor Author

@hirochachacha hirochachacha commented Feb 26, 2016

For the record, root of the problem is code-generation scripts couldn't recognize #undef.
We should fix such scripts and hope nothing changes after re-generation.

@davecheney
Copy link
Contributor

@davecheney davecheney commented Feb 26, 2016

@hirochachacha the syscall package is a mismash of incompatible implementations. The go runtime does not use syscall.Time, it uses an implementation of nanotime(2), although that depends on the platform you are calling it on.

If there is a fix that needs to be made, it will be in the x/sys/unix package, the syscall package is closed for modifications.

@hirochachacha
Copy link
Contributor Author

@hirochachacha hirochachacha commented Feb 27, 2016

the syscall package is a mismash of incompatible implementations. The go runtime does not use syscall.Time, it uses an implementation of nanotime(2), although that depends on the platform you are calling it on.

I see. And also runtime's getrlimit is implemented by ugetrlimit.
Yes, runtime's implementation looks like more sophisticated and well tested than syscall's one to me.

If there is a fix that needs to be made, it will be in the x/sys/unix package, the syscall package is closed for modifications.

Fair enough.

By the way, do you want to accept such a trivial bug report?
I know you guys work for more important issues everyday.
Maybe you don't want to spare time for trivial one.
Maybe you don't want to see the issue tracker is filled by trivial ones.
I don't want annoy you. That's not my intention.
So when I found a trivial issue, what should I do?

Thank you for responses, anyway.

@minux minux reopened this Feb 27, 2016
@minux
Copy link
Member

@minux minux commented Feb 27, 2016

@bradfitz bradfitz added this to the Unplanned milestone Feb 27, 2016
@hirochachacha
Copy link
Contributor Author

@hirochachacha hirochachacha commented Feb 27, 2016

Thanks! I'll send a CL.

and fix the syscall package if the fix is simple.

Yes it's simple. For example, we can implement Time by gettimeofday,
Utime by utimes and Getrlimit by ugetrlimit.

But corresponding syscall numbers are already declaraed by api/go*.txt.
(i.e. SYS_TIME, SYS_UMOUNT, etc ...)

How can I treat these?

@davecheney
Copy link
Contributor

@davecheney davecheney commented Feb 27, 2016

@hirochachacha in general the syscall package exists to support the standard library. If the standard library does not need or use the function (even if it is buggy or not implemented), the preferred way is to fix it in the x/sys/unix repository.

@hirochachacha
Copy link
Contributor Author

@hirochachacha hirochachacha commented Feb 27, 2016

@davecheney I'm confusing. which should I follow?

@davecheney
Copy link
Contributor

@davecheney davecheney commented Feb 27, 2016

@hirochachacha we cannot change any of the public values declared in the syscall package. You may have to declare some private constants instead.

Also, please try to make as small as possible a fix. If the issue is not apparent to callers of this code, ie the rest of the standard library, the recommendation is to fix the issue in the x/sys/unix package instead.

@minux
Copy link
Member

@minux minux commented Feb 27, 2016

Those SYS_* constants are still correct, it's just they
can't be used by the current kernel, so leave them as
they are. The syscall numbers are currently automatically
generated, so it probably hard to add comments to
those numbers, but I think most people will use existing
functions rather than calling Syscall directly anyway, so
it probably won't affect much.

Just fix the implementation to use the correct syscalls.

For the functions in syscall package, I think if the fix
is simple, we can fix those functions. The syscall package
is to serve the standard library, that's the reason why
we don't extend it anymore, but existing bugs should
still be fixed, IMO.

@hirochachacha
Copy link
Contributor Author

@hirochachacha hirochachacha commented Feb 27, 2016

Can you please talk to each other? I want to follow the decision.

@davecheney
Copy link
Contributor

@davecheney davecheney commented Feb 27, 2016

I am sorry for the confusion. I will let you and @minux decide what to do.

On Sat, Feb 27, 2016 at 8:10 PM, hirochachacha notifications@github.com
wrote:

Can you please talk to each other? I want to follow the decision.


Reply to this email directly or view it on GitHub
#14524 (comment).

@hirochachacha
Copy link
Contributor Author

@hirochachacha hirochachacha commented Feb 27, 2016

No worries. Thanks! I will follow @minux 's decision here.

@minux
Copy link
Member

@minux minux commented Feb 27, 2016

To make my suggestions more concrete:

  1. don't change constants in either syscall or x/sys/unix
    (my understanding is that the numbers are correct but they
    are not implemented by the kernel anymore. please correct
    me if I'm wrong.) I'm also open to delete those constants in
    x/sys/unix, but we have to keep those in the syscall package.
  2. move affected functions from auto-generated zsyscall
    file into a linux/arm specific file, and implement them correctly.
  3. For x/sys/unix only, consider adding tests for syscalls that
    are straightforward to test. For example, we can test time.Now()
    and unix.Time agrees with each other. The test should be
    generic so that it runs on all linux platforms (not only arm).
@hirochachacha
Copy link
Contributor Author

@hirochachacha hirochachacha commented Feb 27, 2016

  1. don't change constants in either syscall or x/sys/unix (my understanding is that the numbers are correct but they are not implemented by the kernel anymore. please correct me if I'm wrong.) I'm also open to delete those constants in x/sys/unix, but we have to keep those in the syscall package.

I investigated it. according to this, EABI is supported since 2.6.16.
I downloaded linux-2.6.16.

Here is related source code:

include/asm-arm/unistd.h

#define __NR_time           (__NR_SYSCALL_BASE+ 13)

...

#if !defined(CONFIG_AEABI) || defined(CONFIG_OABI_COMPAT)
#define __ARCH_WANT_SYS_TIME
#define __ARCH_WANT_SYS_OLDUMOUNT
#define __ARCH_WANT_SYS_ALARM
#define __ARCH_WANT_SYS_UTIME
#define __ARCH_WANT_SYS_OLD_GETRLIMIT
#define __ARCH_WANT_OLD_READDIR
#define __ARCH_WANT_SYS_SOCKETCALL
#endif

kernel/time.c

#ifdef __ARCH_WANT_SYS_TIME

asmlinkage long sys_time(time_t __user * tloc)
{
...
}
...
#endif /* __ARCH_WANT_SYS_TIME */

arch/arm/kernel/calls.S

/* 0 */     CALL(sys_restart_syscall)
...
/* 10 */    CALL(sys_unlink)
        CALL(sys_execve_wrapper)
        CALL(sys_chdir)
        CALL(OBSOLETE(sys_time))    /* used by libc4 */
        CALL(sys_mknod)
/* 15 */    CALL(sys_chmod)
...

syscall table is defined by calls.S . So syscall numbers are defined, but functions are not.

  1. move affected functions from auto-generated zsyscall file into a linux/arm specific file, and implement them correctly.

I see.

  1. For x/sys/unix only, consider adding tests for syscalls that are straightforward to test. For example, we can test time.Now() and unix.Time agrees with each other. The test should be generic so that it runs on all linux platforms (not only arm).

I guess you mean time.Unix. I understand.

Thank you.

@hirochachacha
Copy link
Contributor Author

@hirochachacha hirochachacha commented Feb 27, 2016

  1. For x/sys/unix only, consider adding tests for syscalls that are straightforward to test. For example, we can test time.Now() and unix.Time agrees with each other. The test should be generic so that it runs on all linux platforms (not only arm).

I guess you mean time.Unix. I understand.

I'm really sorry. I didn't understand it. what "agrees with" exactly mean?
Is this mean: abs(timeA - timeB) < some value ?

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 27, 2016

CL https://golang.org/cl/19945 mentions this issue.

@minux
Copy link
Member

@minux minux commented Feb 28, 2016

On Sat, Feb 27, 2016 at 6:24 AM, hirochachacha notifications@github.com
wrote:

  1. For x/sys/unix only, consider adding tests for syscalls that are
    straightforward to test. For example, we can test time.Now() and unix.Time
    agrees with each other. The test should be generic so that it runs on all
    linux platforms (not only arm).

I guess you mean time.Unix. I understand.

I'm really sorry. I didn't understand it. what "agrees with" exactly mean?
Is this mean: abs(timeA - timeB) < some value ?

Repeat ten times and see if timeA == timeB at least once?

@hirochachacha
Copy link
Contributor Author

@hirochachacha hirochachacha commented Feb 28, 2016

Repeat ten times and see if timeA == timeB at least once?

Is this preferable to timeA - timeB < n seconds? and why?
Maybe both of them use CLOCK_REALTIME, so sometimes it failed? I'm not sure.

@minux
Copy link
Member

@minux minux commented Feb 28, 2016

gopherbot pushed a commit to golang/sys that referenced this issue Feb 28, 2016
Some system calls are obsolete and no longer available for EABI.
This CL replace such system call usages.

Updates golang/go#14524

Change-Id: Ib99b239455ca677e46d7097911904c45119051bd
Reviewed-on: https://go-review.googlesource.com/19945
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@hirochachacha
Copy link
Contributor Author

@hirochachacha hirochachacha commented Mar 1, 2016

I sent a CL for syscall package.
I've rethought about deleting obsolete syscall numbers, and don't want to change it now.
So if the CL is acceptable, then it's OK to close this.

Thank you.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 1, 2016

CL https://golang.org/cl/20004 mentions this issue.

@hirochachacha
Copy link
Contributor Author

@hirochachacha hirochachacha commented Mar 1, 2016

The CL wasn't approved. But I suppose no one wants to continue this.
So I close it.
Thank you for responses.

@golang golang locked and limited conversation to collaborators Mar 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.