diff mbox

fallocate: pass off_t in register pair correctly for 64-bit off_t

Message ID 1466746995-25982-1-git-send-email-ynorov@caviumnetworks.com
State New, archived
Headers

Commit Message

Yury Norov June 24, 2016, 5:43 a.m. UTC 
  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(+)

Comments

Joseph Myers June 24, 2016, 7:35 a.m. UTC | #1 
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.)
Mike Frysinger June 24, 2016, 8:30 a.m. UTC | #2 
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
Yury Norov June 24, 2016, 8:52 a.m. UTC | #3 
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.
Yury Norov June 24, 2016, 8:53 a.m. UTC | #4 
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.
Mike Frysinger June 24, 2016, 4:15 p.m. UTC | #5 
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
Arnd Bergmann June 24, 2016, 7:53 p.m. UTC | #6 
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 mbox

Patch

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