[PATCH] Fix building xfsprogs on 32-bit platforms

[PATCH] Fix building xfsprogs on 32-bit platforms

[Eric Biggers]

Since commit 7fda99a0c297 ("xfs.h: require transparent LFS for all
users"), building xfsprogs fails on a 32-bit platform because it does
not define _FILE_OFFSET_BITS=64 itself.  Fix it by adding this to CFLAGS
in builddefs.in.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/builddefs.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/builddefs.in b/include/builddefs.in
index aeb2905..5219071 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -25,7 +25,7 @@ OPTIMIZER = @opt_build@
 MALLOCLIB = @malloc_lib@
 LOADERFLAGS = @LDFLAGS@
 LTLDFLAGS = @LDFLAGS@
-CFLAGS = @CFLAGS@
+CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64
 
 LIBRT = @librt@
 LIBUUID = @libuuid@
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH] Fix building xfsprogs on 32-bit platforms

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] Fix building xfsprogs on 32-bit platforms

[Dave Chinner]

On Mon, Nov 28, 2016 at 02:42:35PM -0800, Eric Biggers wrote:
> Since commit 7fda99a0c297 ("xfs.h: require transparent LFS for all
> users"), building xfsprogs fails on a 32-bit platform because it does
> not define _FILE_OFFSET_BITS=64 itself.  Fix it by adding this to CFLAGS
> in builddefs.in.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  include/builddefs.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/builddefs.in b/include/builddefs.in
> index aeb2905..5219071 100644
> --- a/include/builddefs.in
> +++ b/include/builddefs.in
> @@ -25,7 +25,7 @@ OPTIMIZER = @opt_build@
>  MALLOCLIB = @malloc_lib@
>  LOADERFLAGS = @LDFLAGS@
>  LTLDFLAGS = @LDFLAGS@
> -CFLAGS = @CFLAGS@
> +CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64

So, umm, why aren't these lines in configure.ac setting this already
on your system?

AC_SYS_LARGEFILE
AC_NEED_LFS

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] Fix building xfsprogs on 32-bit platforms

[Eric Biggers]

