Message ID | 1466746995-25982-1-git-send-email-ynorov@caviumnetworks.com |
---|---|
State | New |
Headers | show |
On Fri, 24 Jun 2016, Yury Norov wrote: > diff --git a/sysdeps/unix/sysv/linux/fallocate.c b/sysdeps/unix/sysv/linux/fallocate.c > index 6a58a5f..8a7149f 100644 > --- a/sysdeps/unix/sysv/linux/fallocate.c > +++ b/sysdeps/unix/sysv/linux/fallocate.c > @@ -15,6 +15,7 @@ > License along with the GNU C Library; if not, see > <http://www.gnu.org/licenses/>. */ > > +#ifndef __OFF_T_MATCHES_OFF64_T > #include <errno.h> Does the header defining this macro get included implicitly from the command line? If not, you need to include appropriate headers before testing it.... (It might be a good idea, before doing anything more based on such macros, to do a preparatory patch series converting them to 0/1 form with #if used, instead of undefined/defined with #ifdef, so that -Werror -Wundef detects any cases where required headers are not included.)
On 24 Jun 2016 08:43, Yury Norov wrote: > sysdeps/unix/sysv/linux/fallocate.c contains wide-spreaded hack for creating > register pair from 32-bit signed value: > LONG_LONG_PAIR (val >> 31, val) > > It works well for off_t if it is 32-bit lenght. Modern ABIs requres 64-bit off_t, > so this hack doesn't work for them. In this patch, fallocate handler is taken from > fallocate64.c, depending on __OFF_T_MATCHES_OFF64_T. what ABIs exactly are you referring to ? why is this change specific to fallocate ? seems like this argument could be made about any of the funcs that operate on off_t's. what about posix_fallocate ? posix_fadvise ? > The other option to fix it is to introduce macro that creates the pair correctly > for both 32- and 64-bit off_t, like this: if off_t & off64_t are the same size, we don't want duplicate symbols that do the same thing. -mike
On Fri, Jun 24, 2016 at 04:30:23AM -0400, Mike Frysinger wrote: > On 24 Jun 2016 08:43, Yury Norov wrote: > > sysdeps/unix/sysv/linux/fallocate.c contains wide-spreaded hack for creating > > register pair from 32-bit signed value: > > LONG_LONG_PAIR (val >> 31, val) > > > > It works well for off_t if it is 32-bit lenght. Modern ABIs requres 64-bit off_t, > > so this hack doesn't work for them. In this patch, fallocate handler is taken from > > fallocate64.c, depending on __OFF_T_MATCHES_OFF64_T. > > what ABIs exactly are you referring to ? AARCH64/ILP32. It would be any modern 32-bit API, as off_t should be 64-bit for them > why is this change specific to fallocate ? seems like this argument > could be made about any of the funcs that operate on off_t's. what > about posix_fallocate ? posix_fadvise ? Because it's RFC. If this one is OK, I'll prepare correct patch for all affected functions. I also wrote this small patch to ask what way is more preferable - with macro, or with redirection. > > The other option to fix it is to introduce macro that creates the pair correctly > > for both 32- and 64-bit off_t, like this: > > if off_t & off64_t are the same size, we don't want duplicate symbols > that do the same thing. > -mike So it 1st version is OK, I'll prepare full patch with all needed redirections. Yury.
On Fri, Jun 24, 2016 at 07:35:09AM +0000, Joseph Myers wrote: > On Fri, 24 Jun 2016, Yury Norov wrote: > > > diff --git a/sysdeps/unix/sysv/linux/fallocate.c b/sysdeps/unix/sysv/linux/fallocate.c > > index 6a58a5f..8a7149f 100644 > > --- a/sysdeps/unix/sysv/linux/fallocate.c > > +++ b/sysdeps/unix/sysv/linux/fallocate.c > > @@ -15,6 +15,7 @@ > > License along with the GNU C Library; if not, see > > <http://www.gnu.org/licenses/>. */ > > > > +#ifndef __OFF_T_MATCHES_OFF64_T > > #include <errno.h> > > Does the header defining this macro get included implicitly from the > command line? If not, you need to include appropriate headers before > testing it.... (It might be a good idea, before doing anything more based > on such macros, to do a preparatory patch series converting them to 0/1 > form with #if used, instead of undefined/defined with #ifdef, so that > -Werror -Wundef detects any cases where required headers are not > included.) > > -- > Joseph S. Myers > joseph@codesourcery.com Thank you, I'll #include <sys/types> for it.
On 24 Jun 2016 11:52, Yury Norov wrote: > On Fri, Jun 24, 2016 at 04:30:23AM -0400, Mike Frysinger wrote: > > On 24 Jun 2016 08:43, Yury Norov wrote: > > > sysdeps/unix/sysv/linux/fallocate.c contains wide-spreaded hack for creating > > > register pair from 32-bit signed value: > > > LONG_LONG_PAIR (val >> 31, val) > > > > > > It works well for off_t if it is 32-bit lenght. Modern ABIs requres 64-bit off_t, > > > so this hack doesn't work for them. In this patch, fallocate handler is taken from > > > fallocate64.c, depending on __OFF_T_MATCHES_OFF64_T. > > > > what ABIs exactly are you referring to ? > > AARCH64/ILP32. It would be any modern 32-bit API, as off_t should be > 64-bit for them ignoring ILP32 ABIs (which are running on native 64-bit hardware and have full access to the 64-bit registers), no new 32-bit port is being defined with 64-bit off_t types. they follow existing style where LFS is used. whether we want to change all 32-bit ports to be LFS-enabled by default is a different question ... we'd still want them to be consistent. -mike
On Friday, June 24, 2016 12:15:52 PM CEST Mike Frysinger wrote: > On 24 Jun 2016 11:52, Yury Norov wrote: > > On Fri, Jun 24, 2016 at 04:30:23AM -0400, Mike Frysinger wrote: > > > On 24 Jun 2016 08:43, Yury Norov wrote: > > > > sysdeps/unix/sysv/linux/fallocate.c contains wide-spreaded hack for creating > > > > register pair from 32-bit signed value: > > > > LONG_LONG_PAIR (val >> 31, val) > > > > > > > > It works well for off_t if it is 32-bit lenght. Modern ABIs requres 64-bit off_t, > > > > so this hack doesn't work for them. In this patch, fallocate handler is taken from > > > > fallocate64.c, depending on __OFF_T_MATCHES_OFF64_T. > > > > > > what ABIs exactly are you referring to ? > > > > AARCH64/ILP32. It would be any modern 32-bit API, as off_t should be > > 64-bit for them > > ignoring ILP32 ABIs (which are running on native 64-bit hardware and have > full access to the 64-bit registers), no new 32-bit port is being defined > with 64-bit off_t types. they follow existing style where LFS is used. > > whether we want to change all 32-bit ports to be LFS-enabled by default is > a different question ... we'd still want them to be consistent. I've been pushing the move for 64-bit off_t for both RISC-V and ARM64/ILP32, mainly because it seems like the right thing to do (the kernel has stopped supporting 32-bit off_t for new architectures many years ago), and there were no technical arguments[1] in favor of 32-bit off_t emulation in glibc beyond the fact that someone has to do the work to implement the new default once. If you feel strongly about it for some reason, I guess we can go back to defaulting to 32-bit off_t on aarch64/ilp32, but I absolutely agree that we want to be consistent here, and then we should do the same on RISC-V. Arnd [1] actually one argument in favor of 32-bit off_t would be to wait until the kernel can also do 64-bit time_t on all architectures, and then change both at the same time for all architectures merged after that point.
diff --git a/sysdeps/unix/sysv/linux/fallocate.c b/sysdeps/unix/sysv/linux/fallocate.c index 6a58a5f..8a7149f 100644 --- a/sysdeps/unix/sysv/linux/fallocate.c +++ b/sysdeps/unix/sysv/linux/fallocate.c @@ -15,6 +15,7 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#ifndef __OFF_T_MATCHES_OFF64_T #include <errno.h> #include <fcntl.h> #include <sysdep-cancel.h> @@ -33,3 +34,5 @@ fallocate (int fd, int mode, __off_t offset, __off_t len) return -1; #endif } + +#endif /* __OFF_T_MATCHES_OFF64_T */ diff --git a/sysdeps/unix/sysv/linux/fallocate64.c b/sysdeps/unix/sysv/linux/fallocate64.c index 8e76d6f..f4f73d5 100644 --- a/sysdeps/unix/sysv/linux/fallocate64.c +++ b/sysdeps/unix/sysv/linux/fallocate64.c @@ -35,3 +35,7 @@ fallocate64 (int fd, int mode, __off64_t offset, __off64_t len) return -1; #endif } + +#ifdef __OFF_T_MATCHES_OFF64_T +weak_alias(fallocate64, fallocate) +#endif
sysdeps/unix/sysv/linux/fallocate.c contains wide-spreaded hack for creating register pair from 32-bit signed value: LONG_LONG_PAIR (val >> 31, val) It works well for off_t if it is 32-bit lenght. Modern ABIs requres 64-bit off_t, so this hack doesn't work for them. In this patch, fallocate handler is taken from fallocate64.c, depending on __OFF_T_MATCHES_OFF64_T. The other option to fix it is to introduce macro that creates the pair correctly for both 32- and 64-bit off_t, like this: #ifndef __OFF_T_MATCHES_OFF64_T #define CREATE_OFF_PAIR(val) ((val) < 0 ? (-1) : 0 , (val)) #else #define CREATE_OFF_PAIR(val) ((val) >> 32, (val)) #endif If 2nd option is looking more appropriated, I can prepare the patch for all affected syscalls. Signed-off-by: Yury Norov <ynorov@caviumnetworks.com> --- sysdeps/unix/sysv/linux/fallocate.c | 3 +++ sysdeps/unix/sysv/linux/fallocate64.c | 4 ++++ 2 files changed, 7 insertions(+)