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

x/sys/unix: clean up the AIX Timespec/StTimespec difference #32073

Closed
bradfitz opened this issue May 16, 2019 · 7 comments
Closed

x/sys/unix: clean up the AIX Timespec/StTimespec difference #32073

bradfitz opened this issue May 16, 2019 · 7 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-AIX
Milestone

Comments

@bradfitz
Copy link
Contributor

(From discussion on https://go-review.googlesource.com/c/sys/+/177437)

AIX is the only GOOS in x/sys/unix that has Stat_t fields of type StTimespec instead of Timespec. But AIX also is unique in that it has both Timespec and StTimespec.

But seems like we could fix that if we re-generate the AIX Go files with __USE_XOPEN2K8 set?

/cc @trex58 @tklauser @paulzhol @ianlancetaylor @Helflym

@bradfitz bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-AIX labels May 16, 2019
@gopherbot gopherbot added this to the Unreleased milestone May 16, 2019
@Helflym
Copy link
Contributor

Helflym commented May 16, 2019

There is no __USE_XOPEN2K8 on AIX.

Looking at the code Yuval gave in this CL :

 111 # ifdef __USE_XOPEN2K8
 112     /* Nanosecond resolution timestamps are stored in a format
 113        equivalent to 'struct timespec'.  This is the type used
 114        whenever possible but the Unix namespace rules do not allow the
 115        identifier 'timespec' to appear in the <sys/stat.h> header.
 116        Therefore we have to handle the use of this header in strictly
 117        standard-compliant sources special.  */
 118     struct timespec st_atim;            /* Time of last access.  */
 119     struct timespec st_mtim;            /* Time of last modification.  */
 120     struct timespec st_ctim;            /* Time of last status change.  */
 121 # else
 122     __time_t st_atime;                  /* Time of last access.  */
 123     unsigned long int st_atimensec;     /* Nscecs of last access.  */
 124     __time_t st_mtime;                  /* Time of last modification.  */
 125     unsigned long int st_mtimensec;     /* Nsecs of last modification.  */
 126     __time_t st_ctime;                  /* Time of last status change.  */
 127     unsigned long int st_ctimensec;     /* Nsecs of last status change.  */
 128 # endif

we have something similar on AIX.

However, when using the equivalent of __USE_XOPEN2K8,

 118     struct timespec st_atim;            /* Time of last access.  */
 119     struct timespec st_mtim;            /* Time of last modification.  */
 120     struct timespec st_ctim;            /* Time of last status change.  */

are being replaced by

 118     st_timespec_t st_atim;            /* Time of last access.  */
 119     st_timespec_t st_mtim;            /* Time of last modification.  */
 120     st_timespec_t st_ctim;            /* Time of last status change.  */

Therefore, we can't remove the use of st_timespec_t inside Stat_t.
I think the only way to harmonise AIX with other GOOS is to have getters(and setters ?) on Atim, Mtim and Ctim fields of Stat_t.

@paulzhol
Copy link
Member

Are the AIX headers available somwhere? I've found this snippet

in struct stat
#if _XOPEN_SOURCE>=700
        st_timespec_t st_atim;  /* Time of last access  */
        st_timespec_t st_mtim;  /* Time of last data modification */
        st_timespec_t st_ctim;  /* Time of last file status change */
#else
        time_t  st_atime;       /* Time of last access  */
        int     st_atime_n;
        time_t  st_mtime;       /* Time of last data modification */
        int     st_mtime_n;
        time_t  st_ctime;       /* Time of last file status change */
        int     st_ctime_n;
#endif

in struct stat64
#if _XOPEN_SOURCE>=700
        struct timespec64 st_atim;      /* 16: Time of last access */
        struct timespec64 st_mtim;      /* 16: Time of last data mod */
        struct timespec64 st_ctim;      /* 16: Time of last status change */
#else
        time64_t        st_atime;       /* 8: Time of last access */
        int32_t         st_atime_n;     /* 4: nanoseconds */
        int32_t         st_pad1;        /* 4: padding */
        time64_t        st_mtime;       /* 8: Time of last data modification */
        int32_t         st_mtime_n;     /* 4: nanoseconds */
        int32_t         st_pad2;        /* 4: padding */
        time64_t        st_ctime;       /* 8: Time of last file status change */
        int32_t         st_ctime_n;     /* 4: nanoseconds */
        int32_t         st_pad3;        /* 4: padding */
#endif

and the struct timespec and st_timespec
#if _XOPEN_SOURCE>=700
#ifndef _TIMESPEC
#define _TIMESPEC
struct timespec {
        time_t  tv_sec;         /* seconds */
        long    tv_nsec;        /* and nanoseconds */
};
#endif

/* internal timespec to preserve binary compatibility */
typedef struct st_timespec {
        time_t  tv_sec;         /* seconds */
        int     tv_nsec;        /* and nanoseconds */
A
} st_timespec_t;
#endif

Is this still the representation? If so I'm not sure I understand what is causing the extra padding at the end of the StTimespec struct (I would expect cgo -godefs to inject the padding in between the tv_sec and tv_nsec fields).

@paulzhol
Copy link
Member

I'm not sure if it's a good idea, but we could rewrite the Stat_t members to be Timespec:
on GOARCH==ppc StTimespec and Timespec are identical.
on GOARCH==ppc64 they occupy the same memory layout and because this is a big-endian architecture (correct me if I'm mistaken) if we do a Timespec.Nsec >> 4 in the syscall wrapper the nsec values should be the same as if they returned as StTimespec.

@Helflym
Copy link
Contributor

Helflym commented May 17, 2019

Are the AIX headers available somwhere?

I don't think so. But the snippet you gave is the correct one. Not idea about the cgo padding though.

I'm not sure if it's a good idea, but we could rewrite the Stat_t members to be Timespec:
on GOARCH==ppc StTimespec and Timespec are identical.
on GOARCH==ppc64 they occupy the same memory layout and because this is a big-endian architecture (correct me if I'm mistaken) if we do a Timespec.Nsec >> 4 in the syscall wrapper the nsec values should be the same as if they returned as StTimespec.

GOARCH==ppc is not available on Go toolchain only on gccgo. But, it should also be possible to modify the script generating the ztypes equivalent quite easily.
About ppc64, I don't see any reason why your idea won't work. But as you say "is it a good idea" to modify Stat_t ? People might not understand with Stat_t has not the correct layout and it might cause problems with the Go1 compatibility.
Anyway, if @ianlancetaylor, @bradfitz or others do agree with this kind of solution, I'll give it a try.

@paulzhol
Copy link
Member

People might not understand with Stat_t has not the correct layout and it might cause problems with the Go1 compatibility.

This is for x/sys/unix for now, so I think there are no issues with Go1 compatibility.
BTW we already have something of the sorts for FreeBSD 11 where we return FreeBSD 12 compatible structures (mostly extending inode numbers to 64 bit).
It works pretty well with Stat_t, Statvfs_t type of structures. There are some pain points with byte buffers containing Dirent's though.

@Helflym
Copy link
Contributor

Helflym commented May 17, 2019

Arf, I thought it was for syscall package, my bad.
Ok I'll check as soon as I can.

@gopherbot
Copy link

Change https://golang.org/cl/177838 mentions this issue: unix: remove StTimespec type on AIX

@golang golang locked and limited conversation to collaborators May 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-AIX
Projects
None yet
Development

No branches or pull requests

4 participants