On Wed, Nov 30, 2016 at 08:44:56AM +1100, Dave Chinner wrote:
> On Mon, Nov 28, 2016 at 02:42:35PM -0800, Eric Biggers wrote:
> > Since commit 7fda99a0c297 ("xfs.h: require transparent LFS for all
> > users"), building xfsprogs fails on a 32-bit platform because it does
> > not define _FILE_OFFSET_BITS=64 itself.  Fix it by adding this to CFLAGS
> > in builddefs.in.
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >  include/builddefs.in | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/builddefs.in b/include/builddefs.in
> > index aeb2905..5219071 100644
> > --- a/include/builddefs.in
> > +++ b/include/builddefs.in
> > @@ -25,7 +25,7 @@ OPTIMIZER = @opt_build@
> >  MALLOCLIB = @malloc_lib@
> >  LOADERFLAGS = @LDFLAGS@
> >  LTLDFLAGS = @LDFLAGS@
> > -CFLAGS = @CFLAGS@
> > +CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64
> 
> So, umm, why aren't these lines in configure.ac setting this already
> on your system?
> 
> AC_SYS_LARGEFILE
> AC_NEED_LFS
> 
> Cheers,
> 

AC_SYS_LARGEFILE puts the _FILE_OFFSET_BITS definition in the generated config
header, not in CFLAGS.  Normally projects use a config.h.in template that's
generated from configure.ac using autoheader.  xfsprogs doesn't do this but
rather uses a non-generated file include/platform_defs.h.in.  Since there is no
"#undef _FILE_OFFSET_BITS" in platform_defs.h.in, '#define _FILE_OFFSET_BITS 64'
doesn't gets included in platform_defs.h.  Also, even if it was, then I think it
wouldn't always work because not all .c files in xfsprogs have platform_defs.h
as their first #include.

The reason AC_NEED_LFS still passes is that 'configure' includes a temporary
header confdefs.h rather than the final generated header, and the temporary
header includes the _FILE_OFFSET_BITS definition.

Eric

Re: [PATCH] Fix building xfsprogs on 32-bit platforms

[Dave Chinner]

On Tue, Nov 29, 2016 at 02:21:35PM -0800, Eric Biggers wrote:
> On Wed, Nov 30, 2016 at 08:44:56AM +1100, Dave Chinner wrote:
> > On Mon, Nov 28, 2016 at 02:42:35PM -0800, Eric Biggers wrote:
> > > Since commit 7fda99a0c297 ("xfs.h: require transparent LFS for all
> > > users"), building xfsprogs fails on a 32-bit platform because it does
> > > not define _FILE_OFFSET_BITS=64 itself.  Fix it by adding this to CFLAGS
> > > in builddefs.in.
> > > 
> > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > ---
> > >  include/builddefs.in | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/include/builddefs.in b/include/builddefs.in
> > > index aeb2905..5219071 100644
> > > --- a/include/builddefs.in
> > > +++ b/include/builddefs.in
> > > @@ -25,7 +25,7 @@ OPTIMIZER = @opt_build@
> > >  MALLOCLIB = @malloc_lib@
> > >  LOADERFLAGS = @LDFLAGS@
> > >  LTLDFLAGS = @LDFLAGS@
> > > -CFLAGS = @CFLAGS@
> > > +CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64
> > 
> > So, umm, why aren't these lines in configure.ac setting this already
> > on your system?
> > 
> > AC_SYS_LARGEFILE
> > AC_NEED_LFS
> > 
> > Cheers,
> > 
> 
> AC_SYS_LARGEFILE puts the _FILE_OFFSET_BITS definition in the generated config
> header, not in CFLAGS. 

<sigh> It's times like this that I really hate software.

This is not what the documentation implies occurs:

https://www.gnu.org/software/autoconf/manual/autoconf-2.65/html_node/System-Services.html

	Macro: AC_SYS_LARGEFILE

	Arrange for 64-bit file offsets, known as large-file
	support. On some hosts, one must use special compiler
	options to build programs that can access large files.
	Append any such options to the output variable CC. Define
	_FILE_OFFSET_BITS and _LARGE_FILES if necessary.


i.e. it implies that the required LFS options are automatically
appended to CC variable.

> Normally projects use a config.h.in template that's generated from
> configure.ac using autoheader.  xfsprogs doesn't do this but
> rather uses a non-generated file include/platform_defs.h.in.

We didn't add any hoop jumping, because the autoconf documentation
implies it is not necessary. I trusted that the patch submitter
actually tested it on 32 bit systems and that it all worked.

> Since there is no "#undef _FILE_OFFSET_BITS" in
> platform_defs.h.in, '#define _FILE_OFFSET_BITS 64' doesn't gets
> included in platform_defs.h.

platform_defs.h should not need to do this - we handle build
environment specific defines in builddefs.h....

> Also, even if it was, then I think
> it wouldn't always work because not all .c files in xfsprogs have
> platform_defs.h as their first #include.

.... precisely for this reason.

So, builddefs.in should really detect if autoconf set a "large file
required" flag, and in that case add the _FILE_OFFSET_BITS value
that autoconf has generated to the PCFLAGS variable, right? (PCFLAGS
are "Platform CFLAGS")

Or, if we are just going to unconditionally define
-D_FILE_OFFSET_BITS=64 in CFLAGS, then the autoconf checks are
completely useless and should be removed?

> The reason AC_NEED_LFS still passes is that 'configure' includes a
> temporary header confdefs.h rather than the final generated
> header, and the temporary header includes the _FILE_OFFSET_BITS
> definition.

IOWs, stupid code backed by ambiguous documentation means that
developers that try to do the right thing still end up with broken
builds.

Eric, to speed things up a bit next time, can you try to explain how
something is broken in the commit message, not just say "it's
broken, fix it". This avoids the need to go back and forth multiple
times just so we can understand what the problem is that you are
fixing....

Cheers,

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] Fix building xfsprogs on 32-bit platforms

[Eric Biggers]

Hi Dave,

On Wed, Nov 30, 2016 at 10:25:49AM +1100, Dave Chinner wrote:
> 
> So, builddefs.in should really detect if autoconf set a "large file
> required" flag, and in that case add the _FILE_OFFSET_BITS value
> that autoconf has generated to the PCFLAGS variable, right? (PCFLAGS
> are "Platform CFLAGS")
> 
> Or, if we are just going to unconditionally define
> -D_FILE_OFFSET_BITS=64 in CFLAGS, then the autoconf checks are
> completely useless and should be removed?
> 

I don't believe it's possible to use the results of AC_SYS_LARGEFILE without
using a config header that is included *first* in all .c files.

So the other option is to define _FILE_OFFSET_BITS=64 ourselves in CFLAGS.  (Or
PCFLAGS for just the platforms that need it; I know it's needed on 32-bit Linux,
but I don't know about other platforms.)  And yes, in that case the autoconf
macros are useless and should be removed.

Sorry for not explaining more in the commit message --- I hadn't looked into the
details of what was going on with the autoconf macros (or that they were
recently added, for that matter) until you pointed them out.

Eric

> > The reason AC_NEED_LFS still passes is that 'configure' includes a
> > temporary header confdefs.h rather than the final generated
> > header, and the temporary header includes the _FILE_OFFSET_BITS
> > definition.
> 
> IOWs, stupid code backed by ambiguous documentation means that
> developers that try to do the right thing still end up with broken
> builds.
> 
> Eric, to speed things up a bit next time, can you try to explain how
> something is broken in the commit message, not just say "it's
> broken, fix it". This avoids the need to go back and forth multiple
> times just so we can understand what the problem is that you are
> fixing....
> 
> Cheers,
> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com